On Sep 8, 2010, at 2:04 PM, John McCall wrote: > On Sep 8, 2010, at 1:08 PM, Fariborz Jahanian wrote: >> Author: fjahanian >> Date: Wed Sep 8 15:08:18 2010 >> New Revision: 113397 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=113397&view=rev >> Log: >> Fix a crash when overloading id with objc_object*. >> Radar 8400356. >> >> >> Added: >> cfe/trunk/test/SemaObjC/legacy-objc-types.m >> cfe/trunk/test/SemaObjCXX/legacy-objc-types.mm >> Modified: >> cfe/trunk/include/clang/AST/Type.h >> cfe/trunk/lib/AST/Type.cpp >> cfe/trunk/lib/Sema/SemaType.cpp >> >> Modified: cfe/trunk/include/clang/AST/Type.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Type.h?rev=113397&r1=113396&r2=113397&view=diff >> = >> = >> = >> = >> = >> = >> = >> = >> = >> ===================================================================== >> --- cfe/trunk/include/clang/AST/Type.h (original) >> +++ cfe/trunk/include/clang/AST/Type.h Wed Sep 8 15:08:18 2010 >> @@ -932,7 +932,9 @@ >> bool isObjCQualifiedClassType() const; // Class<foo> >> bool isObjCObjectOrInterfaceType() const; >> bool isObjCIdType() const; // id >> + bool isLegacyObjCIdType(ASTContext &) const; // struct_object * >> bool isObjCClassType() const; // Class >> + bool isLegacyObjCClassType(ASTContext &) const; // struct_class * >> bool isObjCSelType() const; // Class >> bool isObjCBuiltinType() const; // 'id' or 'Class' >> bool isTemplateTypeParmType() const; // C++ template type >> parameter >> >> Modified: cfe/trunk/lib/AST/Type.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Type.cpp?rev=113397&r1=113396&r2=113397&view=diff >> = >> = >> = >> = >> = >> = >> = >> = >> = >> ===================================================================== >> --- cfe/trunk/lib/AST/Type.cpp (original) >> +++ cfe/trunk/lib/AST/Type.cpp Wed Sep 8 15:08:18 2010 >> @@ -470,6 +470,24 @@ >> return false; >> } >> >> +bool Type::isLegacyObjCIdType(ASTContext &Ctx) const { >> + if (const PointerType *PTTo = getAs<PointerType>()) { >> + QualType PointeeTy = PTTo->getPointeeType(); >> + if (const RecordType *RTy = PointeeTy->getAs<RecordType>()) >> + return RTy->getDecl()->getIdentifier() == >> &Ctx.Idents.get("objc_object"); >> + } >> + return false; >> +} >> + >> +bool Type::isLegacyObjCClassType(ASTContext &Ctx) const { >> + if (const PointerType *PTTo = getAs<PointerType>()) { >> + QualType PointeeTy = PTTo->getPointeeType(); >> + if (const RecordType *RTy = PointeeTy->getAs<RecordType>()) >> + return RTy->getDecl()->getIdentifier() == >> &Ctx.Idents.get("objc_class"); >> + } >> + return false; >> +} >> + >> bool Type::isEnumeralType() const { >> if (const TagType *TT = dyn_cast<TagType>(CanonicalType)) >> return TT->getDecl()->isEnum(); >> >> Modified: cfe/trunk/lib/Sema/SemaType.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaType.cpp?rev=113397&r1=113396&r2=113397&view=diff >> = >> = >> = >> = >> = >> = >> = >> = >> = >> ===================================================================== >> --- cfe/trunk/lib/Sema/SemaType.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaType.cpp Wed Sep 8 15:08:18 2010 >> @@ -1274,8 +1274,12 @@ >> if (BTy->getKind() == BuiltinType::Float) >> ArgTy = Context.DoubleTy; >> } >> + } else if (getLangOptions().ObjC1) { >> + if (ArgTy->isLegacyObjCIdType(Context)) >> + ArgTy = Context.getObjCIdType(); >> + else if (ArgTy->isLegacyObjCClassType(Context)) >> + ArgTy = Context.getObjCClassType(); >> } >> - >> ArgTys.push_back(ArgTy); >> } > > > 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. > > > 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. > > 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. > > 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.??? - fariborz > > > John. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
