[clang][Interp] Create only globals when initializing a global variable

[tbaeder]

Summary

For this code:

struct O {
  int &&j;
};

O o1(0);

The generated AST for the initializer of o1 is:

VarDecl 0x62100006ab08 <array.cpp:119:3, col:9> col:5 o1 'O':'O' parenlistinit
`-ExprWithCleanups 0x62100006b250 <col:7, col:9> 'O':'O'
  `-CXXParenListInitExpr 0x62100006b210 <col:7, col:9> 'O':'O'
    `-MaterializeTemporaryExpr 0x62100006b1f0 <col:8> 'int' xvalue
      `-IntegerLiteral 0x62100006abd0 <col:8> 'int' 0

Before this patch, we create a local temporary variable for the MaterializeTemporaryExpr and destroy it again when destroying the EvalEmitter we create to interpret the initializer. However, since O::j is a reference, this reference now points to a local variable that doesn't exist anymore.

I've run into this issue a few times before but always postponed working on it. Does this make sense? The only downside I see is that we might be creating more global variables.

[tbaeder]

Ping

[tbaeder]

Ping

[tbaeder]

Ping

[tbaeder]

Ping

[tbaeder]

Ping

[tbaeder]

Ping

[tbaeder]

Ping

[aaron.ballman]

I guess I'm a bit lost as to what the code example has to do with the constant expression interpreter in the first place.

[tbaeder]

It gets interpreted as a constant expression in Sema::CheckCompleteVariableInitialization():

* #0: Context.cpp:73  clang::interp::Context::evaluateAsInitializer(this=0x0000608000005d20, Parent=0x00007fff6dc2a8d0, VD=0x0000621000073b48, Result=0x0000621000099e90)
  #1: ExprConstant.cpp:15600  clang::Expr::EvaluateAsInitializer(this=0x0000621000099e40, Value=0x0000621000099e90, Ctx=0x000062a000000200, VD=0x0000621000073b48, Notes=0x00007fff6deac850, IsConstantInitialization=true) const
  #2: Decl.cpp:2555  clang::VarDecl::evaluateValueImpl(this=0x0000621000073b48, Notes=0x00007fff6deac850, IsConstantInitialization=true) const
  #3: Decl.cpp:2626  clang::VarDecl::checkForConstantInitialization(this=0x0000621000073b48, Notes=0x00007fff6deac850) const
  #4: SemaDecl.cpp:14399  clang::Sema::CheckCompleteVariableDeclaration(this=0x000062900000a200, var=0x0000621000073b48)

[tbaeder]

Ping

[aaron.ballman]

In D156453#4652939, @tbaeder wrote:

It gets interpreted as a constant expression in Sema::CheckCompleteVariableInitialization():

* #0: Context.cpp:73  clang::interp::Context::evaluateAsInitializer(this=0x0000608000005d20, Parent=0x00007fff6dc2a8d0, VD=0x0000621000073b48, Result=0x0000621000099e90)
  #1: ExprConstant.cpp:15600  clang::Expr::EvaluateAsInitializer(this=0x0000621000099e40, Value=0x0000621000099e90, Ctx=0x000062a000000200, VD=0x0000621000073b48, Notes=0x00007fff6deac850, IsConstantInitialization=true) const
  #2: Decl.cpp:2555  clang::VarDecl::evaluateValueImpl(this=0x0000621000073b48, Notes=0x00007fff6deac850, IsConstantInitialization=true) const
  #3: Decl.cpp:2626  clang::VarDecl::checkForConstantInitialization(this=0x0000621000073b48, Notes=0x00007fff6deac850) const
  #4: SemaDecl.cpp:14399  clang::Sema::CheckCompleteVariableDeclaration(this=0x000062900000a200, var=0x0000621000073b48)

