[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

1650 ↗

(On Diff #74760)

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

1652 ↗

(On Diff #74851)

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.

cfe/trunk/lib/CodeGen/CGDecl.cpp

if (accessedByInit && lifetime == Qualifiers::OCL_Strong) {
     EmitStoreOfScalar(value, lvalue, /* isInitialization */ true);
     EmitARCRelease(oldValue, ARCImpreciseLifetime);
     return;
   }
 
   EmitStoreOfScalar(value, lvalue, /* isInitialization */ true);
 }
 
-/// EmitScalarInit - Initialize the given lvalue with the given object.
-void CodeGenFunction::EmitScalarInit(llvm::Value *init, LValue lvalue) {
-  Qualifiers::ObjCLifetime lifetime = lvalue.getObjCLifetime();
-  if (!lifetime)
-    return EmitStoreThroughLValue(RValue::get(init), lvalue, true);
-
-  switch (lifetime) {
-  case Qualifiers::OCL_None:
-    llvm_unreachable("present but none");
-
-  case Qualifiers::OCL_ExplicitNone:
-    // nothing to do
-    break;
-
-  case Qualifiers::OCL_Strong:
-    init = EmitARCRetain(lvalue.getType(), init);
-    break;
-
-  case Qualifiers::OCL_Weak:
-    // Initialize and then skip the primitive store.
-    EmitARCInitWeak(lvalue.getAddress(), init);
-    return;
-
-  case Qualifiers::OCL_Autoreleasing:
-    init = EmitARCRetainAutorelease(lvalue.getType(), init);
-    break;
-  }
-
-  EmitStoreOfScalar(init, lvalue, /* isInitialization */ true);
-}
-
 /// canEmitInitWithFewStoresAfterMemset - Decide whether we can emit the
 /// non-zero parts of the specified initializer with equal or fewer than
 /// NumStores scalar stores.
 static bool canEmitInitWithFewStoresAfterMemset(llvm::Constant *Init,
                                                 unsigned &NumStores) {
   // Zero and Undef never requires any extra stores.
   if (isa<llvm::ConstantAggregateZero>(Init) ||
       isa<llvm::ConstantPointerNull>(Init) ||

cfe/trunk/lib/CodeGen/CGExpr.cpp

if (Qualifiers::ObjCLifetime Lifetime = Dst.getQuals().getObjCLifetime()) {
     case Qualifiers::OCL_None:
       llvm_unreachable("present but none");
 
     case Qualifiers::OCL_ExplicitNone:
       // nothing special
       break;
 
     case Qualifiers::OCL_Strong:
+      if (isInit) {
+        Src = RValue::get(EmitARCRetain(Dst.getType(), Src.getScalarVal()));
+        break;
+      }
       EmitARCStoreStrong(Dst, Src.getScalarVal(), /*ignore*/ true);
       return;
 
     case Qualifiers::OCL_Weak:
+      if (isInit)
+        // Initialize and then skip the primitive store.
+        EmitARCInitWeak(Dst.getAddress(), Src.getScalarVal());
+      else
         EmitARCStoreWeak(Dst.getAddress(), Src.getScalarVal(), /*ignore*/ true);
       return;
 
     case Qualifiers::OCL_Autoreleasing:
       Src = RValue::get(EmitObjCExtendObjectLifetime(Dst.getType(),
                                                      Src.getScalarVal()));
       // fall into the normal path
       break;
     }

cfe/trunk/lib/CodeGen/CGObjC.cpp

CurrentItem = Builder.CreateBitCast(CurrentItem, convertedElementType,
                                       "currentitem");
 
   // Make sure we have an l-value.  Yes, this gets evaluated every
   // time through the loop.
   if (!elementIsVariable) {
     elementLValue = EmitLValue(cast<Expr>(S.getElement()));
     EmitStoreThroughLValue(RValue::get(CurrentItem), elementLValue);
   } else {
-    EmitScalarInit(CurrentItem, elementLValue);
+    EmitStoreThroughLValue(RValue::get(CurrentItem), elementLValue,
+                           /*isInit*/ true);
   }
 
   // If we do have an element variable, this assignment is the end of
   // its initialization.
   if (elementIsVariable)
     EmitAutoVarCleanups(variable);
 
   // Perform the loop body, setting up break and continue labels.

cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp

static Address castValueFromUintptr(CodeGenFunction &CGF, QualType DstType,
 
   // If we are dealing with references we need to return the address of the
   // reference instead of the reference of the value.
   if (isReferenceType) {
     QualType RefType = Ctx.getLValueReferenceType(DstType);
     auto *RefVal = TmpAddr.getPointer();
     TmpAddr = CGF.CreateMemTemp(RefType, Twine(Name) + ".ref");
     auto TmpLVal = CGF.MakeAddrLValue(TmpAddr, RefType);
-    CGF.EmitScalarInit(RefVal, TmpLVal);
+    CGF.EmitStoreThroughLValue(RValue::get(RefVal), TmpLVal, /*isInit*/ true);
   }
 
   return TmpAddr;
 }
 
 llvm::Function *
 CodeGenFunction::GenerateOpenMPCapturedStmtFunction(const CapturedStmt &S) {
   assert(
void CodeGenFunction::EmitOMPForSimdDirective(const OMPForSimdDirective &S) {
   }
 }
 
 static LValue createSectionLVal(CodeGenFunction &CGF, QualType Ty,
                                 const Twine &Name,
                                 llvm::Value *Init = nullptr) {
   auto LVal = CGF.MakeAddrLValue(CGF.CreateMemTemp(Ty, Name), Ty);
   if (Init)
-    CGF.EmitScalarInit(Init, LVal);
+    CGF.EmitStoreThroughLValue(RValue::get(Init), LVal, /*isInit*/ true);
   return LVal;
 }
 
 void CodeGenFunction::EmitSections(const OMPExecutableDirective &S) {
   auto *Stmt = cast<CapturedStmt>(S.getAssociatedStmt())->getCapturedStmt();
   auto *CS = dyn_cast<CompoundStmt>(Stmt);
   bool HasLastprivates = false;
   auto &&CodeGen = [&S, Stmt, CS, &HasLastprivates](CodeGenFunction &CGF,

cfe/trunk/lib/CodeGen/CodeGenFunction.h

#endif
 
   /// EmitVarDecl - Emit a local variable declaration.
   ///
   /// This function can be called with a null (unreachable) insert point.
   void EmitVarDecl(const VarDecl &D);
 
   void EmitScalarInit(const Expr *init, const ValueDecl *D, LValue lvalue,
                       bool capturedByInit);
-  void EmitScalarInit(llvm::Value *init, LValue lvalue);
 
   typedef void SpecialInitFn(CodeGenFunction &Init, const VarDecl &D,
                              llvm::Value *Address);
 
   /// \brief Determine whether the given initializer is trivial in the sense
   /// that it requires no code to be generated.
   bool isTrivialInitializer(const Expr *Init);
 

cfe/trunk/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:   %[[V0:[0-9]+]] = call i8* @objc_retain(i8* bitcast (%struct.__NSConstantString_tag* @[[CFSTRING]]
+// CHECK:   %[[V1:[0-9]+]] = bitcast i8* %[[V0]] to %[[TYPE]]*
+// CHECK:   store %[[TYPE]]* %[[V1]], %[[TYPE]]** %[[ALLOCA]]
+// CHECK:   %[[V2:[0-9]+]] = bitcast %[[TYPE]]** %[[ALLOCA]]
+// CHECK:   call void @objc_storeStrong(i8** %[[V2]], i8* null)
+
+@class NSString;
+
+void test1() {
+  constexpr NSString *S = @"abc";
+}