Patch for Bug 30413
Hello clang developers, I discovered a bug that affects Objective-C programs compiled with clang on Linux. The current code was not emitting Objective-C class name metadata, which made it impossible for programs to do runtime type introspection. I opened a bug report that describes the problem and provides a test program that reproduces it: https://llvm.org/bugs/show_bug.cgi?id=30413 I've attached an svn-generated patch file: the patch changes one argument to the getObjCEncodingForTypeImpl function from false to true, so that Objective-C class names are encoded. If this looks OK, it would be great if it could be added to the trunk. Once that is done, the bug can of course be closed. Thank you. Sincerely, David Lobron PatchForBug30413.patch Description: PatchForBug30413.patch ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Patch for Bug 30413
Hi Vedant, Thanks for the reply! Yes, I have a small test case- I actually attached it to the original bug report (https://llvm.org/bugs/show_bug.cgi?id=30413), but I will go ahead and try adding it to clang/test/CodeGenObjC, or extend a case if possible. Is it OK to make changes to CodeGenObjC directly, or should I submit a diff of those changes to this list, along with my original patch? Thanks! --David > On Feb 9, 2017, at 8:22 PM, Vedant Kumar wrote: > > Hi David, > > Thanks for tracking this down. > > Do you have a small reproducer to use as a test case? Ideally, you'd add a > regression test to clang/test/CodeGenObjC, or extend an existing one. > > vedant > >> On Feb 7, 2017, at 6:42 PM, Lobron, David via cfe-commits >> wrote: >> >> Hello clang developers, >> >> I discovered a bug that affects Objective-C programs compiled with clang on >> Linux. The current code was not emitting Objective-C class name metadata, >> which made it impossible for programs to do runtime type introspection. I >> opened a bug report that describes the problem and provides a test program >> that reproduces it: >> >> https://llvm.org/bugs/show_bug.cgi?id=30413 >> >> I've attached an svn-generated patch file: the patch changes one argument to >> the getObjCEncodingForTypeImpl function from false to true, so that >> Objective-C class names are encoded. If this looks OK, it would be great if >> it could be added to the trunk. Once that is done, the bug can of course be >> closed. >> >> Thank you. >> >> Sincerely, >> >> David Lobron >> >> ___ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Patch for Bug 30413, including test case
Hi All, I am re-submitting my patch for Bug 30413, this time with a test case included as well (ivar-type-encoding.m). The test case file should be added to clang/test/CodeGenObjC. The test verifies that correct metadata is emitted by clang for an object-valued instance variable. I've verified that the test passes when the patch has been applied to ASTContext.cpp, and fails otherwise. Please let me know if this looks OK, or if any additional information is needed. Thanks, David ivar-type-encoding.m Description: ivar-type-encoding.m PatchForBug30413.patch Description: PatchForBug30413.patch ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Patch for Bug 30413, including test case
Hi Akira, Pardon my slow reply here- I was traveling and just got back to work email. I will check into this as soon as I can, and get back to you. Thank you, David > On Feb 20, 2017, at 12:04 AM, Akira Hatanaka wrote: > > This patch changes the encoding of an id conforming to a protocol, which I > think was not intended: For example: > > @encode(id) > > Would passing IVD to the call to getObjCEncodingForType in > CGObjCGNU::GenerateClass solve the problem? > >> On Feb 15, 2017, at 1:59 PM, Lobron, David via cfe-commits >> wrote: >> >> Hi All, >> >> I am re-submitting my patch for Bug 30413, this time with a test case >> included as well (ivar-type-encoding.m). The test case file should be added >> to clang/test/CodeGenObjC. The test verifies that correct metadata is >> emitted by clang for an object-valued instance variable. I've verified that >> the test passes when the patch has been applied to ASTContext.cpp, and fails >> otherwise. >> >> Please let me know if this looks OK, or if any additional information is >> needed. >> >> Thanks, >> >> David >> >> ___ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Patch for Bug 30413, including test case
Hi Akira, Pardon my slowness in addressing your question. > This patch changes the encoding of an id conforming to a protocol, which I > think was not intended: For example: > > @encode(id) > > Would passing IVD to the call to getObjCEncodingForType in > CGObjCGNU::GenerateClass solve the problem? Are you suggesting that passing IVD instead of just IVD->getType() will tell getObjCEncodingForType whether the code being compiled is a protocol, as opposed to an object? I'm a little confused here, because IVD is an object of type ObjCIvarDecl, which is supposed to represent an instance variable, not a protocol. Also, I don't see anything in the definition of ObjCIvarDecl (in AST/DeclObjC.h) that would tell us whether it represents an instance variable or a protocol. Can you explain the case you are concerned about a little more? If IVD can in fact tell us whether the code being compiled is an object, then we can make the EncodeClassNames argument of getObjCEncodingForTypeImpl true for ivars and false for protocols. Thanks! --David ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Patch for Bug 30413, including test case
Hi Akira,
> My concern is that the patch changes the encoding of @encode(id) on
> Darwin, which I think isn’t what you are trying to fix. If you compile the
> following code with command “clang -cc1 -triple x86_64-apple-macosx”, the
> type encoding changes after applying the patch.
>
> const char *foo() {
> return @encode(id);
> }
>
> It seems like you can fix your problem without affecting Darwin by passing an
> extra argument to getObjCEncodingForType, just like
> CGObjCCommonMac::GetMethodVarType does.
Ah, thanks- I understand now. Yes, this change seems a lot safer, and I
verified that it passes my test. I've attached my new patch file, and I've
also attached the test again. Please let me know if this works for you or if
you think it needs any additional work.
--David
CGObjCGNU.cpp.patch
Description: CGObjCGNU.cpp.patch
ivar-type-encoding.m
Description: ivar-type-encoding.m
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Patch for Bug 30413, including test case
Hi Akira,
Thank you very much! Please let me know if I need to take any further steps
beyond this email to cfe-commits in order for the patch and the unit test to be
committed.
Thanks,
David
> On Mar 9, 2017, at 4:46 PM, Akira Hatanaka wrote:
>
> Hi David,
>
> The patch looks good to me.
>
>> On Mar 9, 2017, at 1:01 PM, Lobron, David wrote:
>>
>> Hi Akira,
>>
>>> My concern is that the patch changes the encoding of @encode(id)
>>> on Darwin, which I think isn’t what you are trying to fix. If you compile
>>> the following code with command “clang -cc1 -triple x86_64-apple-macosx”,
>>> the type encoding changes after applying the patch.
>>>
>>> const char *foo() {
>>> return @encode(id);
>>> }
>>>
>>> It seems like you can fix your problem without affecting Darwin by passing
>>> an extra argument to getObjCEncodingForType, just like
>>> CGObjCCommonMac::GetMethodVarType does.
>>
>> Ah, thanks- I understand now. Yes, this change seems a lot safer, and I
>> verified that it passes my test. I've attached my new patch file, and I've
>> also attached the test again. Please let me know if this works for you or
>> if you think it needs any additional work.
>>
>> --David
>>
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: Patch for Bug 30413, including test case
Yes, please, if you don't mind! I'd like to commit both the path and the unit
test, if possible.
Thanks,
David
> On Mar 13, 2017, at 12:47 PM, Akira Hatanaka wrote:
>
> Do you need someone to commit this patch for you?
>
>> On Mar 10, 2017, at 6:44 AM, Lobron, David wrote:
>>
>> Hi Akira,
>>
>> Thank you very much! Please let me know if I need to take any further steps
>> beyond this email to cfe-commits in order for the patch and the unit test to
>> be committed.
>>
>> Thanks,
>>
>> David
>>
>>> On Mar 9, 2017, at 4:46 PM, Akira Hatanaka wrote:
>>>
>>> Hi David,
>>>
>>> The patch looks good to me.
>>>
On Mar 9, 2017, at 1:01 PM, Lobron, David wrote:
Hi Akira,
> My concern is that the patch changes the encoding of
> @encode(id) on Darwin, which I think isn’t what you are trying
> to fix. If you compile the following code with command “clang -cc1
> -triple x86_64-apple-macosx”, the type encoding changes after applying
> the patch.
>
> const char *foo() {
> return @encode(id);
> }
>
> It seems like you can fix your problem without affecting Darwin by
> passing an extra argument to getObjCEncodingForType, just like
> CGObjCCommonMac::GetMethodVarType does.
Ah, thanks- I understand now. Yes, this change seems a lot safer, and I
verified that it passes my test. I've attached my new patch file, and
I've also attached the test again. Please let me know if this works for
you or if you think it needs any additional work.
--David
>>>
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
