Emit discardable-ODR variables via odr-use instead of UsedAttr in GetVariableOffset

lib/CppInterOp/CppInterOp.cpp

@@ -59,6 +59,7 @@
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticSema.h"
+#include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangStandard.h"
 #include "clang/Basic/Linkage.h"
 #include "clang/Basic/OperatorKinds.h"
@@ -402,6 +403,91 @@
 #endif
 }
 
+// Force emission of a variable's definition by synthesizing an odr-use of
+// it, instead of ForceCodeGen's UsedAttr route. The UsedAttr route records
+// the emitted global in codegen's llvm.used list as a weak handle; the
+// handle can go null (global deleted) before a later incremental PTU's
+// emitUsed runs, and release-built clang dereferences it without checking —
+// a process crash that accumulates with interpreter state rather than
+// tracing to any one declaration. The odr-use anchor is a dummy global
+// initialized with the variable's address, built directly through Sema (the
+// AST that parsing "&var" would produce — no source-text round-trip), so
+// the definition flows through the regular deferred-decl path and leaves no
+// used-list residue. Returns false when the odr-use cannot be built — the
+// caller falls back to ForceCodeGen.
+// FIXME: Remove the synthesized-anchor mechanism once clang grows a direct
+// interpreter emission API for a GlobalDecl (the emitUsed hardening it
+// depends on landed via llvm/llvm-project#210959):
+#if CLANG_VERSION_MAJOR >= 24
+#warning "Revisit EmitVariableViaOdrUse: a direct-emission API may exist now"
+#endif
+static bool EmitVariableViaOdrUse(compat::Interpreter& I, VarDecl* VD) {
+  // A reference cannot conjure a definition that does not exist: the dummy
+  // would carry an unresolvable symbol into the JIT (Emscripten's dynamic
+  // loader rejects the whole module over it, and the stale entry then
+  // breaks every later load in the process). The UsedAttr fallback defers
+  // harmlessly for definition-less declarations.
+  VarDecl* Def = VD->getDefinition();
+  if (!Def)
+    return false;
+
+  Sema& S = I.getCI()->getSema();
+  ASTContext& C = S.getASTContext();
+
+  // Open the synthesizing region before ANY Sema work: odr-use marking in
+  // BuildDeclRefExpr can trigger an immediate static-member instantiation,
+  // and cling's DeclCollector aborts on instantiation callbacks that arrive
+  // outside an active transaction. (Inert under clang-repl — TODO destructor
+  // in Compatibility.h.)
+  compat::SynthesizingCodeRAII RAII(&I);
+
+  // Build '&VD' the way the parser would; the address-of marks Def
+  // odr-used, which is what schedules the deferred definition.
+  ExprResult Ref = S.BuildDeclRefExpr(Def, Def->getType().getNonReferenceType(),
+                                      VK_LValue, SourceLocation());
+  if (Ref.isInvalid())
+    return false;
+  ExprResult AddrOf =
+      S.CreateBuiltinUnaryOp(SourceLocation(), UO_AddrOf, Ref.get());
+  if (AddrOf.isInvalid())
+    return false;
+
+  // Anchor the odr-use in an external-linkage dummy the JIT must emit; the
+  // initializer's implicit conversion to 'const void*' is Sema-checked, so
+  // exotic types fail over to the fallback instead of asserting.
+  // FIXME: The synthesized nodes are parked in the TU for the rest of the
+  // session; move them under a scratch area whose AST nodes get deallocated
+  // after emission.
+  std::string DummyName = "__cppinterop_odr_use_v" +
+                          std::to_string(getInterpInfo(&I).OdrUseCounter++);
+  QualType Ty = C.getPointerType(C.VoidTy.withConst());
+  TranslationUnitDecl* TU = C.getTranslationUnitDecl();
+  VarDecl* Dummy = VarDecl::Create(C, TU, SourceLocation(), SourceLocation(),
+                                   &C.Idents.get(DummyName), Ty,
+                                   C.getTrivialTypeSourceInfo(Ty), SC_None);
+  S.AddInitializerToDecl(Dummy, AddrOf.get(), /*DirectInit=*/false);
+  if (Dummy->isInvalidDecl())
+    return false;
+  TU->addDecl(Dummy);
+
+  // Under cling the RAII's transaction commit (at scope exit) triggers
+  // emission of everything collected above plus this dummy.
+  I.getCI()->getASTConsumer().HandleTopLevelDecl(DeclGroupRef(Dummy));
+#ifndef CPPINTEROP_USE_CLING
+  // FIXME: Parsing an empty string is the only way to flush incremental
+  // CodeGen for a decl handed straight to the consumer — the state-reset
+  // bug SynthesizingCodeRAII's clang-repl destructor should eventually
+  // own. Drop this when it does.
+  auto GeneratedPTU = I.Parse("");
+  if (llvm::Error Err =
+          !GeneratedPTU ? GeneratedPTU.takeError() : I.Execute(*GeneratedPTU)) {
+    llvm::consumeError(std::move(Err));
+    return false;
+  }
+#endif
+  return true;
+}
+
 #define DEBUG_TYPE "jitcall"
 bool JitCall::AreArgumentsValid(void* result, ArgList args, void* self,
                                 size_t nary) const {
@@ -2806,7 +2892,14 @@
     }
     if (!address) {
       auto Linkage = C.GetGVALinkageForVariable(VD);
-      if (isDiscardableGVALinkage(Linkage))
+      // Odr-use emission only for discardable-ODR entities (inline/constexpr
+      // statics) — the class the used-list crash traced to. Internal-linkage
+      // variables cannot be odr-used from a later PTU (module-local symbol:
+      // the reference duplicates or misses the entity), and an
+      // available-externally definition would not be emitted by a mere
+      // reference; both stay on the stock UsedAttr path.
+      if (isDiscardableGVALinkage(Linkage) &&
+          (Linkage != GVA_DiscardableODR || !EmitVariableViaOdrUse(I, VD)))
         ForceCodeGen(VD, I);
     }
     auto VDAorErr = compat::getSymbolAddress(I, StringRef(mangledName));
