-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[Clang] Fix static const member address reference #169251
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.
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
[Clang] Fix static const member address reference #169251
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
8eaade1 to
6f6245e
Compare
6f6245e to
0977736
Compare
|
@vgvassilev Would you please review for viability? Paulo's trying to fix #146956, which is a bug preventing |
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Paulo Rafael Feodrippe (pfeodrippe) ChangesFixes #146956 Used Copilot. Run all clang tests locally with ninja check-clang (everything that was passing in main is also passing here.) From #146956 The fix seems to be to ensure static data members with in-class initializers are materialized before use. # Unable to take the address of a static const member.
❯ clang-repl
clang-repl> struct foo{ static int const bar{ 5 }; };
clang-repl> int const *p{ &foo::bar };
clang-repl> #include <cstdio>
clang-repl> printf("%d\n", *p);
JIT session error: Symbols not found: [ _ZN3foo3barE ]
Segmentation fault (core dumped)
# Getting its value works fine.
❯ clang-repl
clang-repl> struct foo{ static int const bar{ 5 }; };
clang-repl> int const i{ foo::bar };
clang-repl> #include <cstdio>
clang-repl> printf("%d\n", i);
5
# LLVM 22 (head)Full diff: https://github.com/llvm/llvm-project/pull/169251.diff 6 Files Affected:
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index b33772919b8c8..9f3b7f686fe4e 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -3111,6 +3111,12 @@ static LValue EmitGlobalVarDeclLValue(CodeGenFunction &CGF,
return CGF.MakeAddrLValue(Addr, T, AlignmentSource::Decl);
}
+ // In incremental mode, ensure static data members with in-class initializers
+ // are materialized before use. This handles lvalue references that bypass
+ // constant evaluation (e.g., passing by reference to a function).
+ if (CGF.CGM.getLangOpts().IncrementalExtensions && VD->isStaticDataMember())
+ VD = CGF.CGM.materializeStaticDataMember(VD);
+
llvm::Value *V = CGF.CGM.GetAddrOfGlobalVar(VD);
if (VD->getTLSKind() != VarDecl::TLS_None)
diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
index 6407afc3d9447..2a97f509b4b9b 100644
--- a/clang/lib/CodeGen/CGExprConstant.cpp
+++ b/clang/lib/CodeGen/CGExprConstant.cpp
@@ -2243,6 +2243,10 @@ ConstantLValueEmitter::tryEmitBase(const APValue::LValueBase &base) {
}
if (const auto *VD = dyn_cast<VarDecl>(D)) {
+ // In incremental mode, ensure static data members with in-class
+ // initializers are materialized on first odr-use (e.g., &Foo::member).
+ if (CGM.getLangOpts().IncrementalExtensions && VD->isStaticDataMember())
+ VD = CGM.materializeStaticDataMember(VD);
// We can never refer to a variable with local storage.
if (!VD->hasLocalStorage()) {
if (VD->isFileVarDecl() || VD->hasExternalStorage())
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 645b78a599f89..eba02c980747e 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -6302,6 +6302,51 @@ CodeGenModule::getLLVMLinkageVarDefinition(const VarDecl *VD) {
return getLLVMLinkageForDeclarator(VD, Linkage);
}
+const VarDecl *CodeGenModule::materializeStaticDataMember(const VarDecl *VD) {
+ if (!VD)
+ return VD;
+
+ const VarDecl *DefinitionVD = VD->getDefinition();
+ if (!DefinitionVD)
+ DefinitionVD = VD;
+
+ // Only in-class static data members need materialization. Out-of-line
+ // definitions are emitted normally.
+ if (!DefinitionVD->isStaticDataMember() || DefinitionVD->isOutOfLine())
+ return DefinitionVD;
+
+ const VarDecl *InitVD = nullptr;
+ // Members without in-class initializers rely on out-of-class definitions.
+ if (!DefinitionVD->getAnyInitializer(InitVD) || !InitVD)
+ return DefinitionVD;
+
+ auto NeedsMaterialization = [](llvm::GlobalValue *GV) {
+ if (!GV)
+ return true;
+ if (GV->isDeclaration())
+ return true;
+ // Inline static members may be emitted as available_externally; upgrade
+ // to linkonce_odr so the JIT can resolve them.
+ if (auto *GVVar = llvm::dyn_cast<llvm::GlobalVariable>(GV))
+ return GVVar->hasAvailableExternallyLinkage();
+ return false;
+ };
+
+ GlobalDecl GD(InitVD);
+ StringRef MangledName = getMangledName(GD);
+ llvm::GlobalValue *GV = GetGlobalValue(MangledName);
+
+ if (NeedsMaterialization(GV)) {
+ EmitGlobalVarDefinition(InitVD, /*IsTentative=*/false);
+ GV = GetGlobalValue(MangledName);
+ if (auto *GVVar = llvm::dyn_cast_or_null<llvm::GlobalVariable>(GV))
+ if (GVVar->hasAvailableExternallyLinkage())
+ GVVar->setLinkage(llvm::GlobalValue::LinkOnceODRLinkage);
+ }
+
+ return InitVD;
+}
+
/// Replace the uses of a function that was declared with a non-proto type.
/// We want to silently drop extra arguments from call sites
static void replaceUsesOfNonProtoConstant(llvm::Constant *old,
diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h
index a253bcda2d06c..1ea582c03a8c6 100644
--- a/clang/lib/CodeGen/CodeGenModule.h
+++ b/clang/lib/CodeGen/CodeGenModule.h
@@ -1839,6 +1839,14 @@ class CodeGenModule : public CodeGenTypeCache {
return TrapReasonBuilder(&getDiags(), DiagID, TR);
}
+ /// Materialize a static data member with an in-class initializer on demand.
+ ///
+ /// In incremental contexts (e.g. clang-repl) a class can be defined in an
+ /// earlier partial translation unit, while the first odr-use (such as taking
+ /// the address) happens later. Ensure we emit a usable definition so the JIT
+ /// can resolve the symbol.
+ const VarDecl *materializeStaticDataMember(const VarDecl *VD);
+
private:
bool shouldDropDLLAttribute(const Decl *D, const llvm::GlobalValue *GV) const;
diff --git a/clang/test/Interpreter/static-const-member.cpp b/clang/test/Interpreter/static-const-member.cpp
new file mode 100644
index 0000000000000..3ba52679305a4
--- /dev/null
+++ b/clang/test/Interpreter/static-const-member.cpp
@@ -0,0 +1,59 @@
+// RUN: cat %s | clang-repl | FileCheck %s
+// Tests for static const member materialization in clang-repl.
+// See https://github.com/llvm/llvm-project/issues/146956
+
+extern "C" int printf(const char*, ...);
+
+struct Foo { static int const bar { 5 }; static int const baz { 10 }; };
+
+// Test 1: Taking address of static const member
+int const * p = &Foo::bar;
+printf("Address test: %d\n", *p);
+// CHECK: Address test: 5
+
+// Test 2: Multiple static const members in same class
+int const * q = &Foo::baz;
+printf("Second member test: %d\n", *q);
+// CHECK: Second member test: 10
+
+// Test 3: static constexpr member (variant of in-class init)
+struct Qux { static constexpr int val = 99; };
+int const *p3 = &Qux::val;
+printf("Constexpr test: %d\n", *p3);
+// CHECK: Constexpr test: 99
+
+// Test 4: Passing static const member by reference (exercises CGExpr.cpp path)
+// NOTE: Uses a separate struct to ensure this is the first odr-use of RefOnly::val
+struct RefOnly { static int const val { 77 }; };
+void useRef(int const &x) { printf("Ref test: %d\n", x); }
+useRef(RefOnly::val);
+// CHECK: Ref test: 77
+
+// ============================================================================
+// Negative tests - cases that should NOT trigger materialization
+// ============================================================================
+
+// Test 5: Out-of-class definition (no need to materialize - already defined)
+struct OutOfLine { static const int value; };
+const int OutOfLine::value = 23;
+int const *p5 = &OutOfLine::value;
+printf("Out-of-line test: %d\n", *p5);
+// CHECK: Out-of-line test: 23
+
+// Test 6: Non-const static member (normal code gen path)
+struct NonConst { static int value; };
+int NonConst::value = 42;
+int *p6 = &NonConst::value;
+printf("Non-const test: %d\n", *p6);
+// CHECK: Non-const test: 42
+
+// ============================================================================
+// Edge case tests
+// ============================================================================
+
+// Test 7: Repeated address-of reuses same definition
+int const *p7 = &Foo::bar;
+printf("Reuse test: %d\n", (*p == *p7) ? 1 : 0);
+// CHECK: Reuse test: 1
+
+%quit
diff --git a/clang/unittests/Interpreter/InterpreterTest.cpp b/clang/unittests/Interpreter/InterpreterTest.cpp
index 9ff9092524d21..341e61563e6bf 100644
--- a/clang/unittests/Interpreter/InterpreterTest.cpp
+++ b/clang/unittests/Interpreter/InterpreterTest.cpp
@@ -443,4 +443,18 @@ TEST_F(InterpreterTest, TranslationUnit_CanonicalDecl) {
sema.getASTContext().getTranslationUnitDecl()->getCanonicalDecl());
}
+TEST_F(InterpreterTest, StaticConstMemberAddress) {
+ std::unique_ptr<Interpreter> Interp = createInterpreter();
+
+ llvm::cantFail(
+ Interp->ParseAndExecute("struct Foo { static int const bar { 5 }; };"));
+
+ Value V;
+ llvm::cantFail(Interp->ParseAndExecute("int const * p = &Foo::bar; *p", &V));
+
+ ASSERT_TRUE(V.isValid());
+ ASSERT_TRUE(V.hasValue());
+ EXPECT_EQ(V.getInt(), 5);
+}
+
} // end anonymous namespace
|
|
@pfeodrippe I had a conversation with Vassil about this and cleared up that it's not something we can fix, since it's based on invalid C++ usage from the start. We can work around it in jank, though. We can close this PR! |
Nice nice, thanks / Closing |
Fixes #146956
Used Copilot. Run all clang tests locally with ninja check-clang (everything that was passing in main is also passing here.)
From #146956
clang::Interpretercannot get the address of a static const member`, we can see the issue below.The fix, only applied in incremental mode (clang-repl / Interpreter), seems to be to ensure static data members with in-class initializers are materialized before use.