[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

[Fariborz Jahanian]

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.