Do not instantiate non-template static data members in GetVariableOffset#1067
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1067 +/- ##
==========================================
+ Coverage 87.01% 87.07% +0.05%
==========================================
Files 23 23
Lines 6069 6073 +4
==========================================
+ Hits 5281 5288 +7
+ Misses 788 785 -3
🚀 New features to boost your workflow:
|
|
clang-tidy review says "All clean, LGTM! 👍" |
378ef0c to
20d4b64
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
| VD = Def; | ||
| } | ||
| if (VD->hasInit() && | ||
| (VD->isConstexpr() || VD->getType().isConstQualified())) { |
There was a problem hiding this comment.
The constexpr branch is critical for performance. Can the new code come after it?
There was a problem hiding this comment.
updated: the constexpr branch hasn't moved, and its hot path runs no new code: the guarded block sits inside if (!VD->hasInit()), and when there's no initializer the constexpr branch has nothing to evaluate yet — the instantiation exists to materialize the initializer that branch consumes, so it can't come after it. But the performance concern did point at a real win: upstream ran InstantiateVariableDefinition unconditionally (transaction push + Sema instantiation machinery). Reworked so an already-parsed out-of-line definition is preferred first — std::partial_ordering::less now reaches the constexpr evaluation with no Sema work at all — and the pattern-guarded instantiation is only the fallback for genuinely templated statics.
|
CI triage for the two red jobs — neither traces to this diff:
All cling/clang-repl functional suites pass. |
Cpp::GetVariableOffset called Sema::InstantiateVariableDefinition on any static data member with no initializer and no resolved address. That Sema entry point requires a variable instantiated from a template: for a non-template class's static member (std::partial_ordering::less is the in-the-wild shape), getTemplateInstantiationPattern() is null, the assert is compiled out in release builds, and clang dereferences null. Resolve an already-parsed out-of-line definition first - it may carry the initializer the constexpr fast path evaluates, with no Sema work at all - and only fall back to InstantiateVariableDefinition when an instantiation pattern actually exists. Co-developed-with-the-help-of: Claude Code (Fable 5, human in the loop)
20d4b64 to
cf7c88a
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
Cpp::GetVariableOffsetcallsSema::InstantiateVariableDefinitionon any static data member whose address is not found and whose declaration has no initializer. That Sema entry point requires a templated variable: for a non-template class's static member,getTemplateInstantiationPattern()returns null, the assert is compiled out in release builds, and clang dereferences null (this == nullptrinsideVarDecl::getDefinition).The libstdc++
<compare>ordering types hit the exact triple condition — declared in-class without an initializer, defined out-of-line asinline constexpr(so no exported symbol to find), not yet JIT-emitted. Minimal repro on a release build:Per review (keeping the constexpr fast path cheap): an already-parsed out-of-line definition is now resolved first — it may carry the initializer the constexpr evaluation consumes, with no Sema work at all — and the pattern-guarded
InstantiateVariableDefinitionis only the fallback for genuinely templated statics.getDefinition()is null-checked in both arms; the existing fallthrough then does the right thing for these variables (discardable-ODR linkage → codegen emission → real address). The regression test crashes at exactly this stack without the fix and passes with it.🤖 Done with the help of Claude Code (Fable 5, human in the loop)