Skip to content

[Clang][CodeGen] Do not set inbounds flag in EmitMemberDataPointerAddress when the base pointer is null #130952

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
merged 3 commits into from
Apr 11, 2025
Merged

[Clang][CodeGen] Do not set inbounds flag in EmitMemberDataPointerAddress when the base pointer is null #130952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a9c7cf
[Clang][CodeGen] Do not set inbounds in `EmitMemberDataPointerAddress…
dtcxzyw Mar 12, 2025
af96610
[Clang][CodeGen] Address review comments.
dtcxzyw Apr 10, 2025
e71ca8b
[Clang][Docs] Update ReleaseNotes. NFC.
dtcxzyw Apr 11, 2025
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
2 changes: 1 addition & 1 deletion clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ C/C++ Language Potentially Breaking Changes
case for old-style offsetof idioms like ``((int)(&(((struct S *)0)->field)))``, to
ensure they are not caught by these optimizations. It is also possible to use
``-fwrapv-pointer`` or ``-fno-delete-null-pointer-checks`` to make pointer arithmetic
on null pointers well-defined. (#GH130734, #GH130742)
on null pointers well-defined. (#GH130734, #GH130742, #GH130952)

C++ Specific Potentially Breaking Changes
-----------------------------------------
Expand Down
7 changes: 3 additions & 4 deletions clang/lib/CodeGen/CGCXXABI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,9 @@ CGCallee CGCXXABI::EmitLoadOfMemberFunctionPointer(
return CGCallee::forDirect(FnPtr, FPT);
}

llvm::Value *
CGCXXABI::EmitMemberDataPointerAddress(CodeGenFunction &CGF, const Expr *E,
Address Base, llvm::Value *MemPtr,
const MemberPointerType *MPT) {
llvm::Value *CGCXXABI::EmitMemberDataPointerAddress(
CodeGenFunction &CGF, const Expr *E, Address Base, llvm::Value *MemPtr,
const MemberPointerType *MPT, bool IsInBounds) {
ErrorUnsupportedABI(CGF, "loads of member pointers");
llvm::Type *Ty =
llvm::PointerType::get(CGF.getLLVMContext(), Base.getAddressSpace());
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/CodeGen/CGCXXABI.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ class CGCXXABI {
virtual llvm::Value *
EmitMemberDataPointerAddress(CodeGenFunction &CGF, const Expr *E,
Address Base, llvm::Value *MemPtr,
const MemberPointerType *MPT);
const MemberPointerType *MPT, bool IsInBounds);

/// Perform a derived-to-base, base-to-derived, or bitcast member
/// pointer conversion.
Expand Down
15 changes: 6 additions & 9 deletions clang/lib/CodeGen/CGClass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,16 +147,13 @@ Address CodeGenFunction::LoadCXXThisAddress() {
/// Emit the address of a field using a member data pointer.
///
/// \param E Only used for emergency diagnostics
Address
CodeGenFunction::EmitCXXMemberDataPointerAddress(const Expr *E, Address base,
llvm::Value *memberPtr,
const MemberPointerType *memberPtrType,
LValueBaseInfo *BaseInfo,
TBAAAccessInfo *TBAAInfo) {
Address CodeGenFunction::EmitCXXMemberDataPointerAddress(
const Expr *E, Address base, llvm::Value *memberPtr,
const MemberPointerType *memberPtrType, bool IsInBounds,
LValueBaseInfo *BaseInfo, TBAAAccessInfo *TBAAInfo) {
// Ask the ABI to compute the actual address.
llvm::Value *ptr =
CGM.getCXXABI().EmitMemberDataPointerAddress(*this, E, base,
memberPtr, memberPtrType);
llvm::Value *ptr = CGM.getCXXABI().EmitMemberDataPointerAddress(
*this, E, base, memberPtr, memberPtrType, IsInBounds);

QualType memberType = memberPtrType->getPointeeType();
CharUnits memberAlign =
Expand Down
11 changes: 6 additions & 5 deletions clang/lib/CodeGen/CGExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,8 +654,8 @@ EmitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *M) {

case SubobjectAdjustment::MemberPointerAdjustment: {
llvm::Value *Ptr = EmitScalarExpr(Adjustment.Ptr.RHS);
Object = EmitCXXMemberDataPointerAddress(E, Object, Ptr,
Adjustment.Ptr.MPT);
Object = EmitCXXMemberDataPointerAddress(
E, Object, Ptr, Adjustment.Ptr.MPT, /*IsInBounds=*/true);
Copy link
Collaborator

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.

break;
}
}
Expand Down Expand Up @@ -6295,9 +6295,10 @@ EmitPointerToDataMemberBinaryExpr(const BinaryOperator *E) {

LValueBaseInfo BaseInfo;
TBAAAccessInfo TBAAInfo;
Address MemberAddr =
EmitCXXMemberDataPointerAddress(E, BaseAddr, OffsetV, MPT, &BaseInfo,
&TBAAInfo);
bool IsInBounds = !getLangOpts().PointerOverflowDefined &&
!isUnderlyingBasePointerConstantNull(E->getLHS());
Address MemberAddr = EmitCXXMemberDataPointerAddress(
E, BaseAddr, OffsetV, MPT, IsInBounds, &BaseInfo, &TBAAInfo);

return MakeAddrLValue(MemberAddr, MPT->getPointeeType(), BaseInfo, TBAAInfo);
}
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/CodeGen/CodeGenFunction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4642,7 +4642,7 @@ class CodeGenFunction : public CodeGenTypeCache {
// Compute the object pointer.
Address EmitCXXMemberDataPointerAddress(
const Expr *E, Address base, llvm::Value *memberPtr,
const MemberPointerType *memberPtrType,
const MemberPointerType *memberPtrType, bool IsInBounds,
LValueBaseInfo *BaseInfo = nullptr, TBAAAccessInfo *TBAAInfo = nullptr);
RValue EmitCXXMemberPointerCallExpr(const CXXMemberCallExpr *E,
ReturnValueSlot ReturnValue,
Expand Down
19 changes: 10 additions & 9 deletions clang/lib/CodeGen/ItaniumCXXABI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,10 @@ class ItaniumCXXABI : public CodeGen::CGCXXABI {
llvm::Value *MemFnPtr,
const MemberPointerType *MPT) override;

llvm::Value *
EmitMemberDataPointerAddress(CodeGenFunction &CGF, const Expr *E,
Address Base,
llvm::Value *MemPtr,
const MemberPointerType *MPT) override;
llvm::Value *EmitMemberDataPointerAddress(CodeGenFunction &CGF, const Expr *E,
Address Base, llvm::Value *MemPtr,
const MemberPointerType *MPT,
bool IsInBounds) override;

llvm::Value *EmitMemberPointerConversion(CodeGenFunction &CGF,
const CastExpr *E,
Expand Down Expand Up @@ -867,14 +866,16 @@ CGCallee ItaniumCXXABI::EmitLoadOfMemberFunctionPointer(
/// base object.
llvm::Value *ItaniumCXXABI::EmitMemberDataPointerAddress(
CodeGenFunction &CGF, const Expr *E, Address Base, llvm::Value *MemPtr,
const MemberPointerType *MPT) {
const MemberPointerType *MPT, bool IsInBounds) {
assert(MemPtr->getType() == CGM.PtrDiffTy);

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.
llvm::Value *BaseAddr = Base.emitRawPointer(CGF);
return Builder.CreateGEP(CGF.Int8Ty, BaseAddr, MemPtr, "memptr.offset",
IsInBounds ? llvm::GEPNoWrapFlags::inBounds()
: llvm::GEPNoWrapFlags::none());
}

// See if it's possible to return a constant signed pointer.
Expand Down
17 changes: 9 additions & 8 deletions clang/lib/CodeGen/MicrosoftCXXABI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,10 +689,10 @@ class MicrosoftCXXABI : public CGCXXABI {
llvm::Value *MemPtr,
const MemberPointerType *MPT) override;

llvm::Value *
EmitMemberDataPointerAddress(CodeGenFunction &CGF, const Expr *E,
Address Base, llvm::Value *MemPtr,
const MemberPointerType *MPT) override;
llvm::Value *EmitMemberDataPointerAddress(CodeGenFunction &CGF, const Expr *E,
Address Base, llvm::Value *MemPtr,
const MemberPointerType *MPT,
bool IsInBounds) override;

llvm::Value *EmitNonNullMemberPointerConversion(
const MemberPointerType *SrcTy, const MemberPointerType *DstTy,
Expand Down Expand Up @@ -3240,7 +3240,7 @@ llvm::Value *MicrosoftCXXABI::AdjustVirtualBase(

llvm::Value *MicrosoftCXXABI::EmitMemberDataPointerAddress(
CodeGenFunction &CGF, const Expr *E, Address Base, llvm::Value *MemPtr,
const MemberPointerType *MPT) {
const MemberPointerType *MPT, bool IsInBounds) {
assert(MPT->isMemberDataPointer());
CGBuilderTy &Builder = CGF.Builder;
const CXXRecordDecl *RD = MPT->getMostRecentCXXRecordDecl();
Expand Down Expand Up @@ -3269,9 +3269,10 @@ 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.
return Builder.CreateGEP(CGF.Int8Ty, Addr, FieldOffset, "memptr.offset",
IsInBounds ? llvm::GEPNoWrapFlags::inBounds()
: llvm::GEPNoWrapFlags::none());
}

llvm::Value *
Expand Down
21 changes: 21 additions & 0 deletions clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
21 changes: 21 additions & 0 deletions clang/test/CodeGenCXX/pointers-to-data-members.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}