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

[Timm Bäder via Phabricator via cfe-commits]

tbaeder created this revision.
tbaeder added reviewers: aaron.ballman, erichkeane, shafik, cor3ntin.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156453

Files:
  clang/lib/AST/Interp/ByteCodeExprGen.cpp
  clang/lib/AST/Interp/ByteCodeExprGen.h
  clang/test/AST/Interp/records.cpp
  clang/test/SemaCXX/paren-list-agg-init.cpp


Index: clang/test/SemaCXX/paren-list-agg-init.cpp
===================================================================
--- clang/test/SemaCXX/paren-list-agg-init.cpp
+++ clang/test/SemaCXX/paren-list-agg-init.cpp
@@ -1,4 +1,6 @@
 // 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}}
Index: clang/test/AST/Interp/records.cpp
===================================================================
--- clang/test/AST/Interp/records.cpp
+++ clang/test/AST/Interp/records.cpp
@@ -921,5 +921,13 @@
   };
 
   constexpr B b(A(1),2);
+
+
+  struct O {
+    int &&j;
+  };
+
+  /// Not constexpr!
+  O o1(0);
 }
 #endif
Index: clang/lib/AST/Interp/ByteCodeExprGen.h
===================================================================
--- clang/lib/AST/Interp/ByteCodeExprGen.h
+++ clang/lib/AST/Interp/ByteCodeExprGen.h
@@ -304,6 +304,9 @@
   /// 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>;
Index: clang/lib/AST/Interp/ByteCodeExprGen.cpp
===================================================================
--- clang/lib/AST/Interp/ByteCodeExprGen.cpp
+++ clang/lib/AST/Interp/ByteCodeExprGen.cpp
@@ -29,14 +29,20 @@
 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.
@@ -1201,8 +1207,11 @@
   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;
@@ -1213,8 +1222,13 @@
     if (SubExprT) {
       if (!this->visit(SubExpr))
         return false;
-      if (!this->emitInitGlobalTemp(*SubExprT, *GlobalIndex, TempDecl, E))
-        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);
     }
 
@@ -1223,7 +1237,9 @@
       return false;
     if (!this->visitInitializer(SubExpr))
       return false;
-    return this->emitInitGlobalTempComp(TempDecl, E);
+    if (IsStatic)
+      return this->emitInitGlobalTempComp(TempDecl, E);
+    return true;
   }
 
   // For everyhing else, use local variables.

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits