[Clang][CodeGen] Do not set inbounds flag in `EmitMemberDataPointerAddress` when the base pointer is null by dtcxzyw · Pull Request #130952 · llvm/llvm-project (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation6 Commits3 Checks13 Files changed

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

[ Show hidden characters]({{ revealButtonHref }})

llvmbot added clang

Clang issues not falling into any other category

clang:codegen

IR generation bugs: mangling, exceptions, etc.

labels

Mar 12, 2025

@llvm/pr-subscribers-clang-codegen

Author: Yingwei Zheng (dtcxzyw)

Changes

See also #130734 for the original motivation.

This pattern (container_of) is also widely used by real-world programs.
Examples:

/// getListOwner - Return the object that owns this list. If this is a list
/// of instructions, it returns the BasicBlock that owns them.
ItemParentClass *getListOwner() {
size_t Offset = reinterpret_cast<size_t>(
&((ItemParentClass )nullptr->ItemParentClass::getSublistAccess(
static_cast<ValueSubClass *>(
nullptr))));
ListTy Anchor = static_cast<ListTy >(this);
return reinterpret_cast<ItemParentClass>(reinterpret_cast<char>(Anchor)-
Offset);
}

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:

(modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (+8-3)
(modified) clang/lib/CodeGen/MicrosoftCXXABI.cpp (+7-3)
(modified) clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp (+21)
(modified) clang/test/CodeGenCXX/pointers-to-data-members.cpp (+21)

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::*)()) == 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(&((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(&((Node*)nullptr->*member));
return reinterpret_cast<Node*>(reinterpret_cast<char*>(list) - offset);
} +}

@llvm/pr-subscribers-clang

Author: Yingwei Zheng (dtcxzyw)

Changes

See also #130734 for the original motivation.

This pattern (container_of) is also widely used by real-world programs.
Examples:

/// getListOwner - Return the object that owns this list. If this is a list
/// of instructions, it returns the BasicBlock that owns them.
ItemParentClass *getListOwner() {
size_t Offset = reinterpret_cast<size_t>(
&((ItemParentClass )nullptr->ItemParentClass::getSublistAccess(
static_cast<ValueSubClass *>(
nullptr))));
ListTy Anchor = static_cast<ListTy >(this);
return reinterpret_cast<ItemParentClass>(reinterpret_cast<char>(Anchor)-
Offset);
}

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:

(modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (+8-3)
(modified) clang/lib/CodeGen/MicrosoftCXXABI.cpp (+7-3)
(modified) clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp (+21)
(modified) clang/test/CodeGenCXX/pointers-to-data-members.cpp (+21)

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());

}

