Emit discardable-ODR variables via odr-use instead of UsedAttr in GetVariableOffset#1068
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1068 +/- ##
==========================================
+ Coverage 87.07% 87.11% +0.04%
==========================================
Files 23 23
Lines 6073 6109 +36
==========================================
+ Hits 5288 5322 +34
- Misses 785 787 +2
🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
916eeb4 to
79d2fdb
Compare
|
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. |
|
clang-tidy review says "All clean, LGTM! 👍" |
I did not mean your patch but the overall logic in this area. |
|
CI triage for the three red jobs — none traces to this diff:
The six cling compile failures from the first push are fixed (PTU white-box assertions now gated to clang-repl builds). |
|
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): What CppInterOp can do without touching clang: essentially this PR — a source-level odr-use where the qualified name round-trips (falling back to Minimal clang modification #1 (tiny, worth doing regardless): null-skip deleted globals in Minimal clang modification #2 (the principled end-state): an emission API on 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 |
|
Sounds like a good plan actually. |
79d2fdb to
8aac73a
Compare
|
Rebased onto main to clear the conflict with #1067 — its definition-first structure is preserved, with the odr-use dispatch layered on top of the discardable-linkage branch; both PRs' tests kept. Gates green locally (check-cppinterop, both Step 2 of the plan is also up: llvm/llvm-project#210959 null-skips deleted globals in @vgvassilev — ready for another look. CI note: the three emscripten job failures are infra — micromamba env setup got an HTTP 500 from prefix.dev fetching repodata, before anything was compiled. They should clear on a rerun (I lack rerun rights here). The root job is the known pre-existing configure breakage. |
| llvm::raw_string_ostream OS(name); | ||
| VD->printQualifiedName(OS); | ||
| } | ||
| if (name.find("(anonymous ") != std::string::npos || |
There was a problem hiding this comment.
We should not do string comparisons but we should use the relevant api to get the same information from the ast/sema.
There was a problem hiding this comment.
Done in 5f96515 — the string sniffing is gone. The guard now asks the AST directly: bail out for declarations without an IdentifierInfo or that are VarTemplateSpecializationDecls, then walk the DeclContext chain rejecting anonymous namespaces, unnamed/lambda records, ClassTemplateSpecializationDecl scopes, and anything else that isn't a named namespace/record (linkage-spec/export blocks are skipped as transparent). printQualifiedName is now only used to spell the odr-use for names the walk has proven spellable. Added test coverage for the anonymous-namespace bailout and the linkage-spec passthrough.
|
clang-tidy review says "All clean, LGTM! 👍" |
8aac73a to
5f96515
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
5f96515 to
0b6e999
Compare
|
CI triage on the last run, all addressed or external:
|
| static unsigned Counter = 0; | ||
| std::string code = "namespace __cppinterop_odr_use { const void* __v" + | ||
| std::to_string(Counter++) + | ||
| " = (const void*)__builtin_addressof(::" + name + "); }"; |
There was a problem hiding this comment.
Can we avoid that string manipulation, too? This leads to memory hoarding on long sessions... Perhaps we should just do what __builtin_addressof would do by calling it directly in Sema if possible...
There was a problem hiding this comment.
Done in e11ca91 — the source-text mechanism is gone entirely. The odr-use anchor is now built directly in Sema: BuildDeclRefExpr + CreateBuiltinUnaryOp(UO_AddrOf) (the AST parsing &var would produce), stored into a synthesized external-linkage const void* dummy at TU scope whose initializer conversion Sema checks, then handed to codegen the way ForceCodeGen already does (HandleTopLevelDecl + empty-parse flush). No per-query source buffers or string assembly beyond the dummy's identifier.
Two knock-on wins:
- Template-specialization members now take the odr-use route as well — no spelling round-trip exists to fail, and the caller has already instantiated the definition (Do not instantiate non-template static data members in GetVariableOffset #1067's shape). Test expectations updated accordingly.
- A new definition-exists guard bails to the UsedAttr fallback for declared-but-never-defined variables, so the dummy can never carry an unresolvable reference into the JIT. That reference was what the emscripten lanes were dying on — a failed wasm module load poisons process-global dynamic-linking state for everything after it (filed as [wasm] Failed forced emission of a never-defined variable poisons all subsequent incremental module loads #1071; the wasm skip on the regression test stays since the scenario it pins is native-ORC mechanics).
|
clang-tidy review says "All clean, LGTM! 👍" |
0b6e999 to
e11ca91
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
|
||
| // 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(), |
There was a problem hiding this comment.
This still hoards much less memory though. Can we add a fixme note with two lines annotating that we want to probably move that under a scratch area where these ast nodes get deallocated and add a ifdef llvm_version < 24 to remind ourselves to remove it once the other fix lands hopefully upstream?
There was a problem hiding this comment.
Done in d317206 — two FIXMEs added: one for moving the synthesized nodes under a deallocatable scratch area, one for removing the whole anchor mechanism once clang grows a direct emission API, with a #if CLANG_VERSION_MAJOR >= 24 + #warning tripwire so the clang-24 bump forces the revisit.
| return true; | ||
| #else // CLANG_REPL | ||
| I.getCI()->getASTConsumer().HandleTopLevelDecl(DeclGroupRef(Dummy)); | ||
| auto GeneratedPTU = I.Parse(""); |
There was a problem hiding this comment.
That’s worth another fixme pointing out that bug which forces parsing an empty string to reset the state in the compiler. I thought we had some raii to use to annotate these places.
There was a problem hiding this comment.
The RAII you're thinking of is compat::SynthesizingCodeRAII — under cling it's PushTransactionRAII (commit = emission), but its clang-repl variant is a stub with a TODO destructor (Compatibility.h). d317206 now uses it on both paths as the annotation, and the empty-parse flush carries a FIXME naming it as the thing the RAII's clang-repl destructor should eventually own.
e11ca91 to
d317206
Compare
| std::string probe = "template <> struct Sweep"; | ||
| probe += n; | ||
| probe += "<int>;"; |
There was a problem hiding this comment.
Is there any value in accumulating in this way? Why can't we do R"(...)?
There was a problem hiding this comment.
Sorry, this seems to be a claude default, and I didn't catch it, shouldn't be building the string this way. I'll update this and push, and refine CLAUDE.md!
There was a problem hiding this comment.
No value in it — that shape was appeasing clang-tidy's inefficient-concatenation check, at readability's expense. Reworked in the current head: the sweep bodies are raw strings (R"(struct Sweep@ { inline static int v@ = @; };)") with a tiny @→index substitution, no accumulation.
There was a problem hiding this comment.
I still don't get it -- can't we have a normal preprocessor macro with a stringify operation?
There was a problem hiding this comment.
Done in 60c19fe — the sweep sources are now assembled entirely at compile time: a SWEEP_CASE(n) macro stringifies the index (#n) into adjacent string literals, expanded into a constexpr std::array for 0–7. No runtime string building left in the test.
Same push also fixes what the osx cling lanes caught on the previous head: under cling, the odr-use marking in BuildDeclRefExpr can trigger an immediate static-member instantiation, and DeclCollector aborts if no transaction is open — the SynthesizingCodeRAII now opens before any Sema work in EmitVariableViaOdrUse (same pattern as the InstantiateVariableDefinition call site from #1067).
|
clang-tidy review says "All clean, LGTM! 👍" |
d317206 to
ec96b54
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
ec96b54 to
3864244
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
3864244 to
60c19fe
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
| // 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. | ||
| static unsigned Counter = 0; |
There was a problem hiding this comment.
That will hurt the multiple interpreters setup. We need to move that variable in the InterpreterInfo object.
There was a problem hiding this comment.
Hmmm, we must not have a test that exposes that. I'll try to add one, and fix. Thanks for all the iteration on this PR!
There was a problem hiding this comment.
Done in d3bd5d3 — OdrUseCounter now lives in InterpreterInfo (accessed via getInterpInfo(&I), and carried through the move ops since DeleteInterpreter's mid-deque erase relocates later entries). Added a regression test first, which fails with the static: VariableReflection_GetVariableOffset_OdrUseAnchorPerInterpreter pins that a fresh interpreter's first anchor is always its own v0, independent of sibling interpreters' (or the process's) query history.
Same push covers the anchor's error paths for the codecov patch gate: the Parse/Execute failure arms now share one consume-and-fail block, exercised by a new UnmaterializableDefinition test (a definition whose dynamic initializer needs an undefined symbol — parses fine, materialization fails, the query fails cleanly).
(Amended 58b9a90 → 97716d3: the new failure-path test provokes an intentional JIT error whose "JIT session error :" stderr line MSBuild promotes to a build error on the win-msvc lane — the test now skips on _WIN32; coverage comes from the linux lanes.)
(Amended 97716d3 → d3bd5d3: the same test now also skips on __EMSCRIPTEN__ — its intentional failed load poisons wasm dynamic linking, #1071 — and the Windows skip message no longer itself contains the error : token MSBuild scrapes.)
60c19fe to
58b9a90
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
58b9a90 to
97716d3
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
…bleOffset 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)
97716d3 to
d3bd5d3
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
ForceCodeGenforces deferred emission by planting a permanent__attribute__((used))on the decl. Every global emitted that way gets aWeakTrackingVHin codegen'sllvm.usedlist; when such a global is deleted beforeemitUsedruns (e.g. ORC freeing a materialized PTU's IR), the handle nulls and release-built clang dereferences it unchecked — SIGSEGV inConstantExpr::getPointerBitCastOrAddrSpaceCastunderCodeGenModule::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 — noUsedAttrplanted, nollvm.usedresidue in later PTU modules — plus a sweep-shaped smoke loop.GetVariableOffsetnow emitsGVA_DiscardableODRvariables (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 theUsedAttrpath:VariableReflection_GetVariableOffset(static int S) and cppyy'sLifeline::countlookup.LLONG_MINnon-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_templatestest32's explicit-instantiation probe).Concurrency: the dummy-name counter is deliberately a plain
static unsigned, matching the file's existinggWrapperSerialidiom. 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-initonce_flag); an atomic here would advertise a guarantee the surrounding function doesn't have. Worst case if raced anyway: a duplicate dummy name fails theDeclareand the caller falls back toForceCodeGen.Known residual, unchanged here:
ForceCodeGen's function path (GetFunctionAddress) still usesUsedAttr— 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)