[cfe-commits] r113397 - in /cfe/trunk: include/clang/AST/Type.h lib/AST/Type.cpp lib/Sema/SemaType.cpp test/SemaObjC/legacy-objc-types.m test/SemaObjCXX/legacy-objc-types.mm

John McCall rjmccall at apple.com
Wed Sep 8 14:59:18 PDT 2010


On Sep 8, 2010, at 2:30 PM, Fariborz Jahanian wrote:
> On Sep 8, 2010, at 2:04 PM, John McCall wrote:
>> If I understand this patch correctly, there's a crash arising from invalid redeclarations of functions.  If that's true, this is almost certainly a workaround rather than a fix, because it's just making the redeclaration valid.  Please fix the underlying crash rather than working around it like this.
> 
> Underlying crash is in IRgen because we do not catch such duplicate function definitions. This patch makes the re-declaration  identical to previous one and forces error in Sema.

Oh, I see, because they mangle the same way.

I think the appropriate place to make this change is in Sema::FunctionArgTypesAreEqual, in SemaOverload.cpp.  We already have similar logic for redeclaring functions with different protocol-qualified types.

>> Also, there are several problems with this pattern of recognizing 'struct objc_object' and 'struct objc_class'.
>> 1)  This is a significant change in behavior;  previously we intentionally never recognized these types as being their respective builtin types.  I'm actually ambivalent about that — we should probably recognize this, at the very least to give an error about it — but we shouldn't change the design lightly just to fix a crash.
> 
> I am open to issuing error on use of such types. But there could be many situations which previous gcc compiled code will break.

Yeah, that's certainly possible.

>> 2)  If we do want to recognize these structs as the builtin types, we should do consistently, not just for parameters, and not just for the exact case of pointers to the type.  The cleanest way to do that is in ConvertDeclSpecToType.
> 
> I thought about this at first. But we don't want to encourage use of these types and make them work silently. I rather let compiler issues error to discourage their use.

Right, but since this logic isn't constrained to just redeclaration checks, it's already going to do that.

>> 3)  The logic to recognize these structs should respect context;  only structs in global scope are the builtins, i.e. if getDeclContext()->getRedeclContext()->isTranslationUnit() (there might be a convenience function for that somewhere).
> 
> I agree on this point. I can tighten up the type checking.
> In any case, point of this patch was to fix the crash in a limited case and not treat 'struct objc_object' as a replacement for 'id' everywhere.
> So, these are alternatives.
> 
> 1. Do nothing and let the test case crash ( I think it was a manufactured test ).
> 2. Accept the alternative 'struct ...' as builtin type everywhere.
> 3. Issue error if it is used anywhere.
> 4.???

Personally I like #3 because ObjC code using 'struct objc_object' is very likely to either not compile or have really unfortunate behavioral changes — I think Argyrios recently hunted down something in the latter category — but that's the sort of problem we can only suss out with SWBs.  For the immediate problem, changing FunctionArgTypesAreEqual should be enough.

John.



More information about the cfe-commits mailing list