Skip to content

Do not instantiate non-template static data members in GetVariableOffset #1067

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vgvassilev merged 1 commit into from
Jul 16, 2026
Merged

Do not instantiate non-template static data members in GetVariableOffset #1067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions lib/CppInterOp/CppInterOp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2779,9 +2779,21 @@ intptr_t GetVariableOffset(compat::Interpreter& I, Decl* D,
address = I.getAddressOfGlobal(GD);
if (!address) {
if (!VD->hasInit()) {
compat::SynthesizingCodeRAII RAII(&getInterp());
getSema().InstantiateVariableDefinition(SourceLocation(), VD);
VD = VD->getDefinition();
// The initializer feeding the constexpr fast path below may live on
// an already-parsed out-of-line definition (a non-template class's
// static data member, e.g. std::partial_ordering::less): prefer it,
// with no Sema work at all. Only a variable instantiated from a
// template can need Sema::InstantiateVariableDefinition — and
// without an instantiation pattern that call dereferences a null
// VarDecl in release builds.
if (VarDecl* Def = VD->getDefinition()) {
VD = Def;
} else if (VD->getTemplateInstantiationPattern()) {
compat::SynthesizingCodeRAII RAII(&getInterp());
getSema().InstantiateVariableDefinition(SourceLocation(), VD);
if (VarDecl* Inst = VD->getDefinition())
VD = Inst;
}
}
if (VD->hasInit() &&
(VD->isConstexpr() || VD->getType().isConstQualified())) {

@vgvassilev vgvassilev Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constexpr branch is critical for performance. Can the new code come after it?

@conrade-ctc conrade-ctc Jul 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
36 changes: 36 additions & 0 deletions unittests/CppInterOp/VariableReflectionTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,42 @@ TYPED_TEST(CPPINTEROP_TEST_MODE, VariableReflection_GetVariableOffset) {
EXPECT_TRUE(Cpp::GetVariableOffset(var));
}

TYPED_TEST(CPPINTEROP_TEST_MODE,
VariableReflection_GetVariableOffset_NonTemplateStaticNoInit) {
// A non-template class's static data member declared in-class and defined
// out-of-line reaches the no-initializer branch with no template
// instantiation pattern. Sema::InstantiateVariableDefinition requires a
// pattern and dereferences null without one (std::partial_ordering::less
// is the in-the-wild shape). Must not crash; must resolve the definition.
TestFixture::CreateInterpreter();
Cpp::Declare(R"(
struct NonTemplateStatic {
static const NonTemplateStatic less;
int value;
};
inline constexpr NonTemplateStatic NonTemplateStatic::less{-1};
)");
Cpp::DeclRef klass = Cpp::GetNamed("NonTemplateStatic");
EXPECT_TRUE(klass);
Cpp::DeclRef var = Cpp::GetNamed("less", klass);
EXPECT_TRUE(var);
EXPECT_TRUE(Cpp::GetVariableOffset(var));

// Declared and never defined: still no pattern and now no definition
// either — the query must fail cleanly, not crash.
Cpp::Declare(R"(
struct NeverDefined {
static const NeverDefined missing;
int value;
};
)");
Cpp::DeclRef klass2 = Cpp::GetNamed("NeverDefined");
EXPECT_TRUE(klass2);
Cpp::DeclRef var2 = Cpp::GetNamed("missing", klass2);
EXPECT_TRUE(var2);
EXPECT_FALSE(Cpp::GetVariableOffset(var2));
}

#define CODE \
class BaseA { \
public: \
Expand Down
Loading