@@ -4989,7 +5082,7 @@
 };
 } // namespace
 
-int Declare(compat::Interpreter& I, const char* code, bool silent) {
+static int Declare(compat::Interpreter& I, const char* code, bool silent) {
   // Trap diagnostics on both paths: I.declare's rc is 0 even when
   // Parse recovered from emitted errors, so callers need the trap to
   // distinguish "parsed cleanly" from "parsed with errors".

lib/CppInterOp/InterpreterInfo.h

@@ -46,14 +46,19 @@ struct InterpreterInfo {
   // Owns the string arguments passed to clang during creation, since the
   // interpreter keeps the raw argv pointers for its whole lifetime
   std::vector<std::string> ArgvStorage;
+  // Names the synthesized odr-use anchors (EmitVariableViaOdrUse);
+  // per-interpreter so a fresh interpreter's AST does not depend on
+  // sibling interpreters' query history.
+  unsigned OdrUseCounter = 0;
 
   InterpreterInfo(compat::Interpreter* I, bool Owned,
                   std::vector<std::string> ArgvStrs = {})
       : Interpreter(I), isOwned(Owned), ArgvStorage(std::move(ArgvStrs)) {}
 
   InterpreterInfo(InterpreterInfo&& Other) noexcept
       : Interpreter(Other.Interpreter), isOwned(Other.isOwned),
-        ArgvStorage(std::move(Other.ArgvStorage)) {
+        ArgvStorage(std::move(Other.ArgvStorage)),
+        OdrUseCounter(Other.OdrUseCounter) {
     Other.Interpreter = nullptr;
     Other.isOwned = false;
   }
@@ -64,6 +69,7 @@ struct InterpreterInfo {
       Interpreter = Other.Interpreter;
       isOwned = Other.isOwned;
       ArgvStorage = std::move(Other.ArgvStorage);
+      OdrUseCounter = Other.OdrUseCounter;
       Other.Interpreter = nullptr;
       Other.isOwned = false;
     }

unittests/CppInterOp/VariableReflectionTest.cpp

@@ -1,16 +1,24 @@
 #include "Utils.h"
 
+#include "../../lib/CppInterOp/Unwrap.h"
+
 #include "CppInterOp/CppInterOp.h"
+#include "CppInterOp/CppInterOpTypes.h"
 
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
 #include "clang/Basic/Version.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Sema/Sema.h"
 
+#include "llvm/Support/Error.h"
+
 #include "gtest/gtest.h"
-#include <string>
 
+#include <array>
 #include <cstddef>
+#include <string>
+#include <utility>
 
 using namespace TestUtils;
 using namespace llvm;
@@ -376,6 +384,211 @@ TYPED_TEST(CPPINTEROP_TEST_MODE,
   EXPECT_FALSE(Cpp::GetVariableOffset(var2));
 }
 
+TYPED_TEST(CPPINTEROP_TEST_MODE,
+           VariableReflection_GetVariableOffset_NoStaleUsedHandle) {
+#ifdef __EMSCRIPTEN__
+  // The stale-handle crash this test pins is native-JIT mechanics (ORC
+  // freeing materialized IR), and a single failed module load poisons
+  // Emscripten's process-global dynamic-linking state for every test that
+  // follows (issue #1071) — not worth the fragility for a native-only
+  // scenario.
+  GTEST_SKIP() << "Stale-used-handle scenario is native-JIT specific";
+#endif
+  // Emitting a discardable-ODR variable through the UsedAttr route records
+  // the global as a weak handle in codegen's llvm.used list. If the global
+  // is later replaced/erased (weak-def discard when a later PTU re-emits
+  // the same entity), the next PTU's emitUsed dereferences the nulled
+  // handle. The offset query must not leave used-list residue.
+  TestFixture::CreateInterpreter();
+  Cpp::Declare(R"(
+    struct UsedHandle {
+      inline static int probe = 3;
+    };
+  )");
+  Cpp::DeclRef klass = Cpp::GetNamed("UsedHandle");
+  EXPECT_TRUE(klass);
+  Cpp::DeclRef var = Cpp::GetNamed("probe", klass);
+  EXPECT_TRUE(var);
+  EXPECT_TRUE(Cpp::GetVariableOffset(var));
+  // ForceCodeGen's UsedAttr is planted permanently on the AST decl; the
+  // odr-use route must not.
+  EXPECT_FALSE(Cpp::unwrap<Decl>(var)->hasAttr<clang::UsedAttr>());
+#ifndef CPPINTEROP_USE_CLING
+  // The UsedAttr lives on the AST decl, so every later PTU that re-emits
+  // the entity re-adds it to that module's llvm.used — the residue the
+  // stale-handle crash grows from. Neither a module that re-emits the
+  // variable nor an unrelated one may carry llvm.used. (PTU/TheModule is
+  // clang::Interpreter surface; cling covers this path via the UsedAttr
+  // assert above.)
+  {
+    auto PTUOrErr = Interp->Parse("int consume_probe = UsedHandle::probe;");
+    ASSERT_TRUE(bool(PTUOrErr));
+    EXPECT_EQ(PTUOrErr->TheModule->getNamedGlobal("llvm.used"), nullptr);
+    if (auto Err = Interp->Execute(*PTUOrErr))
+      llvm::consumeError(std::move(Err));
+  }
+  {
+    auto PTUOrErr = Interp->Parse("int flush_ptu = 0;");
+    ASSERT_TRUE(bool(PTUOrErr));
+    EXPECT_EQ(PTUOrErr->TheModule->getNamedGlobal("llvm.used"), nullptr);
+    if (auto Err = Interp->Execute(*PTUOrErr))
+      llvm::consumeError(std::move(Err));
+  }
+  EXPECT_TRUE(Cpp::GetNamed("flush_ptu"));
+#endif // !CPPINTEROP_USE_CLING
+  // The crash this guards against was cumulative — many force-emitted
+  // statics plus JIT materialization cycles, interleaved with absorbed
+  // parse failures. Mimic that sweep shape as a regression net; the
+  // per-index sources are assembled at compile time by stringification.
+  struct SweepCase {
+    const char* decl;
+    const char* cls;
+    const char* var;
+    const char* probe;
+    const char* use;
+  };
+#define SWEEP_CASE(n)                                                          \
+  {                                                                            \
+    "struct Sweep" #n " { inline static int v" #n " = " #n "; };", "Sweep" #n, \
+        "v" #n, "template <> struct Sweep" #n "<int>;",                        \
+        "int use" #n " = Sweep" #n "::v" #n ";"                                \
+  }
+  constexpr std::array<SweepCase, 8> Sweeps = {
+      {SWEEP_CASE(0), SWEEP_CASE(1), SWEEP_CASE(2), SWEEP_CASE(3),
+       SWEEP_CASE(4), SWEEP_CASE(5), SWEEP_CASE(6), SWEEP_CASE(7)}};
+#undef SWEEP_CASE
+  for (const SweepCase& S : Sweeps) {
+    Cpp::Declare(S.decl);
+    Cpp::DeclRef k = Cpp::GetNamed(S.cls);
+    ASSERT_TRUE(k);
+    Cpp::DeclRef v = Cpp::GetNamed(S.var, k);
+    ASSERT_TRUE(v);
+    EXPECT_TRUE(Cpp::GetVariableOffset(v));
+    Cpp::Declare(S.probe, /*silent=*/true); // expected parse failure
+    Cpp::Declare(S.use);
+  }
+  Cpp::Declare("int sweep_done = 1;");
+  EXPECT_TRUE(Cpp::GetNamed("sweep_done"));
+
+  // Template-specialization members take the odr-use route too — the
+  // Sema-built address-of needs no source spelling of the specialization,
+  // and the caller has already instantiated the definition.
+  Cpp::Declare(R"(
+    template <typename T> struct TmplStatic {
+      inline static int member = 7;
+    };
+  )");
+  Cpp::DeclRef tmpl = Cpp::GetNamed("TmplStatic");
+  EXPECT_TRUE(tmpl);
+  ASTContext& C = Interp->getCI()->getASTContext();
+  std::vector<Cpp::TemplateArgInfo> template_args = {
+      {C.IntTy.getAsOpaquePtr()}};
+  Cpp::DeclRef inst = Cpp::InstantiateTemplate(tmpl, template_args);
+  EXPECT_TRUE(inst);
+  Cpp::DeclRef member = Cpp::GetNamed("member", inst);
+  EXPECT_TRUE(member);
+  EXPECT_TRUE(Cpp::GetVariableOffset(member));
+  EXPECT_FALSE(Cpp::unwrap<Decl>(member)->hasAttr<clang::UsedAttr>());
+
+  // Anonymous-namespace members cannot be named from a fresh chunk of
+  // source; they bail out of the odr-use route the same way.
+  Cpp::Declare(R"(
+    namespace {
+      struct AnonNsStatic {
+        inline static int member = 9;
+      };
+    }
+  )");
+  Cpp::DeclRef anon_klass = Cpp::GetNamed("AnonNsStatic");
+  ASSERT_TRUE(anon_klass);
+  Cpp::DeclRef anon_member = Cpp::GetNamed("member", anon_klass);
+  ASSERT_TRUE(anon_member);
+  EXPECT_TRUE(Cpp::GetVariableOffset(anon_member));
+#ifndef CPPINTEROP_USE_CLING
+  EXPECT_TRUE(Cpp::unwrap<Decl>(anon_member)->hasAttr<clang::UsedAttr>());
+#endif
+
+  // A linkage-spec block is transparent for naming purposes and stays on
+  // the odr-use route.
+  Cpp::Declare(R"(
+    extern "C++" {
+      struct CxxLinkStatic {
+        inline static int member = 11;
+      };
+    }
+  )");
+  Cpp::DeclRef link_klass = Cpp::GetNamed("CxxLinkStatic");
+  ASSERT_TRUE(link_klass);
+  Cpp::DeclRef link_member = Cpp::GetNamed("member", link_klass);
+  ASSERT_TRUE(link_member);
+  EXPECT_TRUE(Cpp::GetVariableOffset(link_member));
+  EXPECT_FALSE(Cpp::unwrap<Decl>(link_member)->hasAttr<clang::UsedAttr>());
+}
+
+#ifndef CPPINTEROP_USE_CLING
+TYPED_TEST(CPPINTEROP_TEST_MODE,
+           VariableReflection_GetVariableOffset_UnmaterializableDefinition) {
+  // A definition whose dynamic initializer needs an undefined symbol parses
+  // fine but cannot be materialized: the odr-use anchor's execution fails
+  // and the query must fail cleanly. (The anchor's flush block is
+  // clang-repl-only, hence the gate.)
+#ifdef __EMSCRIPTEN__
+  // The intentional materialization failure leaves the process-global wasm
+  // dynamic-linking state broken for every later load (see issue #1071).
+  GTEST_SKIP() << "A failed module load poisons wasm dynamic linking";
+#endif
+#ifdef _WIN32
+  // The intentional failure makes ORC print "JIT session error :" on
+  // stderr, which MSBuild's canonical-error scraping promotes to a build
+  // error (MSB8066) even though every test passes.
+  GTEST_SKIP() << "MSBuild's canonical-error scraping treats ORC's "
+                  "JIT-session-failure stderr line as a build failure";
+#endif
+  TestFixture::CreateInterpreter();
+  Cpp::Declare(R"(
+    int undefined_fn();
+    struct Unmaterializable {
+      inline static int probe = undefined_fn();
+    };
+  )");
+  Cpp::DeclRef klass = Cpp::GetNamed("Unmaterializable");
+  ASSERT_TRUE(klass);
+  Cpp::DeclRef var = Cpp::GetNamed("probe", klass);
+  ASSERT_TRUE(var);
+  EXPECT_FALSE(Cpp::GetVariableOffset(var));
+}
+#endif // !CPPINTEROP_USE_CLING
+
+TYPED_TEST(CPPINTEROP_TEST_MODE,
+           VariableReflection_GetVariableOffset_OdrUseAnchorPerInterpreter) {
+  // The synthesized odr-use anchors must be named per-interpreter: a fresh
+  // interpreter's AST (and anything derived from it, e.g. a crash
+  // reproducer) may not depend on how many queries a sibling interpreter
+  // has run.
+  Cpp::InterpRef I1 = TestFixture::CreateInterpreter();
+  ASSERT_TRUE(I1);
+  Cpp::Declare(R"(struct PerInterpA { inline static int probe = 1; };)");
+  Cpp::DeclRef ka = Cpp::GetNamed("PerInterpA");
+  ASSERT_TRUE(ka);
+  Cpp::DeclRef va = Cpp::GetNamed("probe", ka);
+  ASSERT_TRUE(va);
+  EXPECT_TRUE(Cpp::GetVariableOffset(va));
+  EXPECT_TRUE(Cpp::GetNamed("__cppinterop_odr_use_v0"));
+
+  Cpp::InterpRef I2 = TestFixture::CreateInterpreter();
+  ASSERT_TRUE(I2);
+  Cpp::Declare(R"(struct PerInterpB { inline static int probe = 2; };)");
+  Cpp::DeclRef kb = Cpp::GetNamed("PerInterpB");
+  ASSERT_TRUE(kb);
+  Cpp::DeclRef vb = Cpp::GetNamed("probe", kb);
+  ASSERT_TRUE(vb);
+  EXPECT_TRUE(Cpp::GetVariableOffset(vb));
+  // The second interpreter's first anchor is also its v0.
+  EXPECT_TRUE(Cpp::GetNamed("__cppinterop_odr_use_v0"));
+  EXPECT_FALSE(Cpp::GetNamed("__cppinterop_odr_use_v1"));
+  EXPECT_TRUE(Cpp::DeleteInterpreter(I2));
+}
+
 #define CODE                                                                   \
   class BaseA {                                                                \
   public:                                                                      \