Ahh, we are doing this for static initialization cases with thread locals (https://godbolt.org/z/73dMx55xq), okay, that makes more sense now. Thank you!

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1213

Should we be looking at the TLS kind of the extended declaration? (CheckCompleteVariableDeclaration() is using VarDecl::getTLSKind() == VarDecl::TLS_Static)

Would something along these lines work instead?

bool EmitGlobalTemp = E->getStorageDuration() == SD_Static;
if (!EmitGlobalTemp) {
  if (const LifetimeExtendedTemporaryDecl *LETD = E->getLifetimeExtendedTemporaryDecl()) {
    if (const auto *VD = dyn_cast_if_present<VarDecl>(LETD->getExtendingDecl()) {
      EmitGlobalTemp= VD->getTLSKind() == VarDecl::TLS_Static;
    }
  }
}

clang/test/AST/Interp/records.cpp

931

It'd be nice to add another test for:

constinit O o2(0); // error: can't bind to temporary

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1213

That code definitely works for the current AST/Interp/ tests, but we don't have tests for thread local stuff in there right now.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1213

Hmm, I think we'll need those tests: https://eel.is/c++draft/expr.const#5.2

That seems to be the only mention about thread local storage duration for constant expressions in C++, so it might make sense to tackle that as part of this change?

(I worry that we'll forget to come back to this detail later, basically. So either we should have failing test coverage showing we need a fix, an issue in GitHub so we know to come back to it, etc. or just do the work up front given that it's closely related.)

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1213

I've pushed https://github.com/llvm/llvm-project/commit/11f5e5eb90c883d4b9ddba318e8fc57914b22ef3

As you can see, the problem also exists for static variables. I think this needs additional checks at interpretation time, or even new opcodes to check the declaration when the initializer is computed. So I'd rather have this in a separate followup patch.

[aaron.ballman]

LGTM

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1213

Follow-up patch is fine, thank you for the additional test coverage for it!

clang/lib/AST/Interp/ByteCodeExprGen.h

protected:
   const Expr *SourceLocDefaultExpr = nullptr;
 
   /// Flag indicating if return value is to be discarded.
   bool DiscardResult = false;
 
   /// Flag inidicating if we're initializing an already created
   /// variable. This is set in visitInitializer().
   bool Initializing = false;
+
+  /// Flag indicating if we're initializing a global variable.
+  bool GlobalDecl = false;
 };
 
 extern template class ByteCodeExprGen<ByteCodeEmitter>;
 extern template class ByteCodeExprGen<EvalEmitter>;
 
 /// Scope chain managing the variable lifetimes.
 template <class Emitter> class VariableScope {
 public:

clang/lib/AST/Interp/ByteCodeExprGen.cpp

 
 namespace clang {
 namespace interp {
 
 /// Scope used to handle temporaries in toplevel variable declarations.
 template <class Emitter> class DeclScope final : public VariableScope<Emitter> {
 public:
   DeclScope(ByteCodeExprGen<Emitter> *Ctx, const ValueDecl *VD)
-      : VariableScope<Emitter>(Ctx), Scope(Ctx->P, VD) {}
+      : VariableScope<Emitter>(Ctx), Scope(Ctx->P, VD),
+        OldGlobalDecl(Ctx->GlobalDecl) {
+    Ctx->GlobalDecl = Context::shouldBeGloballyIndexed(VD);
+  }
 
   void addExtended(const Scope::Local &Local) override {
     return this->addLocal(Local);
   }
 
+  ~DeclScope() { this->Ctx->GlobalDecl = OldGlobalDecl; }
+
 private:
   Program::DeclScope Scope;
+  bool OldGlobalDecl;
 };
 
 /// Scope used to handle initialization methods.
 template <class Emitter> class OptionScope final {
 public:
   /// Root constructor, compiling or discarding primitives.
   OptionScope(ByteCodeExprGen<Emitter> *Ctx, bool NewDiscardResult,
               bool NewInitializing)
if (Initializing) {
     // We already have a value, just initialize that.
     return this->visitInitializer(SubExpr);
   }
   // If we don't end up using the materialized temporary anyway, don't
   // bother creating it.
   if (DiscardResult)
     return this->discard(SubExpr);
 
+  // When we're initializing a global variable *or* the storage duration of
+  // the temporary is explicitly static, create a global variable.
   std::optional<PrimType> SubExprT = classify(SubExpr);
-  if (E->getStorageDuration() == SD_Static) {
+  bool IsStatic = E->getStorageDuration() == SD_Static;
+  if (GlobalDecl || IsStatic) {
     std::optional<unsigned> GlobalIndex = P.createGlobal(E);
     if (!GlobalIndex)
       return false;
 
     const LifetimeExtendedTemporaryDecl *TempDecl =
         E->getLifetimeExtendedTemporaryDecl();
 
     if (SubExprT) {
       if (!this->visit(SubExpr))
         return false;
+      if (IsStatic) {
         if (!this->emitInitGlobalTemp(*SubExprT, *GlobalIndex, TempDecl, E))
           return false;
+      } else {
+        if (!this->emitInitGlobal(*SubExprT, *GlobalIndex, E))
+          return false;
+      }
       return this->emitGetPtrGlobal(*GlobalIndex, E);
     }
 
     // Non-primitive values.
     if (!this->emitGetPtrGlobal(*GlobalIndex, E))
       return false;
     if (!this->visitInitializer(SubExpr))
       return false;
+    if (IsStatic)
       return this->emitInitGlobalTempComp(TempDecl, E);
+    return true;
   }
 
   // For everyhing else, use local variables.
   if (SubExprT) {
     if (std::optional<unsigned> LocalIndex = allocateLocalPrimitive(
             SubExpr, *SubExprT, /*IsConst=*/true, /*IsExtended=*/true)) {
       if (!this->visit(SubExpr))
         return false;

clang/test/AST/Interp/records.cpp

struct A {
     int a;
   };
 
   struct B : A {
     int b;
   };
 
   constexpr B b(A(1),2);
+
+
+  struct O {
+    int &&j;
+  };
+
+  /// Not constexpr!
+  O o1(0);
 }
 #endif

clang/test/SemaCXX/paren-list-agg-init.cpp

 // RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only
+// RUN: %clang_cc1 -verify -std=c++20 %s -fsyntax-only -fexperimental-new-constant-interpreter
+// RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only -fexperimental-new-constant-interpreter
 // RUN: %clang_cc1 -verify=expected,beforecxx20 -Wc++20-extensions -std=c++20 %s -fsyntax-only
 
 struct A { // expected-note 4{{candidate constructor}}
   char i;
   double j;
 };
 
 struct B {