Fix IRGen for referencing a static local before emitting its decl

[rnk]

Summary

Previously CodeGen assumed that static locals were emitted before they
could be accessed, which is true for automatic storage duration locals.
However, it is possible to have CodeGen emit a nested function that uses
a static local before emitting the function that defines the static
local, breaking that assumption.

Fix it by splitting static local creation up into a getAddrOf phase and
a definition phase, as is done for other globals.

Fixes PR18020. See also previous attempts to fix static locals in
PR6769 and PR7101.

[rsmith]

Hmm, I think you need to emit a definition, not just a declaration, when you see the first use of a static local variable. Here's a valid C++1y program that I think the current patch would reject (with a link error):

static auto f() {
  static int n;
  struct S { int &operator()() { return n; } };
  return S();
}

int main() { return decltype(f())()(); }

[rnk]

• Use getOrCreate* instead of GetAddr* and Create*

[rnk]

In D4787#4, @rsmith wrote:

Hmm, I think you need to emit a definition, not just a declaration, when you see the first use of a static local variable. Here's a valid C++1y program that I think the current patch would reject (with a link error):

static auto f() {
  static int n;
  struct S { int &operator()() { return n; } };
  return S();
}

int main() { return decltype(f())()(); }

Hopefully the new patch addresses this.

[rsmith]

lib/CodeGen/CGDecl.cpp

174 ↗

