r179153 - Don't crash when mangling types defined in ObjC class extensions.

[John McCall]

On Apr 10, 2013, at 11:41 AM, jahanian <fjahanian at apple.com> wrote:
> On Apr 10, 2013, at 11:35 AM, John McCall <rjmccall at apple.com> wrote:
>> On Apr 10, 2013, at 9:11 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>>> On Apr 9, 2013, at 23:08 , John McCall <rjmccall at apple.com> wrote:
>>>> Author: rjmccall
>>>> Date: Wed Apr 10 01:08:21 2013
>>>> New Revision: 179153
>>>> 
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=179153&view=rev
>>>> Log:
>>>> Don't crash when mangling types defined in ObjC class extensions.
>>>> 
>>>> The original test case here was mangling a type name for TBAA,
>>>> but we can provoke this in C++11 easily enough.
>>>> 
>>>> rdar://13434937
>>>> 
>>>> Modified:
>>>>    cfe/trunk/lib/AST/ItaniumMangle.cpp
>>>>    cfe/trunk/test/CodeGenObjCXX/mangle.mm
>>>> 
>>>> Modified: cfe/trunk/lib/AST/ItaniumMangle.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ItaniumMangle.cpp?rev=179153&r1=179152&r2=179153&view=diff
>>>> ==============================================================================
>>>> --- cfe/trunk/lib/AST/ItaniumMangle.cpp (original)
>>>> +++ cfe/trunk/lib/AST/ItaniumMangle.cpp Wed Apr 10 01:08:21 2013
>>>> @@ -1095,6 +1095,15 @@ void CXXNameMangler::mangleUnqualifiedNa
>>>>       mangleSourceName(FD->getIdentifier());
>>>>       break;
>>>>     }
>>>> +
>>>> +    // Class extensions have no name as a category, and it's possible
>>>> +    // for them to be the semantic parent of certain declarations
>>>> +    // (primarily, tag decls defined within declarations).  Such
>>>> +    // declarations will always have internal linkage, so the name
>>>> +    // doesn't really matter, but we shouldn't crash on them.  For
>>>> +    // safety, just handle all ObjC containers here.
>>>> +    if (isa<ObjCContainerDecl>(ND))
>>>> +      break;
>>>> 
>>> 
>>> Is that true? If I do this awful thing:
>>> 
>>> @interface SomeClass ()
>>> @property struct Evil {
>>> 	int i;
>>> 	void print() const;
>>> } myEvil;
>>> @end
>>> 
>>> then Evil gets internal linkage? (I guess that's an acceptable language rule, but I'm not sure where we would have stated that explicitly.)
>> 
>> It does.  If we change our minds on this, then I think the right fix is to semantically parent 'struct Evil' to the surrounding context (which is always the global context) instead of the @interface.  (It can still be lexically parented to the @interface, of course.)
>> 
>>> Full test case (haven't rebuilt Clang yet, so I don't know what happens):
>>> 
>>> @interface SomeClass
>>> @end
>>> 
>>> @interface SomeClass ()
>>> @property struct Evil {
>>> 	
>>> int i;
>>> 	
>>> void print() const;
>>> } myEvil;
>>> @end
>>> 
>>> @implementation SomeClass
>>> @end
>>> 
>>> void Evil::print() const {}
>> 
>> I think that this works is fine.  The actually problematic thing is that you can also declare struct Evil outside of this, and we don't detect it as an invalid redeclaration.  That's probably not good.
>> 
>> A lot of stuff with non-ObjC declarations in ObjC containers is pretty problematic, really.
> 
> C declarations inside ObjC containers does not provide any local context. It should't cause any unforeseen problem if we accept this
> basic fact.

I mean in terms of their linkage, usability, redeclarability, etc. outside of the container.  I agree that the basics of "doesn't have a self context" work fine.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130410/8913d118/attachment.html>