[CodeGen][ObjC] Do not emit objc_storeStrong to initialize a constexpr variable

[ahatanak]

Summary

When compiling a constexpr NSString initialized with an objective-c string literal, CodeGen currently emits objc_storeStrong on an uninitialized alloca, which causes a crash:

constexpr NSString *S = @"abc";

%S = alloca %0*, align 8
%0 = bitcast %0** %S to i8** ; this points to uninitialized memory
call void @objc_storeStrong(i8** %0, i8* bitcast (%struct.__NSConstantString_tag* @_unnamed_cfstring_ to i8*)) #1

This patch fixes the crash by directly storing the pointer to the __NSConstantString_tag for the string into the allocated alloca. The rationale for this change is that retain or release on an Objective-c string literal (or anything used to initialize a constexpr variable) has no effect and therefore objc_storeStrong isn't needed in this case.

rdar://problem/28562009

[rjmccall]

The correct fix is to honor isInit by folding the logic for EmitScalarInit into this function. That should allow you to eliminate EmitScalarInit completely, although it would be fine to leave it as just a call to EmitStoreThroughLValue. I did a quick audit of all the calls to EmitStoreThroughLValue that might pass isInit=true, and it does look like none of them are relying on the current behavior for ARC ownership; the most suspicious are the calls from EmitObjCCollectionLiteral, but the l-value there is non lifetime-qualified, so it's fine.

[ahatanak]

There are two overloaded functions of CodeGenFunction::EmitScalarInit. Are you suggesting we fold both of them into EmitStoreThroughLValue and remove them?

[rjmccall]

Sorry, no, just the one that takes an llvm::Value* instead of an Expr*.

[ahatanak]

Fold EmitScalarInit into EmitStoreThroughLValue and remove EmitScalarInit.

I don't think ElementType in EmitObjCCollectionLiteral has a lifetime qualifier, so it should be safe to call EmitStoreThroughLValue instead of EmitScalarInit in that function.

[rjmccall]

lib/CodeGen/CGExpr.cpp

1649

I think you can fold this a bit more. :) You have exactly the same switch statement below, and several of the cases are identical; for the others, you can just sink the isInit check into the case.

Note that calling EmitStoreOfScalar and returning has the same behavior as "falling into the normal path". isObjCWeak() / isObjCStrong() are checking for the GC qualifiers, which are exclusive with ARC lifetime.

[ahatanak]

Simplify the code a bit more.

[rjmccall]

Thanks! A couple minor tweaks, then LGTM.

lib/CodeGen/CGExpr.cpp

1643

