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

[conrade-ctc]

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 (e.g. ORC freeing a materialized PTU's IR), the handle nulls and release-built clang dereferences it unchecked — SIGSEGV in ConstantExpr::getPointerBitCastOrAddrSpaceCast under CodeGenModule::Release() of a later incremental parse. The failure is cumulative interpreter state, not a culprit type: it only manifests after enough force-emitted statics plus JIT materialization cycles (we hit it on large reflection sweeps; per-type bisection never converged), so the regression test asserts the mechanism instead — no UsedAttr planted, no llvm.used residue in later PTU modules — plus a sweep-shaped smoke loop.

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: the definition flows through the regular deferred-decl path and leaves no used-list residue and no permanent attribute. Everything else deliberately stays on the UsedAttr path:

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

Also evaluated and rejected: Interpreter::Undo(1) on parse failure to drop failed PTUs — cppyy depends on failed parses leaving side-effect declarations (test_templates test32's explicit-instantiation probe).

Concurrency: the dummy-name counter is deliberately a plain static unsigned, matching the file's existing gWrapperSerial idiom. The interpreter API is caller-serialized (the incremental parse either route drives is not thread-safe, and the file's only sync primitive is the process-init once_flag); an atomic here would advertise a guarantee the surrounding function doesn't have. Worst case if raced anyway: a duplicate dummy name fails the Declare and the caller falls back to ForceCodeGen.

Known residual, unchanged here: ForceCodeGen's function path (GetFunctionAddress) still uses UsedAttr — same theoretical hazard, never observed.

Independent of #1067 (both touch GetVariableOffset; whichever lands second has a trivial test-file conflict).

🤖 Done with the help of Claude Code (Fable 5, human in the loop)

[conrade-ctc]

cc @vgvassilev @rathorey-ctc

[codecov]

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.04%. Comparing base (3103be9) to head (79d2fdb).

Files with missing lines	Patch %	Lines
lib/CppInterOp/CppInterOp.cpp	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1068      +/-   ##
==========================================
+ Coverage   87.01%   87.04%   +0.03%     
==========================================
  Files          23       23              
  Lines        6069     6084      +15     
==========================================
+ Hits         5281     5296      +15     
  Misses        788      788              

Files with missing lines	Coverage Δ
lib/CppInterOp/CppInterOp.cpp	89.55% <94.11%> (+0.05%)	⬆️

Files with missing lines	Coverage Δ
lib/CppInterOp/CppInterOp.cpp	89.55% <94.11%> (+0.05%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 11. Check the log or trigger a new build to see more.

[vgvassilev]

This entire approach of adding these attributes to force emission is sketchy to say the least. Can you ask Claude what alternatives we have including perhaps minimal reasonable modifications to clang.

[conrade-ctc]

This entire approach of adding these attributes to force emission is sketchy to say the least. Can you ask Claude what alternatives we have including perhaps minimal reasonable modifications to clang.

ya, i didn't love this at all either... felt pretty gross to me. let me iterate a bit and see what we might do upstream to address it.

[github-actions]

clang-tidy review says "All clean, LGTM! 👍"

[vgvassilev]

This entire approach of adding these attributes to force emission is sketchy to say the least. Can you ask Claude what alternatives we have including perhaps minimal reasonable modifications to clang.

ya, i didn't love this at all either... felt pretty gross to me. let me iterate a bit and see what we might do upstream to address it.

I did not mean your patch but the overall logic in this area.

[conrade-ctc]

CI triage for the three red jobs — none traces to this diff:

• osx26-x86-clang-llvm21-cppyy: runner network flake — CppInterOp's own tests passed 7/7, then the cppyy-stack step died on git clone: Could not resolve host: github.com. A rerun should clear it (I don't have rerun rights here).
• ubu24-x86-gcc14-cling-llvm20-root: pre-existing ROOT-configure ninja failure (Box.h defined as an output multiple times), same on other open PRs.
• ubu24-x86-gcc14-cling-llvm20-asan: pre-existing intermittent LSAN exit-leak, byte-identical (269,234 bytes / 231 allocations) to a failure on unchanged main (https://github.com/compiler-research/CppInterOp/actions/runs/29124927678); details in the sibling comment on Do not instantiate non-template static data members in GetVariableOffset #1067.

The six cling compile failures from the first push are fixed (PTU white-box assertions now gated to clang-repl builds).

[conrade-ctc]

Asked Claude to survey the design space; here's the grounded version (file/line refs against release/21.x).

Why the current approach is sketchy (agreed): ForceCodeGen mutates the AST as a side effect of a reflection query — UsedAttr::CreateImplicit is visible to every later semantic check — and the attr drags the emitted global into llvm.used, a std::vector<llvm::WeakTrackingVH>. Incremental PTUs can delete such a global, the handle goes null, and emitUsed does cast<llvm::Constant>(&*List[i]) with no null check (CodeGenModule.cpp:~3203) — that's the crash this PR routes around. On top, the generated module's internal-linkage globals get rewritten to weak.

What CppInterOp can do without touching clang: essentially this PR — a source-level odr-use where the qualified name round-trips (falling back to UsedAttr for template-specialization spellings and anonymous scopes). The semantically honest AST-level version — Sema::MarkVariableReferenced + HandleTopLevelDecl — dead-ends because ASTContext::DeclMustBeEmitted doesn't consult isUsed() for variables (the FIXME in ForceCodeGen says exactly this), so codegen still defers the definition.

Minimal clang modification #1 (tiny, worth doing regardless): null-skip deleted globals in emitUsed's loop. In incremental mode a used global can legitimately be destroyed between PTUs; today that's a release-build null deref. ~3 lines, protects every existing embedder of the UsedAttr pattern — including this PR's own fallback branch — and needs no design discussion. I'll send that patch independently.

Minimal clang modification #2 (the principled end-state): an emission API on clang::Interpreter — e.g. emitGlobalDecl(GlobalDecl). The pieces already exist: CodeGenerator::GetAddrOfGlobal(GD, /*isForDefinition=*/true) is public (ModuleBuilder.h:98) and schedules the deferred definition (GetOrCreateLLVMGlobal promotes the DeferredDecls entry, which moveLazyEmissionStates explicitly carries across incremental modules — CodeGenModule.cpp:8111); all that's missing is that Interpreter::getCodeGen() is private, so embedders can't reach it. Such an API would emit with natural linkonce_odr linkage — no attribute, no llvm.used residue, no name round-trip, works for the template/anonymous shapes this PR has to punt on — and would let ForceCodeGen's whole UsedAttr + Parse("") + linkage-rewrite dance be deleted.

Suggested sequencing: land this PR now as the crash fix — it helps every clang version that exists today, and CppInterOp's supported-version window means the eventual cleanup is a version gate rather than a revert (prefer emitGlobalDecl under #if CLANG_VERSION_MAJOR >= N once the API lands, keep this mechanism as the legacy branch for older majors). The emitUsed null-skip goes upstream immediately on its own; emitGlobalDecl goes through design review on its own cycle. Happy to draft both clang patches.

[vgvassilev]

Sounds like a good plan actually.