From d3bd5d302c8d766f8b5a483bfc9c0d2b84d5f127 Mon Sep 17 00:00:00 2001 From: Emery Conrad Date: Wed, 15 Jul 2026 12:30:02 -0500 Subject: [PATCH] Emit discardable-ODR variables via odr-use, not UsedAttr, in GetVariableOffset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ForceCodeGen forces deferred emission by planting a permanent __attribute__((used)) on the decl; every global emitted that way gets a WeakTrackingVH in codegen's llvm.used list. When such a global is deleted before emitUsed runs (ORC freeing a materialized PTU's IR), the handle nulls and release-built clang dereferences it unchecked — a process crash that accumulates with interpreter state rather than tracing to any one declaration (large reflection sweeps died; per-type bisection never converged, so the crash itself has no compact unit repro — root-caused via gdb). GetVariableOffset now emits GVA_DiscardableODR variables (the inline/constexpr class the crash traced to) by declaring an external-linkage odr-use of the qualified name instead: the definition flows through the regular deferred-decl path and leaves no used-list residue, no permanent attribute. Everything else keeps the UsedAttr path, deliberately: - Internal-linkage variables cannot be odr-used from a later PTU (the module-local symbol duplicates or goes missing); getting this wrong broke VariableReflection_GetVariableOffset (static int S) and cppyy's Lifeline::count lookup. - Available-externally definitions would not be emitted by a mere reference. - Template-specialization and anonymous/lambda spellings do not reliably round-trip as source (a printed LLONG_MIN non-type argument re-parses as an overflowing literal), and a parse-failing declaration poisons the incremental interpreter. An interpreter-level alternative — Undo(1) on parse failure to drop the failed PTU — was evaluated and rejected: cppyy depends on failed parses leaving side-effect declarations (an explicit-instantiation probe of a declared-but-undefined template is expected to fail and leave the specialization decl behind; test_templates test32). The dummy-name counter is deliberately plain, matching gWrapperSerial: the interpreter API is caller-serialized. ForceCodeGen's function path (GetFunctionAddress) still uses UsedAttr — same theoretical hazard, never observed. The test pins the mechanism: after an offset query on a discardable-ODR static, the decl carries no UsedAttr and later PTU modules carry no llvm.used, with a sweep-shaped regression net over interleaved absorbed parse failures. Co-developed-with-the-help-of: Claude Code (Fable 5, human in the loop) --- lib/CppInterOp/CppInterOp.cpp | 97 +++++++- lib/CppInterOp/InterpreterInfo.h | 8 +- .../CppInterOp/VariableReflectionTest.cpp | 215 +++++++++++++++++- 3 files changed, 316 insertions(+), 4 deletions(-) diff --git a/lib/CppInterOp/CppInterOp.cpp b/lib/CppInterOp/CppInterOp.cpp index 61abcb363..79855b977 100644 --- a/lib/CppInterOp/CppInterOp.cpp +++ b/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 @@ static void ForceCodeGen(Decl* D, compat::Interpreter& I) { #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 @@ intptr_t GetVariableOffset(compat::Interpreter& I, Decl* D, } 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 @@ class clangSilent { }; } // 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". diff --git a/lib/CppInterOp/InterpreterInfo.h b/lib/CppInterOp/InterpreterInfo.h index a61e5c744..f8869b767 100644 --- a/lib/CppInterOp/InterpreterInfo.h +++ b/lib/CppInterOp/InterpreterInfo.h @@ -46,6 +46,10 @@ 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 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 ArgvStrs = {}) @@ -53,7 +57,8 @@ struct InterpreterInfo { 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; } diff --git a/unittests/CppInterOp/VariableReflectionTest.cpp b/unittests/CppInterOp/VariableReflectionTest.cpp index ac51292bd..026e6338b 100644 --- a/unittests/CppInterOp/VariableReflectionTest.cpp +++ b/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 +#include #include +#include +#include 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(var)->hasAttr()); +#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 use" #n " = Sweep" #n "::v" #n ";" \ + } + constexpr std::array 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 struct TmplStatic { + inline static int member = 7; + }; + )"); + Cpp::DeclRef tmpl = Cpp::GetNamed("TmplStatic"); + EXPECT_TRUE(tmpl); + ASTContext& C = Interp->getCI()->getASTContext(); + std::vector 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(member)->hasAttr()); + + // 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(anon_member)->hasAttr()); +#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(link_member)->hasAttr()); +} + +#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: \