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

173–174

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.

189

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.

218–219

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

173–174

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

189

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

218–219

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

175

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

306–307

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

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

255

Should we handle ObjCMethodDecls here?

[rnk]

Thanks!

lib/CodeGen/CGDecl.cpp

255

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).

lib/CodeGen/CGDecl.cpp

if (!CGF.CurFuncDecl) {
     ContextName = CGM.getMangledName(FD);
   else if (isa<ObjCMethodDecl>(CGF.CurFuncDecl))
     ContextName = CGF.CurFn->getName();
   else
     llvm_unreachable("Unknown context for static var decl");
 
   return ContextName.str() + "." + D.getNameAsString();
 }
 
-llvm::Constant *
-CodeGenFunction::CreateStaticVarDecl(const VarDecl &D,
-                                     llvm::GlobalValue::LinkageTypes Linkage) {
-  QualType Ty = D.getType();
+llvm::Constant *CodeGenFunction::GetAddrOfStaticLocal(const VarDecl &VD) {
+  // Static locals can be used inside nested functions, so they are tracked on
+  // the CodeGenModule.
+  llvm::Constant *Addr = CGM.getStaticLocalDeclAddress(&VD);
+  if (Addr)
+    return Addr;
+
+  QualType Ty = VD.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);
+  if (VD.hasAttr<AsmLabelAttr>())
+    Name = CGM.getMangledName(&VD);
   else
-    Name = GetStaticDeclName(*this, D);
+    Name = GetStaticDeclName(*this, VD);
 
   llvm::Type *LTy = CGM.getTypes().ConvertTypeForMem(Ty);
-  unsigned AddrSpace =
-   CGM.GetGlobalVarAddressSpace(&D, CGM.getContext().getTargetAddressSpace(Ty));
+  unsigned AddrSpace = CGM.GetGlobalVarAddressSpace(
+      &VD, CGM.getContext().getTargetAddressSpace(Ty));
+  llvm::GlobalVariable *GV = new llvm::GlobalVariable(
+      CGM.getModule(), LTy, Ty.isConstant(getContext()),
+      llvm::GlobalValue::ExternalLinkage, nullptr, Name, nullptr,
+      llvm::GlobalVariable::NotThreadLocal, AddrSpace);
+
+  // Make sure the result is of the correct type.
+  unsigned ExpectedAddrSpace = CGM.getContext().getTargetAddressSpace(Ty);
+  if (AddrSpace != ExpectedAddrSpace) {
+    llvm::PointerType *PTy = llvm::PointerType::get(LTy, ExpectedAddrSpace);
+    Addr = llvm::ConstantExpr::getAddrSpaceCast(GV, PTy);
+  } else {
+    Addr = GV;
+  }
+
+  CGM.setStaticLocalDeclAddress(&VD, Addr);
+  return Addr;
+}
+
+llvm::Constant *
+CodeGenFunction::CreateStaticVarDecl(const VarDecl &D,
+                                     llvm::GlobalValue::LinkageTypes Linkage) {
+  llvm::Constant *Addr = GetAddrOfStaticLocal(D);
+
+  // The address has to be a cast of a global variable.
   llvm::GlobalVariable *GV =
-    new llvm::GlobalVariable(CGM.getModule(), LTy,
-                             Ty.isConstant(getContext()), Linkage,
-                             CGM.EmitNullConstant(D.getType()), Name, nullptr,
-                             llvm::GlobalVariable::NotThreadLocal,
-                             AddrSpace);
+      cast<llvm::GlobalVariable>(Addr->stripPointerCasts());
+
+  // 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.
+  if (!GV->isDeclaration())
+    return Addr;
+
+  // Set initializer, linkage, etc, now that we have a definition.
+  GV->setInitializer(CGM.EmitNullConstant(D.getType()));
+  GV->setLinkage(Linkage);
   GV->setAlignment(getContext().getDeclAlign(&D).getQuantity());
   CGM.setGlobalVisibility(GV, &D);
 
   if (D.getTLSKind())
     CGM.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);
-  if (AddrSpace != ExpectedAddrSpace) {
-    llvm::PointerType *PTy = llvm::PointerType::get(LTy, ExpectedAddrSpace);
-    return llvm::ConstantExpr::getAddrSpaceCast(GV, PTy);
-  }
-
-  return GV;
+  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();
 }
 
 /// 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 *
 CodeGenFunction::AddInitializerToStaticVarDecl(const VarDecl &D,
                                                llvm::GlobalVariable *GV) {
   llvm::Constant *Init = CGM.EmitConstantInit(D, this);
 
   // If constant emission failed, then this should be a C++ static
if (GV->getType()->getElementType() != Init->getType()) {
     llvm::ConstantExpr::getBitCast(GV, OldGV->getType());
     OldGV->replaceAllUsesWith(NewPtrForOldDecl);
 
     // Erase the old global, since it is no longer used.
     OldGV->eraseFromParent();
   }
 
   GV->setConstant(CGM.isTypeConstant(D.getType(), true));
   GV->setInitializer(Init);
 
   if (hasNontrivialDestruction(D.getType())) {
     // We have a constant initializer, but a nontrivial destructor. We still
     // need to perform a guarded "initialization" in order to register the
     // destructor.
     EmitCXXGuardedInit(D, GV, /*PerformInit*/false);
   }
 
   return GV;
 }
 
 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 = CreateStaticVarDecl(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.

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 = GetAddrOfStaticLocal(*VD);
 
     // 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,

lib/CodeGen/CGExprConstant.cpp

if (const ValueDecl *Decl = LVBase.dyn_cast<const ValueDecl*>()) {
         return CGM.GetWeakRefReference(Decl);
       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);
+          else if (VD->isLocalVarDecl()) {
+            assert(CGF && "cannot refer to static local outside function");
+            return CGF->GetAddrOfStaticLocal(*VD);
+          }
         }
       }
       return nullptr;
     }
 
     Expr *E = const_cast<Expr*>(LVBase.get<const Expr*>());
     switch (E->getStmtClass()) {
     default: break;
case Expr::PredefinedExprClass: {
         return cast<llvm::Constant>(Res.getAddress());
       } else if (Type == PredefinedExpr::PrettyFunction) {
         return CGM.GetAddrOfConstantCString("top level", ".tmp");
       }
 
       return CGM.GetAddrOfConstantCString("", ".tmp");
     }
     case Expr::AddrLabelExprClass: {
       assert(CGF && "Invalid address of label expression outside function.");
       llvm::Constant *Ptr =
         CGF->GetAddrOfLabel(cast<AddrLabelExpr>(E)->getLabel());
       return llvm::ConstantExpr::getBitCast(Ptr, ConvertType(E->getType()));
     }
     case Expr::CallExprClass: {
       CallExpr* CE = cast<CallExpr>(E);
       unsigned builtin = CE->getBuiltinCallee();
       if (builtin !=
             Builtin::BI__builtin___CFStringMakeConstantString &&

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);
 
+  /// Get the address of a static local variabld declaration.
+  llvm::Constant *GetAddrOfStaticLocal(const VarDecl &VD);
+
   /// 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

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

-// RUN: %clang_cc1 -emit-llvm -o %t %s
-// PR6769
+// RUN: %clang_cc1 -triple x86_64-linux -emit-llvm -o - %s -std=c++1y | FileCheck %s
 
+
+// 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
 
+// FIXME: Surely we can exercise this edge case in C with -fblocks.