[Clang][CodeGen] Do not set inbounds flag in EmitMemberDataPointerAddress when the base pointer is null
#130952
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen Author: Yingwei Zheng (dtcxzyw) ChangesSee also #130734 for the original motivation. This pattern ( llvm-project/llvm/include/llvm/IR/SymbolTableListTraits.h Lines 77 to 87 in 1d89d7d https://github.com/nodejs/node/blob/a2a53cb728ec5f776ac16879ce0f480a8e838320/src/util-inl.h#L134-L137 https://github.com/search?q=*%29nullptr-%3E*&type=code Full diff: https://github.com/llvm/llvm-project/pull/130952.diff 4 Files Affected:
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index b145da0f0ec09..b2eaea683ebe7 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -872,9 +872,14 @@ llvm::Value *ItaniumCXXABI::EmitMemberDataPointerAddress(
CGBuilderTy &Builder = CGF.Builder;
- // Apply the offset, which we assume is non-null.
- return Builder.CreateInBoundsGEP(CGF.Int8Ty, Base.emitRawPointer(CGF), MemPtr,
- "memptr.offset");
+ // Apply the offset.
+ // Specially, we don't add inbounds flags if the base pointer is a constant
+ // null. This is a workaround for old-style container_of macros.
+ llvm::Value *BaseAddr = Base.emitRawPointer(CGF);
+ return Builder.CreateGEP(CGF.Int8Ty, BaseAddr, MemPtr, "memptr.offset",
+ isa<llvm::ConstantPointerNull>(BaseAddr)
+ ? llvm::GEPNoWrapFlags::none()
+ : llvm::GEPNoWrapFlags::inBounds());
}
// See if it's possible to return a constant signed pointer.
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 4b55fc3f17bd7..3501b2b2fdca8 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -3265,9 +3265,13 @@ llvm::Value *MicrosoftCXXABI::EmitMemberDataPointerAddress(
Addr = Base.emitRawPointer(CGF);
}
- // Apply the offset, which we assume is non-null.
- return Builder.CreateInBoundsGEP(CGF.Int8Ty, Addr, FieldOffset,
- "memptr.offset");
+ // Apply the offset.
+ // Specially, we don't add inbounds flags if the base pointer is a constant
+ // null. This is a workaround for old-style container_of macros.
+ return Builder.CreateGEP(CGF.Int8Ty, Addr, FieldOffset, "memptr.offset",
+ isa<llvm::ConstantPointerNull>(Addr)
+ ? llvm::GEPNoWrapFlags::none()
+ : llvm::GEPNoWrapFlags::inBounds());
}
llvm::Value *
diff --git a/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp b/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
index fc8a31e0350e5..fe4cab164249f 100644
--- a/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
+++ b/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
@@ -964,3 +964,24 @@ static_assert(sizeof(b::d) == 16, "");
static_assert(sizeof(void (a<int>::*)()) == 16, "");
#endif
}
+
+namespace ContainerOf {
+ using size_t = unsigned long long;
+
+ struct List {
+ int data;
+ };
+
+ struct Node {
+ int data;
+ struct List list1;
+ struct List list2;
+ };
+
+ // CHECK-LABEL: define{{.*}} ptr @"?getOwner@ContainerOf@@
+ // CHECK: %memptr.offset = getelementptr i8, ptr null, {{.*}}
+ Node* getOwner(List *list, List Node::*member) {
+ size_t offset = reinterpret_cast<size_t>(&((Node*)nullptr->*member));
+ return reinterpret_cast<Node*>(reinterpret_cast<char*>(list) - offset);
+ }
+}
diff --git a/clang/test/CodeGenCXX/pointers-to-data-members.cpp b/clang/test/CodeGenCXX/pointers-to-data-members.cpp
index cf1d6c018409d..2ee6c65cf167d 100644
--- a/clang/test/CodeGenCXX/pointers-to-data-members.cpp
+++ b/clang/test/CodeGenCXX/pointers-to-data-members.cpp
@@ -265,3 +265,24 @@ namespace PR47864 {
struct D : B { int m; };
auto x = (int B::*)&D::m;
}
+
+namespace ContainerOf {
+ using size_t = unsigned long long;
+
+ struct List {
+ int data;
+ };
+
+ struct Node {
+ int data;
+ struct List list1;
+ struct List list2;
+ };
+
+ // CHECK-LABEL: define{{.*}} ptr @_ZN11ContainerOf8getOwnerEPNS_4ListEMNS_4NodeES0_
+ // CHECK: %memptr.offset = getelementptr i8, ptr null, i64 {{.*}}
+ Node* getOwner(List *list, List Node::*member) {
+ size_t offset = reinterpret_cast<size_t>(&((Node*)nullptr->*member));
+ return reinterpret_cast<Node*>(reinterpret_cast<char*>(list) - offset);
+ }
+}
|
|
@llvm/pr-subscribers-clang Author: Yingwei Zheng (dtcxzyw) ChangesSee also #130734 for the original motivation. This pattern ( llvm-project/llvm/include/llvm/IR/SymbolTableListTraits.h Lines 77 to 87 in 1d89d7d https://github.com/nodejs/node/blob/a2a53cb728ec5f776ac16879ce0f480a8e838320/src/util-inl.h#L134-L137 https://github.com/search?q=*%29nullptr-%3E*&type=code Full diff: https://github.com/llvm/llvm-project/pull/130952.diff 4 Files Affected:
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index b145da0f0ec09..b2eaea683ebe7 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -872,9 +872,14 @@ llvm::Value *ItaniumCXXABI::EmitMemberDataPointerAddress(
CGBuilderTy &Builder = CGF.Builder;
- // Apply the offset, which we assume is non-null.
- return Builder.CreateInBoundsGEP(CGF.Int8Ty, Base.emitRawPointer(CGF), MemPtr,
- "memptr.offset");
+ // Apply the offset.
+ // Specially, we don't add inbounds flags if the base pointer is a constant
+ // null. This is a workaround for old-style container_of macros.
+ llvm::Value *BaseAddr = Base.emitRawPointer(CGF);
+ return Builder.CreateGEP(CGF.Int8Ty, BaseAddr, MemPtr, "memptr.offset",
+ isa<llvm::ConstantPointerNull>(BaseAddr)
+ ? llvm::GEPNoWrapFlags::none()
+ : llvm::GEPNoWrapFlags::inBounds());
}
// See if it's possible to return a constant signed pointer.
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 4b55fc3f17bd7..3501b2b2fdca8 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -3265,9 +3265,13 @@ llvm::Value *MicrosoftCXXABI::EmitMemberDataPointerAddress(
Addr = Base.emitRawPointer(CGF);
}
- // Apply the offset, which we assume is non-null.
- return Builder.CreateInBoundsGEP(CGF.Int8Ty, Addr, FieldOffset,
- "memptr.offset");
+ // Apply the offset.
+ // Specially, we don't add inbounds flags if the base pointer is a constant
+ // null. This is a workaround for old-style container_of macros.
+ return Builder.CreateGEP(CGF.Int8Ty, Addr, FieldOffset, "memptr.offset",
+ isa<llvm::ConstantPointerNull>(Addr)
+ ? llvm::GEPNoWrapFlags::none()
+ : llvm::GEPNoWrapFlags::inBounds());
}
llvm::Value *
diff --git a/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp b/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
index fc8a31e0350e5..fe4cab164249f 100644
--- a/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
+++ b/clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
@@ -964,3 +964,24 @@ static_assert(sizeof(b::d) == 16, "");
static_assert(sizeof(void (a<int>::*)()) == 16, "");
#endif
}
+
+namespace ContainerOf {
+ using size_t = unsigned long long;
+
+ struct List {
+ int data;
+ };
+
+ struct Node {
+ int data;
+ struct List list1;
+ struct List list2;
+ };
+
+ // CHECK-LABEL: define{{.*}} ptr @"?getOwner@ContainerOf@@
+ // CHECK: %memptr.offset = getelementptr i8, ptr null, {{.*}}
+ Node* getOwner(List *list, List Node::*member) {
+ size_t offset = reinterpret_cast<size_t>(&((Node*)nullptr->*member));
+ return reinterpret_cast<Node*>(reinterpret_cast<char*>(list) - offset);
+ }
+}
diff --git a/clang/test/CodeGenCXX/pointers-to-data-members.cpp b/clang/test/CodeGenCXX/pointers-to-data-members.cpp
index cf1d6c018409d..2ee6c65cf167d 100644
--- a/clang/test/CodeGenCXX/pointers-to-data-members.cpp
+++ b/clang/test/CodeGenCXX/pointers-to-data-members.cpp
@@ -265,3 +265,24 @@ namespace PR47864 {
struct D : B { int m; };
auto x = (int B::*)&D::m;
}
+
+namespace ContainerOf {
+ using size_t = unsigned long long;
+
+ struct List {
+ int data;
+ };
+
+ struct Node {
+ int data;
+ struct List list1;
+ struct List list2;
+ };
+
+ // CHECK-LABEL: define{{.*}} ptr @_ZN11ContainerOf8getOwnerEPNS_4ListEMNS_4NodeES0_
+ // CHECK: %memptr.offset = getelementptr i8, ptr null, i64 {{.*}}
+ Node* getOwner(List *list, List Node::*member) {
+ size_t offset = reinterpret_cast<size_t>(&((Node*)nullptr->*member));
+ return reinterpret_cast<Node*>(reinterpret_cast<char*>(list) - offset);
+ }
+}
|
|
Same concerns about null pointer checking as #130734. |
dtcxzyw
force-pushed
the
remove-inbounds-member-pointer
branch
from
824a5e7 to
425801d
Compare
efriedma-quic
approved these changes
Apr 10, 2025
| Object = EmitCXXMemberDataPointerAddress(E, Object, Ptr, | ||
| Adjustment.Ptr.MPT); | ||
| Object = EmitCXXMemberDataPointerAddress( | ||
| E, Object, Ptr, Adjustment.Ptr.MPT, /*IsInBounds=*/true); |
There was a problem hiding this comment.
This sent me down a really deep rabbit-hole to figure out when this code actually runs... apparently, in C++98 mode, we delay creating temporaries for things that would be xvalues in newer standard versions, and then we try to fix things up later using skipRValueSubobjectAdjustments(). We don't have an in-tree tests for this particular codepath. The whole thing is really ugly, and we should really come up with a better solution...
In any case, the base object should be an alloca here, so inbounds should be fine.
dtcxzyw
force-pushed
the
remove-inbounds-member-pointer
branch
from
425801d to
e71ca8b
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/3246 Here is the relevant piece of the build log for the reference |
dtcxzyw
added a commit
that referenced
this pull request
May 30, 2025
This patch fixes the "dereferencing null" UB. Unfortunately, C++ doesn't provide an inverse operation for `p->*pmf`. See also #130952.
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 30, 2025
This patch fixes the "dereferencing null" UB. Unfortunately, C++ doesn't provide an inverse operation for `p->*pmf`. See also llvm/llvm-project#130952.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See also #130734 for the original motivation.
This pattern (
container_of) is also widely used by real-world programs.Examples:
llvm-project/llvm/include/llvm/IR/SymbolTableListTraits.h
Lines 77 to 87 in 1d89d7d
https://github.com/nodejs/node/blob/a2a53cb728ec5f776ac16879ce0f480a8e838320/src/util-inl.h#L134-L137
https://github.com/search?q=*%29nullptr-%3E*&type=code