[CodeGen][ObjC] Emit a primitive store to store a __strong field in ExpandTypeFromArgs

[ahatanak]

Summary

This fixes a bug in IRGen where a call to llvm.objc.storeStrong was being emitted to initialize a __strong field of an uninitialized temporary struct, which caused crashes at runtime.
rdar://problem/51807365

[ahatanak]

Note that passing isInit=true to EmitStoreThroughLValue to make it emit llvm.objc.retain would be incorrect since the callee destructs the struct argument.

[rjmccall]

clang/lib/CodeGen/CGCall.cpp

1056

This definitely deserves a comment.

I don't think the assertion is right; the condition is that the type is legal for a field in a struct that can be passed directly, and while that does exclude __weak (because the struct will have to be passed indirectly) and __autoreleasing (because that's not legal in a struct), it doesn't exclude __unsafe_unretained.

This function is implementing an operation that's broadly meaningful (it's a store-init of an owned value, in contrast to a store-init with an unowned value which is what isInit is implementing) but not extensively used in the C frontend. On some level, I feel like we should probably teach EmitStoreThroughLValue to handle that properly, but that's a more significant refactor.

It does look like this change isn't enough to handle __ptrauth, which will assume that the source value is signed appropriately for the unqualified type when probably it should be signed appropriately for the qualifier (which, like __weak, cannot be address-discriminated because it would stop being passed directly). Probably the default case should be to use EmitStoreOfScalar, and EmitStoreThroughLValue should only be used if the l-value is a bit-field (the only non-simple case that can actually happen from drilling down to a field).

The same logic applies on the load side in the abstract, except that it is only causing problems for __ptrauth (well, if we change the behavior above) because loads from the ARC qualifiers don't implicitly retain. Regardless, analogously to this, EmitRValueForField should only be calling EmitLoadOfLValue for bit-fields and should otherwise call EmitLoadOfScalar. Please add a comment on both sides making it clear that the logic is expected to be parallel.

[ahatanak]

Call EmitStoreThroughLValue and EmitLoadOfLValue only when the lvalue is a bitfield.

clang/lib/CodeGen/CGCall.cpp

1056

Ah right, the assertion isn't correct. My intention was to catch __weak pointers.

Let me know if the comment I added is OK.

[rjmccall]

Yes, that's great, thank you.

[ahatanak]

Fixed in d8136f14f125fb27f2326f397df0964bf62078ca

clang/lib/CodeGen/CGCall.cpp

for (auto FD : RExp->Fields) {
       LValue SubLV = EmitLValueForFieldInitialization(LV, FD);
       ExpandTypeFromArgs(FD->getType(), SubLV, AI);
     }
   } else if (isa<ComplexExpansion>(Exp.get())) {
     auto realValue = *AI++;
     auto imagValue = *AI++;
     EmitStoreOfComplex(ComplexPairTy(realValue, imagValue), LV, /*init*/ true);
   } else {
+    // Call EmitStoreOfScalar except when the lvalue is a bitfield to emit a
+    // primitive store.
     assert(isa<NoExpansion>(Exp.get()));
+    if (LV.isBitField())
       EmitStoreThroughLValue(RValue::get(*AI++), LV);
+    else
+      EmitStoreOfScalar(*AI++, LV);
   }
 }
 
 void CodeGenFunction::ExpandTypeToArgs(
     QualType Ty, CallArg Arg, llvm::FunctionType *IRFuncTy,
     SmallVectorImpl<llvm::Value *> &IRCallArgs, unsigned &IRCallArgPos) {
   auto Exp = getTypeExpansion(Ty, getContext());
   if (auto CAExp = dyn_cast<ConstantArrayExpansion>(Exp.get())) {

clang/lib/CodeGen/CGExpr.cpp

case TEK_Complex:
     return RValue::getComplex(EmitLoadOfComplex(FieldLV, Loc));
   case TEK_Aggregate:
     return FieldLV.asAggregateRValue(*this);
   case TEK_Scalar:
     // This routine is used to load fields one-by-one to perform a copy, so
     // don't load reference fields.
     if (FD->getType()->isReferenceType())
       return RValue::get(FieldLV.getPointer(*this));
+    // Call EmitLoadOfScalar except when the lvalue is a bitfield to emit a
+    // primitive load.
+    if (FieldLV.isBitField())
       return EmitLoadOfLValue(FieldLV, Loc);
+    return RValue::get(EmitLoadOfScalar(FieldLV, Loc));
   }
   llvm_unreachable("bad evaluation kind");
 }
 
 //===--------------------------------------------------------------------===//
 //                             Expression Emission
 //===--------------------------------------------------------------------===//
 

clang/test/CodeGenObjC/nontrivial-struct-param-init.m

+// RUN: %clang_cc1 -triple i386-apple-watchos6.0-simulator -emit-llvm -fblocks -fobjc-arc -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_S:.*]] = type { i8* }
+
+typedef struct {
+  id x;
+} S;
+
+// CHECK: define void @test0(i8* %[[A_0:.*]])
+// CHECK: %[[A:.*]] = alloca %[[STRUCT_S]], align 4
+// CHECK: %[[X:.*]] = getelementptr inbounds %[[STRUCT_S]], %[[STRUCT_S]]* %[[A]], i32 0, i32 0
+// CHECK: store i8* %[[A_0]], i8** %[[X]], align 4
+// CHECK: %[[V0:.*]] = bitcast %[[STRUCT_S]]* %[[A]] to i8**
+// CHECK: call void @__destructor_4_s0(i8** %[[V0]]) #2
+
+void test0(S a) {
+}