[CodeGen][ObjC] zero-ext an i1 value to i8

[ahatanak]

Summary

This patch fixes an assert that fires when there is a property that has attribute nonatomic and type _Atomic(_Bool). The assert fires when an i1 value is bitcast to i8 (which is the type ConvertType(propType) returns).

I have a couple of questions as I wasn't sure this was the right way to fix the assert.

1. Is it legal to have attribute nonatomic on a property that has an atomic type? Should clang error out?
2. Should the return type of the getter method be i1 or i8? I chose not to change the return type (which was i8), but I think there are cases where changing it to i1 might make more sense. For example, the following code currently doesn't compile because foo1 and the getter for "p" (which is the property of A in the test case) have different return types.

_Bool foo1() {
  A *a = [A new];
  return i.p;
}

rdar://problem/26322972

[ahatanak]

Also, it seems that there are bugs in the way clang handles functions whose return types are atomic. Clang asserts when compiling the following test case:

$ cat t1.c

_Atomic _Bool b1;

_Atomic _Bool foo1() {
  return b1;
}

$ clang -std=c11 -o - -S t1.c -emit-llvm

[rjmccall]

_Atomic is functionally a type qualifier and should be removed in Sema when computing the result type of the getter and the parameter type of the setter. That is, if the user declares a property of type _Atomic(_Bool), we should pretend that the property has type _Bool when creating the getter and setter.

[ahatanak]

In D20407#433915, @rjmccall wrote:

_Atomic is functionally a type qualifier and should be removed in Sema when computing the result type of the getter and the parameter type of the setter. That is, if the user declares a property of type _Atomic(_Bool), we should pretend that the property has type _Bool when creating the getter and setter.

That sounds like a more principled way to fix the problem. We'll have to remove _Atomic from return types of normal functions too, not just objective-c getters and setters, because otherwise programs like this won't compile:

_Atomic(_Bool) foo1() {
  A *a = [A new];
  return a.p;
}

[ahatanak]

I've managed to remove _Atomic from the types of the return and parameter of getter and setter functions in my new patch.

I'm not sure how I should handle typedefs though. If I had the following typedef,

typedef _Atomic(_Bool) AtomicBool,

would it be OK to desugar the typedef and then remove _Atomic? I'm not sure this is always correct because dusugaring typedefs will remove the attributes attached to them that might be important too.

[rjmccall]

Top-level attributes shouldn't really be affecting the ABI or code-generation in any way, so desugaring should be fine.

[ahatanak]

Rewrote the patch based on John's review comment.

Remove typedefs and _Atomic from the return and parameter types of getters and setters of objective-c properties.

[rjmccall]

include/clang/AST/Type.h

1084 ↗