(On Diff #13291)

Does it still make sense for this to be on CodeGenFunction since it can now be called while emitting the "wrong" function? It seems to risk using local state of the CodeGenFunction object, which would be bad.

191 ↗

(On Diff #13291)

Case in point: this will presumably produce the wrong mangled name if the static local's emission is triggered by a function other than the containing one.

199 ↗

(On Diff #13291)

This doesn't look right. If the static local has a constant initializer, you need to emit it here, not as part of emitting the surrounding function. Given:

static auto f() {
  static int n = 1;
  struct S { int &operator()() { return n; } };
  return S();
}

int main() { return decltype(f())()(); }

... main should return 1, and I think with this patch it returns 0.

[rnk]

This is turning out to be really, really difficult.

lib/CodeGen/CGDecl.cpp

174 ↗

(On Diff #13291)

It seems there's two uses of local state:

• CGF affects name mangling for static locals in non-C++ languages, but in those languages, the name is presumably not significant
• CGF matters to CGExprConstant, in particular for taking the address of a label

191 ↗

(On Diff #13291)

No, in C++ mode it just mangles the VarDecl, which has all the info needed.

199 ↗

(On Diff #13291)

OK. This can be done, but it has considerable complexity because the initializer can reference itself circularly, apparently.

[rnk]

• Use getOrCreate* instead of GetAddr* and Create*
• Emit constant initializers of local statics

[rnk]

ping

[rsmith]

lib/CodeGen/CGDecl.cpp

170 ↗

(On Diff #13732)

Would be useful for this function name to mention something about static local variables.

307–309 ↗

(On Diff #13732)

Do we ever need to re-emit the initializer for correctness? The only cases I can think of are like:

extern const int n;
auto f() {
  static const int k = n;
  return [] { return k; }
}
auto x = f();
const int n = 3;

... where we can choose to use either static or dynamic initialization for f()::k.

lib/CodeGen/CGExprConstant.cpp

921–922 ↗

(On Diff #13732)

Oh, I see. This is why EmitInitializerForStaticVarDecl might need to re-emit the initializer. It seems a little scary for this to silently return 0 in other cases, but I suppose it's not too bad. A comment here would be useful (and also one in EmitInitializerForStaticVarDecl explaining why we re-emit).

I'm not convinced this is correct, though. Consider an inline function that has a static local with an initializer that uses the address of a label, and exposes a local lambda that uses the static local. Even if that function is not used in *this* TU, we must still make the label addresses be correct, in case the function is used in a different TU. For instance:

inline auto foo(void *p) {
  if (p) goto *p;
  static void *q = &&label;
  while (1) {
    struct S { static void *get() { return q; } };
    return S();
  label:
    puts("hello world");
  }
}

TU1:

void *global = decltype(foo(0))::get();

TU2:

int main() { extern void *global; foo(global); }

You might say that you don't care about this case. I'd be inclined to agree =) Perhaps we should ErrorUnsupported on an AddrLabelExpr with no CGF?

[rnk]

• Back up and do this the easy way

[rsmith]

LGTM, subject to handling of ObjCMethodDecl.

lib/CodeGen/CGDecl.cpp

231 ↗

(On Diff #14535)

Should we handle ObjCMethodDecls here?

[rnk]

Thanks!

lib/CodeGen/CGDecl.cpp

231 ↗

(On Diff #14535)

I thought about this a lot and I don't think this bug is reachable with an objective C method decl. They always appear at global scope, and you can't actually refer to one without constructing an object of the ObjC class type and calling the method indirectly via string lookup. If you've done that, that implies we've emitted the code.

That overflowed my objective C thinking budget, so I avoided crashing on them and moved on. I'll try to comment on that.

[rnk]

Closed by commit rL219265 (authored by @rnk).

cfe/trunk/lib/CodeGen/CGDecl.cpp

void CodeGenFunction::EmitVarDecl(const VarDecl &D) {
 
   if (D.getStorageClass() == SC_OpenCLWorkGroupLocal)
     return CGM.getOpenCLRuntime().EmitWorkGroupLocalVarDecl(*this, D);
 
   assert(D.hasLocalStorage());
   return EmitAutoVarDecl(D);
 }
 
-static std::string GetStaticDeclName(CodeGenFunction &CGF, const VarDecl &D) {
-  CodeGenModule &CGM = CGF.CGM;
-
-  if (CGF.getLangOpts().CPlusPlus)
+static std::string getStaticDeclName(CodeGenModule &CGM, const VarDecl &D) {
+  if (CGM.getLangOpts().CPlusPlus)
     return CGM.getMangledName(&D).str();
 
-  StringRef ContextName;
-  if (!CGF.CurFuncDecl) {
-    // Better be in a block declared in global scope.
-    const NamedDecl *ND = cast<NamedDecl>(&D);
-    const DeclContext *DC = ND->getDeclContext();
-    if (const BlockDecl *BD = dyn_cast<BlockDecl>(DC))
-      ContextName = CGM.getBlockMangledName(GlobalDecl(), BD);
-    else
-      llvm_unreachable("Unknown context for block static var decl");
-  } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(CGF.CurFuncDecl))
+  // If this isn't C++, we don't need a mangled name, just a pretty one.
+  assert(!D.isExternallyVisible() && "name shouldn't matter");
+  std::string ContextName;
+  const DeclContext *DC = D.getDeclContext();
+  if (const auto *FD = dyn_cast<FunctionDecl>(DC))
     ContextName = CGM.getMangledName(FD);
-  else if (isa<ObjCMethodDecl>(CGF.CurFuncDecl))
-    ContextName = CGF.CurFn->getName();
+  else if (const auto *BD = dyn_cast<BlockDecl>(DC))
+    ContextName = CGM.getBlockMangledName(GlobalDecl(), BD);
+  else if (const auto *OMD = dyn_cast<ObjCMethodDecl>(DC))
+    ContextName = OMD->getSelector().getAsString();
   else
     llvm_unreachable("Unknown context for static var decl");
 
-  return ContextName.str() + "." + D.getNameAsString();
+  ContextName += "." + D.getNameAsString();
+  return ContextName;
 }
 
-llvm::Constant *
-CodeGenFunction::CreateStaticVarDecl(const VarDecl &D,
-                                     llvm::GlobalValue::LinkageTypes Linkage) {
+llvm::Constant *CodeGenModule::getOrCreateStaticVarDecl(
+    const VarDecl &D, llvm::GlobalValue::LinkageTypes Linkage) {
+  // In general, we don't always emit static var decls once before we reference
+  // them. It is possible to reference them before emitting the function that
+  // contains them, and it is possible to emit the containing function multiple
+  // times.
+  if (llvm::Constant *ExistingGV = StaticLocalDeclMap[&D])
+    return ExistingGV;
+
   QualType Ty = D.getType();
   assert(Ty->isConstantSizeType() && "VLAs can't be static");
 
   // Use the label if the variable is renamed with the asm-label extension.
   std::string Name;
   if (D.hasAttr<AsmLabelAttr>())
-    Name = CGM.getMangledName(&D);
+    Name = getMangledName(&D);
   else
-    Name = GetStaticDeclName(*this, D);
+    Name = getStaticDeclName(*this, D);
 
-  llvm::Type *LTy = CGM.getTypes().ConvertTypeForMem(Ty);
+  llvm::Type *LTy = getTypes().ConvertTypeForMem(Ty);
   unsigned AddrSpace =
-   CGM.GetGlobalVarAddressSpace(&D, CGM.getContext().getTargetAddressSpace(Ty));
+      GetGlobalVarAddressSpace(&D, getContext().getTargetAddressSpace(Ty));
   llvm::GlobalVariable *GV =
-    new llvm::GlobalVariable(CGM.getModule(), LTy,
+    new llvm::GlobalVariable(getModule(), LTy,
                              Ty.isConstant(getContext()), Linkage,
-                             CGM.EmitNullConstant(D.getType()), Name, nullptr,
+                             EmitNullConstant(D.getType()), Name, nullptr,
                              llvm::GlobalVariable::NotThreadLocal,
                              AddrSpace);
   GV->setAlignment(getContext().getDeclAlign(&D).getQuantity());
-  CGM.setGlobalVisibility(GV, &D);
+  setGlobalVisibility(GV, &D);
 
   if (D.getTLSKind())
-    CGM.setTLSMode(GV, D);
+    setTLSMode(GV, D);
 
   if (D.isExternallyVisible()) {
     if (D.hasAttr<DLLImportAttr>())
       GV->setDLLStorageClass(llvm::GlobalVariable::DLLImportStorageClass);
     else if (D.hasAttr<DLLExportAttr>())
       GV->setDLLStorageClass(llvm::GlobalVariable::DLLExportStorageClass);
   }
 
   // Make sure the result is of the correct type.
-  unsigned ExpectedAddrSpace = CGM.getContext().getTargetAddressSpace(Ty);
+  unsigned ExpectedAddrSpace = getContext().getTargetAddressSpace(Ty);
+  llvm::Constant *Addr = GV;
   if (AddrSpace != ExpectedAddrSpace) {
     llvm::PointerType *PTy = llvm::PointerType::get(LTy, ExpectedAddrSpace);
-    return llvm::ConstantExpr::getAddrSpaceCast(GV, PTy);
+    Addr = llvm::ConstantExpr::getAddrSpaceCast(GV, PTy);
   }
 
-  return GV;
+  setStaticLocalDeclAddress(&D, Addr);
+
+  // Ensure that the static local gets initialized by making sure the parent
+  // function gets emitted eventually.
+  const Decl *DC = cast<Decl>(D.getDeclContext());
+
+  // We can't name blocks or captured statements directly, so try to emit their
+  // parents.
+  if (isa<BlockDecl>(DC) || isa<CapturedDecl>(DC)) {
+    DC = DC->getNonClosureContext();
+    // FIXME: Ensure that global blocks get emitted.
+    if (!DC)
+      return Addr;
+  }
+
+  GlobalDecl GD;
+  if (const auto *CD = dyn_cast<CXXConstructorDecl>(DC))
+    GD = GlobalDecl(CD, Ctor_Base);
+  else if (const auto *DD = dyn_cast<CXXDestructorDecl>(DC))
+    GD = GlobalDecl(DD, Dtor_Base);
+  else if (const auto *FD = dyn_cast<FunctionDecl>(DC))
+    GD = GlobalDecl(FD);
+  else {
+    // Don't do anything for Obj-C method decls or global closures. We should
+    // never defer them.
+    assert(isa<ObjCMethodDecl>(DC) && "unexpected parent code decl");
+  }
+  if (GD.getDecl())
+    (void)GetAddrOfGlobal(GD);
+
+  return Addr;
 }
 
 /// hasNontrivialDestruction - Determine whether a type's destruction is
 /// non-trivial. If so, and the variable uses static initialization, we must
 /// register its destructor to run on exit.
 static bool hasNontrivialDestruction(QualType T) {
   CXXRecordDecl *RD = T->getBaseElementTypeUnsafe()->getAsCXXRecordDecl();
   return RD && !RD->hasTrivialDestructor();

 void CodeGenFunction::EmitStaticVarDecl(const VarDecl &D,
                                       llvm::GlobalValue::LinkageTypes Linkage) {
   llvm::Value *&DMEntry = LocalDeclMap[&D];
   assert(!DMEntry && "Decl already exists in localdeclmap!");
 
   // Check to see if we already have a global variable for this
   // declaration.  This can happen when double-emitting function
   // bodies, e.g. with complete and base constructors.
-  llvm::Constant *addr =
-    CGM.getStaticLocalDeclAddress(&D);
-
-  if (!addr)
-    addr = CreateStaticVarDecl(D, Linkage);
+  llvm::Constant *addr = CGM.getOrCreateStaticVarDecl(D, Linkage);
 
   // Store into LocalDeclMap before generating initializer to handle
   // circular references.
   DMEntry = addr;
-  CGM.setStaticLocalDeclAddress(&D, addr);
 
   // We can't have a VLA here, but we can have a pointer to a VLA,
   // even though that doesn't really make any sense.
   // Make sure to evaluate VLA bounds now so that we have them for later.
   if (D.getType()->isVariablyModifiedType())
     EmitVariablyModifiedType(D.getType());
 
   // Save the type in case adding the initializer forces a type change.
if (shouldUseMemSetPlusStoresToInitialize(constant,
     // Zero and undef don't require a stores.
     if (!constant->isNullValue() && !isa<llvm::UndefValue>(constant)) {
       Loc = Builder.CreateBitCast(Loc, constant->getType()->getPointerTo());
       emitStoresForInitAfterMemset(constant, Loc, isVolatile, Builder);
     }
   } else {
     // Otherwise, create a temporary global with the initializer then
     // memcpy from the global to the alloca.
-    std::string Name = GetStaticDeclName(*this, D);
+    std::string Name = getStaticDeclName(CGM, D);
     llvm::GlobalVariable *GV =
       new llvm::GlobalVariable(CGM.getModule(), constant->getType(), true,
                                llvm::GlobalValue::PrivateLinkage,
                                constant, Name);
     GV->setAlignment(alignment.getQuantity());
     GV->setUnnamedAddr(true);
 
     llvm::Value *SrcPtr = GV;

cfe/trunk/lib/CodeGen/CGExpr.cpp

if (const auto *VD = dyn_cast<VarDecl>(ND)) {
     // Check if this is a global variable.
     if (VD->hasLinkage() || VD->isStaticDataMember())
       return EmitGlobalVarDeclLValue(*this, E, VD);
 
     bool isBlockVariable = VD->hasAttr<BlocksAttr>();
 
     llvm::Value *V = LocalDeclMap.lookup(VD);
     if (!V && VD->isStaticLocal())
-      V = CGM.getStaticLocalDeclAddress(VD);
+      V = CGM.getOrCreateStaticVarDecl(
+          *VD, CGM.getLLVMLinkageVarDefinition(VD, /*isConstant=*/false));
 
     // Use special handling for lambdas.
     if (!V) {
       if (FieldDecl *FD = LambdaCaptureFields.lookup(VD)) {
         return EmitCapturedFieldLValue(*this, FD, CXXABIThisValue);
       } else if (CapturedStmtInfo) {
         if (const FieldDecl *FD = CapturedStmtInfo->lookup(VD))
           return EmitCapturedFieldLValue(*this, FD,

cfe/trunk/lib/CodeGen/CGExprConstant.cpp

if (const ValueDecl *Decl = LVBase.dyn_cast<const ValueDecl*>()) {
       if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(Decl))
         return CGM.GetAddrOfFunction(FD);
       if (const VarDecl* VD = dyn_cast<VarDecl>(Decl)) {
         // We can never refer to a variable with local storage.
         if (!VD->hasLocalStorage()) {
           if (VD->isFileVarDecl() || VD->hasExternalStorage())
             return CGM.GetAddrOfGlobalVar(VD);
           else if (VD->isLocalVarDecl())
-            return CGM.getStaticLocalDeclAddress(VD);
+            return CGM.getOrCreateStaticVarDecl(
+                *VD, CGM.getLLVMLinkageVarDefinition(VD, /*isConstant=*/false));
         }
       }
       return nullptr;
     }
 
     Expr *E = const_cast<Expr*>(LVBase.get<const Expr*>());
     switch (E->getStmtClass()) {
     default: break;

cfe/trunk/lib/CodeGen/CodeGenFunction.h

   void EmitComplexExprIntoLValue(const Expr *E, LValue dest, bool isInit);
 
   /// EmitStoreOfComplex - Store a complex number into the specified l-value.
   void EmitStoreOfComplex(ComplexPairTy V, LValue dest, bool isInit);
 
   /// EmitLoadOfComplex - Load a complex number from the specified l-value.
   ComplexPairTy EmitLoadOfComplex(LValue src, SourceLocation loc);
 
-  /// CreateStaticVarDecl - Create a zero-initialized LLVM global for
-  /// a static local variable.
-  llvm::Constant *CreateStaticVarDecl(const VarDecl &D,
-                                      llvm::GlobalValue::LinkageTypes Linkage);
-
   /// AddInitializerToStaticVarDecl - Add the initializer for 'D' to the
   /// global variable that has already been created for it.  If the initializer
   /// has a different type than GV does, this may free GV and return a different
   /// one.  Otherwise it just returns GV.
   llvm::GlobalVariable *
   AddInitializerToStaticVarDecl(const VarDecl &D,
                                 llvm::GlobalVariable *GV);
 

cfe/trunk/lib/CodeGen/CodeGenModule.h

public:
   llvm::Constant *getStaticLocalDeclAddress(const VarDecl *D) {
     return StaticLocalDeclMap[D];
   }
   void setStaticLocalDeclAddress(const VarDecl *D, 
                                  llvm::Constant *C) {
     StaticLocalDeclMap[D] = C;
   }
 
+  llvm::Constant *
+  getOrCreateStaticVarDecl(const VarDecl &D,
+                           llvm::GlobalValue::LinkageTypes Linkage);
+
   llvm::GlobalVariable *getStaticLocalDeclGuardAddress(const VarDecl *D) {
     return StaticLocalDeclGuardMap[D];
   }
   void setStaticLocalDeclGuardAddress(const VarDecl *D, 
                                       llvm::GlobalVariable *C) {
     StaticLocalDeclGuardMap[D] = C;
   }
 

cfe/trunk/test/CodeGenCXX/static-local-in-local-class.cpp

-// RUN: %clang_cc1 -emit-llvm -o %t %s
-// PR6769
+// RUN: %clang_cc1 -triple x86_64-linux -fblocks -emit-llvm -o - %s -std=c++1y | FileCheck %s
 
+// CHECK: @"_ZZNK3$_2clEvE1x" = internal global i32 42
+// CHECK: @_ZZ18static_local_labelPvE1q = linkonce_odr global i8* blockaddress(@_Z18static_local_labelPv, %{{.*}})
+// CHECK: @_ZZZL20block_deduced_returnvEUb_E1n = internal global i32 42
+// CHECK: @_ZZL14deduced_returnvE1n = internal global i32 42
+// CHECK: @"_ZZZNK17pr18020_constexpr3$_1clEvENKUlvE_clEvE2l2" =
+// CHECK: internal global i32* @"_ZZNK17pr18020_constexpr3$_1clEvE2l1"
+
+namespace pr6769 {
 struct X {
   static void f();
 };
 
 void X::f() {
   static int *i;
   {
     struct Y {
       static void g() {
         i = new int();
   *i = 100;
   (*i) = (*i) +1;
       }
     };
     (void)Y::g();
   }
   (void)i;
 }
+}
 
-// pr7101
+namespace pr7101 {
 void foo() {
     static int n = 0;
     struct Helper {
         static void Execute() {
             n++;
         }
     };
     Helper::Execute();
 }
+}
+
+// These tests all break the assumption that the static var decl has to be
+// emitted before use of the var decl.  This happens because we defer emission
+// of variables with internal linkage and no initialization side effects, such
+// as 'x'.  Then we hit operator()() in 'f', and emit the callee before we emit
+// the arguments, so we emit the innermost function first.
+
+namespace pr18020_lambda {
+// Referring to l1 before emitting it used to crash.
+auto x = []() {
+  static int l1 = 0;
+  return [] { return l1; };
+};
+int f() { return x()(); }
+}
+
+// CHECK-LABEL: define internal i32 @"_ZZNK14pr18020_lambda3$_0clEvENKUlvE_clEv"
+// CHECK: load i32* @"_ZZNK14pr18020_lambda3$_0clEvE2l1"
+
+namespace pr18020_constexpr {
+// Taking the address of l1 in a constant expression used to crash.
+auto x = []() {
+  static int l1 = 0;
+  return [] {
+    static int *l2 = &l1;
+    return *l2;
+  };
+};
+int f() { return x()(); }
+}
+
+// CHECK-LABEL: define internal i32 @"_ZZNK17pr18020_constexpr3$_1clEvENKUlvE_clEv"
+// CHECK: load i32** @"_ZZZNK17pr18020_constexpr3$_1clEvENKUlvE_clEvE2l2"
+
+// Lambda-less reduction that references l1 before emitting it.  This didn't
+// crash if you put it in a namespace.
+struct pr18020_class {
+  auto operator()() {
+    static int l1 = 0;
+    struct U {
+      int operator()() { return l1; }
+    };
+    return U();
+  }
+};
+static pr18020_class x;
+int pr18020_f() { return x()(); }
+
+// CHECK-LABEL: define linkonce_odr i32 @_ZZN13pr18020_classclEvEN1UclEv
+// CHECK: load i32* @_ZZN13pr18020_classclEvE2l1
+
+// In this test case, the function containing the static local will not be
+// emitted because it is unneeded. However, the operator call of the inner class
+// is called, and the static local is referenced and must be emitted.
+static auto deduced_return() {
+  static int n = 42;
+  struct S { int *operator()() { return &n; } };
+  return S();
+}
+extern "C" int call_deduced_return_operator() {
+  return *decltype(deduced_return())()();
+}
+
+// CHECK-LABEL: define i32 @call_deduced_return_operator()
+// CHECK: call i32* @_ZZL14deduced_returnvEN1SclEv(%struct.S* %tmp)
+// CHECK: load i32* %
+// CHECK: ret i32 %
+
+// CHECK-LABEL: define internal i32* @_ZZL14deduced_returnvEN1SclEv(%struct.S* %this)
+// CHECK: ret i32* @_ZZL14deduced_returnvE1n
+
+static auto block_deduced_return() {
+  auto (^b)() = ^() {
+    static int n = 42;
+    struct S { int *operator()() { return &n; } };
+    return S();
+  };
+  return b();
+}
+extern "C" int call_block_deduced_return() {
+  return *decltype(block_deduced_return())()();
+}
+
+// CHECK-LABEL: define i32 @call_block_deduced_return()
+// CHECK: call i32* @_ZZZL20block_deduced_returnvEUb_EN1SclEv(%struct.S.6* %tmp)
+// CHECK: load i32* %
+// CHECK: ret i32 %
+
+// CHECK-LABEL: define internal i32* @_ZZZL20block_deduced_returnvEUb_EN1SclEv(%struct.S.6* %this) #0 align 2 {
+// CHECK: ret i32* @_ZZZL20block_deduced_returnvEUb_E1n
+
+inline auto static_local_label(void *p) {
+  if (p)
+    goto *p;
+  static void *q = &&label;
+  struct S { static void *get() { return q; } };
+  return S();
+label:
+  __builtin_abort();
+}
+void *global_label = decltype(static_local_label(0))::get();
+
+// CHECK-LABEL: define linkonce_odr i8* @_ZZ18static_local_labelPvEN1S3getEv()
+// CHECK: %[[lbl:[^ ]*]] = load i8** @_ZZ18static_local_labelPvE1q
+// CHECK: ret i8* %[[lbl]]
+
+auto global_lambda = []() {
+  static int x = 42;
+  struct S { static int *get() { return &x; } };
+  return S();
+};
+extern "C" int use_global_lambda() {
+  return *decltype(global_lambda())::get();
+}
+// CHECK-LABEL: define i32 @use_global_lambda()
+// CHECK: call i32* @"_ZZNK3$_2clEvEN1S3getEv"()
 
+// CHECK-LABEL: define internal i32* @"_ZZNK3$_2clEvEN1S3getEv"()
+// CHECK: ret i32* @"_ZZNK3$_2clEvE1x"