// 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());

}

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(&((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(&((Node*)nullptr->*member));
return reinterpret_cast<Node*>(reinterpret_cast<char*>(list) - offset);
} +}

Same concerns about null pointer checking as #130734.

Choose a reason for hiding this comment

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

LGTM

Object = EmitCXXMemberDataPointerAddress(E, Object, Ptr,
Adjustment.Ptr.MPT);
Object = EmitCXXMemberDataPointerAddress(
E, Object, Ptr, Adjustment.Ptr.MPT, /IsInBounds=/true);

Choose a reason for hiding this comment

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

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.

…` when the base pointer is null

dtcxzyw deleted the remove-inbounds-member-pointer branch

April 11, 2025 02:51

LLVM Buildbot has detected a new failure on builder openmp-offload-amdgpu-runtime-2 running on rocm-worker-hw-02 while building clang at step 5 "compile-openmp".

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

Step 5 (compile-openmp) failure: build (failure)
...
0.148 [390/130/177] Generating header ctype.h from /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/runtimes/../libc/include/ctype.yaml
0.148 [389/130/178] Generating /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/include/amdgcn-amd-amdhsa/llvm-libc-decls/locale.h
0.148 [389/129/179] Generating header locale.h from /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/runtimes/../libc/include/locale.yaml
0.149 [387/130/180] Generating /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/include/amdgcn-amd-amdhsa/llvm-libc-decls/uchar.h
0.150 [386/130/181] Generating header uchar.h from /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/runtimes/../libc/include/uchar.yaml
0.150 [385/130/182] Generating /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/include/amdgcn-amd-amdhsa/llvm-libc-decls/time.h
0.150 [384/130/183] Generating header time.h from /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/runtimes/../libc/include/time.yaml
0.151 [383/130/184] Building CXX object libc/src/math/amdgpu/CMakeFiles/libc.src.math.amdgpu.lgamma_r.dir/lgamma_r.cpp.o
0.151 [382/130/185] Generating /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/include/amdgcn-amd-amdhsa/llvm-libc-decls/string.h
0.152 [381/130/186] Building CXX object libc/src/stdlib/CMakeFiles/libc.src.stdlib.memalignment.dir/memalignment.cpp.o
FAILED: libc/src/stdlib/CMakeFiles/libc.src.stdlib.memalignment.dir/memalignment.cpp.o 
/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/./bin/clang++ --target=amdgcn-amd-amdhsa -DLIBC_NAMESPACE=__llvm_libc_21_0_0_git -D__LIBC_USE_FLOAT16_CONVERSION -I/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/libc -isystem /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/include/amdgcn-amd-amdhsa -O3 -DNDEBUG --target=amdgcn-amd-amdhsa -DLIBC_QSORT_IMPL=LIBC_QSORT_QUICK_SORT -DLIBC_ADD_NULL_CHECKS "-DLIBC_MATH=(LIBC_MATH_SKIP_ACCURATE_PASS | LIBC_MATH_SMALL_TABLES | LIBC_MATH_NO_ERRNO | LIBC_MATH_NO_EXCEPT)" -fpie -DLIBC_FULL_BUILD -nostdlibinc -ffixed-point -fno-exceptions -fno-lax-vector-conversions -fno-unwind-tables -fno-asynchronous-unwind-tables -fno-rtti -ftrivial-auto-var-init=pattern -fno-omit-frame-pointer -Wall -Wextra -Werror -Wconversion -Wno-sign-conversion -Wdeprecated -Wno-c99-extensions -Wno-gnu-imaginary-constant -Wno-pedantic -Wimplicit-fallthrough -Wwrite-strings -Wextra-semi -Wnewline-eof -Wnonportable-system-include-path -Wstrict-prototypes -Wthread-safety -Wglobal-constructors -nogpulib -fvisibility=hidden -fconvergent-functions -flto -Wno-multi-gpu -Xclang -mcode-object-version=none -DLIBC_COPT_PUBLIC_PACKAGING -UNDEBUG -MD -MT libc/src/stdlib/CMakeFiles/libc.src.stdlib.memalignment.dir/memalignment.cpp.o -MF libc/src/stdlib/CMakeFiles/libc.src.stdlib.memalignment.dir/memalignment.cpp.o.d -o libc/src/stdlib/CMakeFiles/libc.src.stdlib.memalignment.dir/memalignment.cpp.o -c /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/libc/src/stdlib/memalignment.cpp
/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/libc/src/stdlib/memalignment.cpp:20:3: error: unknown type name 'uintptr_t'
   20 |   uintptr_t addr = reinterpret_cast<uintptr_t>(p);
      |   ^
/home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/libc/src/stdlib/memalignment.cpp:20:37: error: unknown type name 'uintptr_t'
   20 |   uintptr_t addr = reinterpret_cast<uintptr_t>(p);
      |                                     ^
2 errors generated.
0.152 [381/129/187] Generating /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/include/amdgcn-amd-amdhsa/llvm-libc-decls/ctype.h
0.152 [381/128/188] Generating /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/include/amdgcn-amd-amdhsa/llvm-libc-decls/stdlib.h
0.152 [381/127/189] Generating header stdlib.h from /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/runtimes/../libc/include/stdlib.yaml
0.152 [381/126/190] Generating /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/include/amdgcn-amd-amdhsa/llvm-libc-decls/stdio.h
0.152 [381/125/191] Generating /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.build/include/amdgcn-amd-amdhsa/llvm-libc-decls/signal.h
0.154 [381/124/192] Building CXX object libc/src/strings/CMakeFiles/libc.src.strings.bcopy.dir/bcopy.cpp.o
0.157 [381/123/193] Generating header signal.h from /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/runtimes/../libc/include/signal.yaml
0.165 [381/122/194] Generating header stdio.h from /home/botworker/builds/openmp-offload-amdgpu-runtime-2/llvm.src/runtimes/../libc/include/stdio.yaml
0.175 [381/121/195] Building CXX object libc/src/string/CMakeFiles/libc.src.string.strstr.dir/strstr.cpp.o
0.182 [381/120/196] Building CXX object libc/src/math/generic/CMakeFiles/libc.src.math.generic.common_constants.dir/common_constants.cpp.o
0.184 [381/119/197] Building CXX object libc/src/__support/GPU/CMakeFiles/libc.src.__support.GPU.allocator.dir/allocator.cpp.o
0.187 [381/118/198] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_leading_zeros_ull.dir/stdc_leading_zeros_ull.cpp.o
0.195 [381/117/199] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_leading_ones_ui.dir/stdc_leading_ones_ui.cpp.o
0.196 [381/116/200] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_leading_ones_ul.dir/stdc_leading_ones_ul.cpp.o
0.199 [381/115/201] Building CXX object libc/src/string/CMakeFiles/libc.src.string.strcasestr.dir/strcasestr.cpp.o
0.207 [381/114/202] Building CXX object libc/src/errno/CMakeFiles/libc.src.errno.errno.dir/libc_errno.cpp.o
0.213 [381/113/203] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_leading_zeros_ui.dir/stdc_leading_zeros_ui.cpp.o
0.213 [381/112/204] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_leading_ones_uc.dir/stdc_leading_ones_uc.cpp.o
0.214 [381/111/205] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_bit_ceil_ul.dir/stdc_bit_ceil_ul.cpp.o
0.215 [381/110/206] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_trailing_zeros_ul.dir/stdc_trailing_zeros_ul.cpp.o
0.216 [381/109/207] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_first_trailing_zero_uc.dir/stdc_first_trailing_zero_uc.cpp.o
0.216 [381/108/208] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_leading_zeros_ul.dir/stdc_leading_zeros_ul.cpp.o
0.217 [381/107/209] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_count_ones_ull.dir/stdc_count_ones_ull.cpp.o
0.217 [381/106/210] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_count_ones_uc.dir/stdc_count_ones_uc.cpp.o
0.217 [381/105/211] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_has_single_bit_uc.dir/stdc_has_single_bit_uc.cpp.o
0.218 [381/104/212] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_count_ones_ul.dir/stdc_count_ones_ul.cpp.o
0.219 [381/103/213] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_trailing_ones_ui.dir/stdc_trailing_ones_ui.cpp.o
0.219 [381/102/214] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_bit_floor_us.dir/stdc_bit_floor_us.cpp.o
0.220 [381/101/215] Building CXX object libc/src/stdbit/CMakeFiles/libc.src.stdbit.stdc_first_leading_one_ui.dir/stdc_first_leading_one_ui.cpp.o
0.220 [381/100/216] Building CXX object libc/src/ctype/CMakeFiles/libc.src.ctype.isalpha_l.dir/isalpha_l.cpp.o

var-const pushed a commit to ldionne/llvm-project that referenced this pull request

Apr 17, 2025

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.

sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request

Jun 3, 2025

This patch fixes the "dereferencing null" UB. Unfortunately, C++ doesn't provide an inverse operation for p->*pmf. See also llvm#130952.

Labels

clang:codegen

IR generation bugs: mangling, exceptions, etc.

clang

Clang issues not falling into any other category

[Clang][CodeGen] Do not set inbounds flag in EmitMemberDataPointerAddress when the base pointer is null by dtcxzyw · Pull Request #130952 · llvm/llvm-project (original) (raw)

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Clang][CodeGen] Do not set inbounds flag in `EmitMemberDataPointerAddress` when the base pointer is null by dtcxzyw · Pull Request #130952 · llvm/llvm-project (original) (raw)