(On Diff #58144)

Please name this getAtomicUnqualifiedType() and have it promise to remove all qualifiers including _Atomic. It should not promise to remove type sugar.

lib/AST/Type.cpp

1282 ↗

(On Diff #58144)

This should just be:

if (auto AT = getAs<AtomicType>()) {
  return AT->getValueType().getUnqualifiedType();
} else {
  return getUnqualifiedType();
}

lib/CodeGen/CGObjC.cpp

901 ↗

(On Diff #58144)

The code around here uses lowercase identifiers, please be consistent.

1023 ↗

(On Diff #58144)

I don't understand the purpose of doing two bitcasts here, so let's just drop this first one.

lib/Sema/SemaDeclObjC.cpp

4319 ↗

(On Diff #58144)

Hmm. I'm not sure you should be doing this here; or at least, if we're going to do it, we should probably be doing it consistently in CheckFunctionReturnType.

4361 ↗

(On Diff #58144)

Same thing. If the user wants to explicitly declare a parameter _Atomic, they can do so.

lib/Sema/SemaObjCProperty.cpp

1497 ↗

(On Diff #58144)

This is incorrect. The property ivar type should definitely remain _Atomic if the property was declared that way.

2210 ↗

(On Diff #58144)

Please pull this above the comment and give it its own comment, something like:

// The getter returns the declared property type with all qualifiers removed.

2282 ↗

(On Diff #58144)

Same thing: please pull this above and give it its own comment.

[ahatanak]

Address John's review comments.

[ahatanak]

lib/AST/Type.cpp

1282 ↗

(On Diff #58495)

I added getTypePtr() because the code didn't compile.

lib/CodeGen/CGObjC.cpp

901 ↗

(On Diff #58495)

I've only changed ivarSize. I can change the variable names defined below (RetTy and RetTySize) too if that is necesasry.

[ahatanak]

I reverted the changes I made in SemaDeclObjC.cpp as they weren't needed to pass the regression tests I added. clang still asserts when it compiles an objective-c method returning _Atomic and those changes will become necessary when I fix that later. Normal functions returning _Atomic doesn't compile either.

[rjmccall]

lib/AST/Type.cpp

1282 ↗

(On Diff #58495)

Sure.

lib/CodeGen/CGObjC.cpp

901 ↗

(On Diff #58495)

Please do.

lib/Sema/SemaObjCProperty.cpp

1497 ↗

(On Diff #58495)

You know what, I'm sorry, I didn't look closely enough at the context. Please just rename PropertyIvarType to something like PropertyRValueType.

[rjmccall]

In D20407#439887, @ahatanak wrote:

I reverted the changes I made in SemaDeclObjC.cpp as they weren't needed to pass the regression tests I added. clang still asserts when it compiles an objective-c method returning _Atomic and those changes will become necessary when I fix that later. Normal functions returning _Atomic doesn't compile either.

The C standard is poorly-written in this area, but I think it would be reasonable for CheckFunctionReturnType to just silently remove _Atomic. (You will not be able to just re-use your new method there; removing other qualifiers is not acceptable, I'm afraid.)

[ahatanak]

Rename variables.

[ahatanak]

In D20407#439951, @rjmccall wrote:

The C standard is poorly-written in this area, but I think it would be reasonable for CheckFunctionReturnType to just silently remove _Atomic. (You will not be able to just re-use your new method there; removing other qualifiers is not acceptable, I'm afraid.)

Do you mean I should fix those cases in this patch too?

[rjmccall]

In D20407#440050, @ahatanak wrote:

In D20407#439951, @rjmccall wrote:

The C standard is poorly-written in this area, but I think it would be reasonable for CheckFunctionReturnType to just silently remove _Atomic. (You will not be able to just re-use your new method there; removing other qualifiers is not acceptable, I'm afraid.)

Do you mean I should fix those cases in this patch too?

No, sorry, just pointing something out for your other patch.

[rjmccall]

One minor improvement to the API and LGTM.

include/clang/AST/Type.h

1084 ↗

(On Diff #58512)

This doesn't need an ASTContext.

[ahatanak]

Thanks for the review. I've removed parameter ASTContext that was unused.

cfe/trunk/include/clang/AST/Type.h

public:
   /// parameters with their corresponding arguments.
   QualType substObjCMemberType(QualType objectType,
                                const DeclContext *dc,
                                ObjCSubstitutionContext context) const;
 
   /// Strip Objective-C "__kindof" types from the given type.
   QualType stripObjCKindOfType(const ASTContext &ctx) const;
 
+  /// Remove all qualifiers including _Atomic.
+  QualType getAtomicUnqualifiedType() const;
+
 private:
   // These methods are implemented in a separate translation unit;
   // "static"-ize them to avoid creating temporary QualTypes in the
   // caller.
   static bool isConstant(QualType T, const ASTContext& Ctx);
   static QualType getDesugaredType(QualType T, const ASTContext &Context);
   static SplitQualType getSplitDesugaredType(QualType T);
   static SplitQualType getSplitUnqualifiedTypeImpl(QualType type);

cfe/trunk/lib/AST/Type.cpp

return simpleTransform(ctx, *this,
                                               /*isKindOf=*/false),
                         splitType.Quals);
              }
 
              return type;
            });
 }
 
+QualType QualType::getAtomicUnqualifiedType() const {
+  if (auto AT = getTypePtr()->getAs<AtomicType>())
+    return AT->getValueType().getUnqualifiedType();
+  return getUnqualifiedType();
+}
+
 Optional<ArrayRef<QualType>> Type::getObjCSubstitutions(
                                const DeclContext *dc) const {
   // Look through method scopes.
   if (auto method = dyn_cast<ObjCMethodDecl>(dc))
     dc = method->getDeclContext();
 
   // Find the class or category in which the type we're substituting
   // was declared.

cfe/trunk/lib/CodeGen/CGObjC.cpp

case PropertyImplStrategy::Native: {
     // We don't need to do anything for a zero-size struct.
     if (strategy.getIvarSize().isZero())
       return;
 
     LValue LV = EmitLValueForIvar(TypeOfSelfObject(), LoadObjCSelf(), ivar, 0);
 
     // Currently, all atomic accesses have to be through integer
     // types, so there's no point in trying to pick a prettier type.
-    llvm::Type *bitcastType =
-      llvm::Type::getIntNTy(getLLVMContext(),
-                            getContext().toBits(strategy.getIvarSize()));
+    uint64_t ivarSize = getContext().toBits(strategy.getIvarSize());
+    llvm::Type *bitcastType = llvm::Type::getIntNTy(getLLVMContext(), ivarSize);
     bitcastType = bitcastType->getPointerTo(); // addrspace 0 okay
 
     // Perform an atomic load.  This does not impose ordering constraints.
     Address ivarAddr = LV.getAddress();
     ivarAddr = Builder.CreateBitCast(ivarAddr, bitcastType);
     llvm::LoadInst *load = Builder.CreateLoad(ivarAddr, "load");
     load->setAtomic(llvm::AtomicOrdering::Unordered);
 
     // Store that value into the return address.  Doing this with a
     // bitcast is likely to produce some pretty ugly IR, but it's not
     // the *most* terrible thing in the world.
-    Builder.CreateStore(load, Builder.CreateBitCast(ReturnValue, bitcastType));
+    llvm::Type *retTy = ConvertType(getterMethod->getReturnType());
+    uint64_t retTySize = CGM.getDataLayout().getTypeSizeInBits(retTy);
+    llvm::Value *ivarVal = load;
+    if (ivarSize > retTySize) {
+      llvm::Type *newTy = llvm::Type::getIntNTy(getLLVMContext(), retTySize);
+      ivarVal = Builder.CreateTrunc(load, newTy);
+      bitcastType = newTy->getPointerTo();
+    }
+    Builder.CreateStore(ivarVal,
+                        Builder.CreateBitCast(ReturnValue, bitcastType));
 
     // Make sure we don't do an autorelease.
     AutoreleaseResult = false;
     return;
   }
 
   case PropertyImplStrategy::GetSetProperty: {
     llvm::Value *getPropertyFn =
case TEK_Scalar: {
 
         // Otherwise we want to do a simple load, suppressing the
         // final autorelease.
         } else {
           value = EmitLoadOfLValue(LV, SourceLocation()).getScalarVal();
           AutoreleaseResult = false;
         }
 
-        value = Builder.CreateBitCast(value, ConvertType(propType));
         value = Builder.CreateBitCast(
             value, ConvertType(GetterMethodDecl->getReturnType()));
       }
       
       EmitReturnOfRValue(RValue::get(value), propType);
       return;
     }
     }

cfe/trunk/lib/Sema/SemaObjCProperty.cpp

 }
 
 bool Sema::DiagnosePropertyAccessorMismatch(ObjCPropertyDecl *property,
                                             ObjCMethodDecl *GetterMethod,
                                             SourceLocation Loc) {
   if (!GetterMethod)
     return false;
   QualType GetterType = GetterMethod->getReturnType().getNonReferenceType();
-  QualType PropertyIvarType = property->getType().getNonReferenceType();
-  bool compat = Context.hasSameType(PropertyIvarType, GetterType);
+  QualType PropertyRValueType =
+      property->getType().getNonReferenceType().getAtomicUnqualifiedType();
+  bool compat = Context.hasSameType(PropertyRValueType, GetterType);
   if (!compat) {
     const ObjCObjectPointerType *propertyObjCPtr = nullptr;
     const ObjCObjectPointerType *getterObjCPtr = nullptr;
-    if ((propertyObjCPtr = PropertyIvarType->getAs<ObjCObjectPointerType>()) && 
+    if ((propertyObjCPtr =
+             PropertyRValueType->getAs<ObjCObjectPointerType>()) &&
         (getterObjCPtr = GetterType->getAs<ObjCObjectPointerType>()))
       compat = Context.canAssignObjCInterfaces(getterObjCPtr, propertyObjCPtr);
-    else if (CheckAssignmentConstraints(Loc, GetterType, PropertyIvarType) 
+    else if (CheckAssignmentConstraints(Loc, GetterType, PropertyRValueType)
               != Compatible) {
           Diag(Loc, diag::error_property_accessor_type)
-            << property->getDeclName() << PropertyIvarType
+            << property->getDeclName() << PropertyRValueType
             << GetterMethod->getSelector() << GetterType;
           Diag(GetterMethod->getLocation(), diag::note_declared_at);
           return true;
     } else {
       compat = true;
-      QualType lhsType =Context.getCanonicalType(PropertyIvarType).getUnqualifiedType();
+      QualType lhsType = Context.getCanonicalType(PropertyRValueType);
       QualType rhsType =Context.getCanonicalType(GetterType).getUnqualifiedType();
       if (lhsType != rhsType && lhsType->isArithmeticType())
         compat = false;
     }
   }
   
   if (!compat) {
     Diag(Loc, diag::warn_accessor_property_type_mismatch)
void Sema::ProcessPropertyDecl(ObjCPropertyDecl *property) {
   // (which is odd, but allowed). Sema should be typechecking that the
   // declarations jive in that situation (which it is not currently).
   if (!GetterMethod) {
     // No instance/class method of same name as property getter name was found.
     // Declare a getter method and add it to the list of methods
     // for this class.
     SourceLocation Loc = property->getLocation();
 
+    // The getter returns the declared property type with all qualifiers
+    // removed.
+    QualType resultTy = property->getType().getAtomicUnqualifiedType();
+
     // If the property is null_resettable, the getter returns nonnull.
-    QualType resultTy = property->getType();
     if (property->getPropertyAttributes() &
         ObjCPropertyDecl::OBJC_PR_null_resettable) {
       QualType modifiedTy = resultTy;
       if (auto nullability = AttributedType::stripOuterNullability(modifiedTy)) {
         if (*nullability == NullabilityKind::Unspecified)
           resultTy = Context.getAttributedType(AttributedType::attr_nonnull,
                                                modifiedTy, modifiedTy);
       }
if (!SetterMethod) {
                                /*isPropertyAccessor=*/true,
                                /*isImplicitlyDeclared=*/true,
                                /*isDefined=*/false,
                                (property->getPropertyImplementation() ==
                                 ObjCPropertyDecl::Optional) ?
                                 ObjCMethodDecl::Optional :
                                 ObjCMethodDecl::Required);
 
+      // Remove all qualifiers from the setter's parameter type.
+      QualType paramTy =
+          property->getType().getUnqualifiedType().getAtomicUnqualifiedType();
+
       // If the property is null_resettable, the setter accepts a
       // nullable value.
-      QualType paramTy = property->getType().getUnqualifiedType();
       if (property->getPropertyAttributes() &
           ObjCPropertyDecl::OBJC_PR_null_resettable) {
         QualType modifiedTy = paramTy;
         if (auto nullability = AttributedType::stripOuterNullability(modifiedTy)){
           if (*nullability == NullabilityKind::Unspecified)
             paramTy = Context.getAttributedType(AttributedType::attr_nullable,
                                                 modifiedTy, modifiedTy);
         }

cfe/trunk/test/CodeGenObjC/property-atomic-bool.m

+// RUN: %clang_cc1 -triple x86_64-apple-macosx10 -emit-llvm -x objective-c %s -o - | FileCheck %s
+
+// CHECK: define internal zeroext i1 @"\01-[A0 p]"(
+// CHECK:   %[[ATOMIC_LOAD:.*]] = load atomic i8, i8* %{{.*}} seq_cst
+// CHECK:   %[[TOBOOL:.*]] = trunc i8 %[[ATOMIC_LOAD]] to i1
+// CHECK:   ret i1 %[[TOBOOL]]
+
+// CHECK: define internal void @"\01-[A0 setP:]"({{.*}} i1 zeroext {{.*}})
+// CHECK:   store atomic i8 %{{.*}}, i8* %{{.*}} seq_cst
+// CHECK:   ret void
+
+// CHECK: define internal zeroext i1 @"\01-[A1 p]"(
+// CHECK:   %[[ATOMIC_LOAD:.*]] = load atomic i8, i8* %{{.*}} unordered
+// CHECK:   %[[TOBOOL:.*]] = trunc i8 %load to i1
+// CHECK:   ret i1 %[[TOBOOL]]
+
+// CHECK: define internal void @"\01-[A1 setP:]"({{.*}} i1 zeroext %p)
+// CHECK:   store atomic i8 %{{.*}}, i8* %{{.*}} unordered
+// CHECK:   ret void
+
+@interface A0
+@property(nonatomic) _Atomic(_Bool) p;
+@end
+@implementation A0
+@end
+
+@interface A1 {
+  _Atomic(_Bool) p;
+}
+@property _Atomic(_Bool) p;
+@end
+@implementation A1
+@synthesize p;
+@end

cfe/trunk/test/SemaObjC/property-atomic-bool.m

+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.10 -ast-dump "%s" 2>&1 | FileCheck %s
+
+// CHECK: TypedefDecl {{.*}} referenced AtomicBool '_Atomic(_Bool)'
+// CHECK:  AtomicType {{.*}} '_Atomic(_Bool)'
+// CHECK:   BuiltinType {{.*}} '_Bool'
+// CHECK: ObjCInterfaceDecl {{.*}} A0
+// CHECK:  ObjCPropertyDecl {{.*}} p '_Atomic(_Bool)' {{.*}} nonatomic
+// CHECK:  ObjCMethodDecl {{.*}} implicit - p '_Bool'
+// CHECK:  ObjCMethodDecl {{.*}} implicit - setP: 'void'
+// CHECK:   ParmVarDecl {{.*}} p '_Bool'
+// CHECK: ObjCInterfaceDecl {{.*}} A1
+// CHECK:  ObjCPropertyDecl {{.*}} p 'AtomicBool':'_Atomic(_Bool)' {{.*}} nonatomic
+// CHECK:  ObjCMethodDecl {{.*}} implicit - p '_Bool'
+// CHECK:  ObjCMethodDecl {{.*}} implicit - setP: 'void'
+// CHECK:   ParmVarDecl {{.*}} p '_Bool'
+// CHECK: ObjCInterfaceDecl {{.*}} A2
+// CHECK:  ObjCIvarDecl {{.*}} p '_Atomic(_Bool)' protected
+// CHECK:  ObjCPropertyDecl {{.*}} p '_Atomic(_Bool)'
+// CHECK:  ObjCMethodDecl {{.*}} implicit - p '_Bool'
+// CHECK:  ObjCMethodDecl {{.*}} implicit - setP: 'void'
+// CHECK:   ParmVarDecl {{.*}} p '_Bool'
+// CHECK: ObjCInterfaceDecl {{.*}} A3
+// CHECK:  ObjCIvarDecl {{.*}} p 'AtomicBool':'_Atomic(_Bool)' protected
+// CHECK:  ObjCPropertyDecl {{.*}} p 'AtomicBool':'_Atomic(_Bool)'
+// CHECK:  ObjCMethodDecl {{.*}} implicit - p '_Bool'
+// CHECK:  ObjCMethodDecl {{.*}} implicit - setP: 'void'
+// CHECK:   ParmVarDecl {{.*}} p '_Bool'
+
+typedef _Atomic(_Bool) AtomicBool;
+
+@interface A0
+@property(nonatomic) _Atomic(_Bool) p;
+@end
+@implementation A0
+@end
+
+@interface A1
+@property(nonatomic) AtomicBool p;
+@end
+@implementation A1
+@end
+
+@interface A2 {
+  _Atomic(_Bool) p;
+}
+@property _Atomic(_Bool) p;
+@end
+
+@implementation A2
+@synthesize p;
+@end
+
+@interface A3 {
+  AtomicBool p;
+}
+@property AtomicBool p;
+@end
+
+@implementation A3
+@synthesize p;
+@end