These two cases are actually identical in behavior (because __autoreleased variables don't actually own their current value). I think I prefer the second way of writing it.

lib/CodeGen/CGObjC.cpp

1665 ↗

(On Diff #74851)

Please add /*isInit*/ comments here and elsewhere.

[ahatanak]

Address review comments. Simplify and add comments.

[rjmccall]

Thanks, that looks great! LGTM.

lib/CodeGen/CGDecl.cpp

if (!constant) {
     lv.setNonGC(true);
     return EmitExprAsInit(Init, &D, lv, capturedByInit);
   }
 
   if (!emission.IsConstantAggregate) {
     // For simple scalar/complex initialization, store the value directly.
     LValue lv = MakeAddrLValue(Loc, type);
     lv.setNonGC(true);
-    return EmitStoreThroughLValue(RValue::get(constant), lv, true);
+    return EmitStoreThroughLValue(RValue::get(constant), lv, true, true);
   }
 
   // If this is a simple aggregate initialization, we can optimize it
   // in various ways.
   bool isVolatile = type.isVolatileQualified();
 
   llvm::Value *SizeVal =
     llvm::ConstantInt::get(IntPtrTy,

lib/CodeGen/CGExpr.cpp

RValue CodeGenFunction::EmitLoadOfGlobalRegLValue(LValue LV) {
   return RValue::get(Call);
 }
 
 
 /// EmitStoreThroughLValue - Store the specified rvalue into the specified
 /// lvalue, where both are guaranteed to the have the same type, and that type
 /// is 'Ty'.
 void CodeGenFunction::EmitStoreThroughLValue(RValue Src, LValue Dst,
-                                             bool isInit) {
+                                             bool isInit, bool IsConstExpr) {
   if (!Dst.isSimple()) {
     if (Dst.isVectorElt()) {
       // Read/modify/write the vector, inserting the new element.
       llvm::Value *Vec = Builder.CreateLoad(Dst.getVectorAddress(),
                                             Dst.isVolatileQualified());
       Vec = Builder.CreateInsertElement(Vec, Src.getScalarVal(),
                                         Dst.getVectorIdx(), "vecins");
       Builder.CreateStore(Vec, Dst.getVectorAddress(),
if (!Dst.isSimple()) {
     if (Dst.isGlobalReg())
       return EmitStoreThroughGlobalRegLValue(Src, Dst);
 
     assert(Dst.isBitField() && "Unknown LValue type");
     return EmitStoreThroughBitfieldLValue(Src, Dst);
   }
 
   // There's special magic for assigning into an ARC-qualified l-value.
+  if (!IsConstExpr) {
     if (Qualifiers::ObjCLifetime Lifetime = Dst.getQuals().getObjCLifetime()) {
       switch (Lifetime) {
       case Qualifiers::OCL_None:
         llvm_unreachable("present but none");
 
       case Qualifiers::OCL_ExplicitNone:
         // nothing special
         break;
 
       case Qualifiers::OCL_Strong:
         EmitARCStoreStrong(Dst, Src.getScalarVal(), /*ignore*/ true);
         return;
 
       case Qualifiers::OCL_Weak:
         EmitARCStoreWeak(Dst.getAddress(), Src.getScalarVal(), /*ignore*/ true);
         return;
 
       case Qualifiers::OCL_Autoreleasing:
-      Src = RValue::get(EmitObjCExtendObjectLifetime(Dst.getType(),
-                                                     Src.getScalarVal()));
+        Src = RValue::get(
+            EmitObjCExtendObjectLifetime(Dst.getType(), Src.getScalarVal()));
         // fall into the normal path
         break;
       }
     }
 
     if (Dst.isObjCWeak() && !Dst.isNonGC()) {
       // load of a __weak object.
       Address LvalueDst = Dst.getAddress();
       llvm::Value *src = Src.getScalarVal();
       CGM.getObjCRuntime().EmitObjCWeakAssign(*this, src, LvalueDst);
       return;
     }
 
     if (Dst.isObjCStrong() && !Dst.isNonGC()) {
       // load of a __strong object.
       Address LvalueDst = Dst.getAddress();
       llvm::Value *src = Src.getScalarVal();
       if (Dst.isObjCIvar()) {
         assert(Dst.getBaseIvarExp() && "BaseIvarExp is NULL");
         llvm::Type *ResultType = IntPtrTy;
         Address dst = EmitPointerWithAlignment(Dst.getBaseIvarExp());
         llvm::Value *RHS = dst.getPointer();
         RHS = Builder.CreatePtrToInt(RHS, ResultType, "sub.ptr.rhs.cast");
         llvm::Value *LHS =
             Builder.CreatePtrToInt(LvalueDst.getPointer(), ResultType,
                                    "sub.ptr.lhs.cast");
         llvm::Value *BytesBetween = Builder.CreateSub(LHS, RHS, "ivar.offset");
         CGM.getObjCRuntime().EmitObjCIvarAssign(*this, src, dst,
                                                 BytesBetween);
       } else if (Dst.isGlobalObjCRef()) {
         CGM.getObjCRuntime().EmitObjCGlobalAssign(*this, src, LvalueDst,
                                                   Dst.isThreadLocalRef());
-    }
-    else
+      } else
         CGM.getObjCRuntime().EmitObjCStrongCastAssign(*this, src, LvalueDst);
       return;
     }
+  }
 
   assert(Src.isScalar() && "Can't emit an agg store with this method");
   EmitStoreOfScalar(Src.getScalarVal(), Dst, isInit);
 }
 
 void CodeGenFunction::EmitStoreThroughBitfieldLValue(RValue Src, LValue Dst,
                                                      llvm::Value **Result) {
   const CGBitFieldInfo &Info = Dst.getBitFieldInfo();

lib/CodeGen/CodeGenFunction.h

public:
   RValue EmitLoadOfLValue(LValue V, SourceLocation Loc);
   RValue EmitLoadOfExtVectorElementLValue(LValue V);
   RValue EmitLoadOfBitfieldLValue(LValue LV);
   RValue EmitLoadOfGlobalRegLValue(LValue LV);
 
   /// EmitStoreThroughLValue - Store the specified rvalue into the specified
   /// lvalue, where both are guaranteed to the have the same type, and that type
   /// is 'Ty'.
-  void EmitStoreThroughLValue(RValue Src, LValue Dst, bool isInit = false);
+  void EmitStoreThroughLValue(RValue Src, LValue Dst, bool isInit = false,
+                              bool IsConstExpr = false);
   void EmitStoreThroughExtVectorComponentLValue(RValue Src, LValue Dst);
   void EmitStoreThroughGlobalRegLValue(RValue Src, LValue Dst);
 
   /// EmitStoreThroughBitfieldLValue - Store Src into Dst with same constraints
   /// as EmitStoreThroughLValue.
   ///
   /// \param Result [out] - If non-null, this will be set to a Value* for the
   /// bit-field contents after the store, appropriate for use as the result of

test/CodeGenObjCXX/arc-constexpr.mm

+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -fobjc-arc -o - -std=c++11 %s | FileCheck %s
+
+// CHECK: %[[TYPE:[a-z0-9]+]] = type opaque
+// CHECK: @[[CFSTRING:[a-z0-9_]+]] = private global %struct.__NSConstantString_tag
+
+// CHECK: define void @_Z5test1v
+// CHECK:   %[[ALLOCA:[A-Z]+]] = alloca %[[TYPE]]*
+// CHECK:   store %[[TYPE]]* bitcast (%struct.__NSConstantString_tag* @[[CFSTRING]] to %[[TYPE]]*), %[[TYPE]]** %[[ALLOCA]]
+// CHECK:   %[[V0:[a-z0-9]+]] = bitcast %[[TYPE]]** %[[ALLOCA]] to i8**
+// CHECK:   call void @objc_storeStrong(i8** %[[V0]], i8* null)
+
+@class NSString;
+
+void test1() {
+  constexpr NSString *S = @"abc";
+}