Skip to content

[Clang][CodeGen] Do not set inbounds flag for struct GEP with null base pointers #130734

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 for struct GEP with null base pointers #130734

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
0f6ff60
[Clang][CodeGen] Do not set inbounds flag for struct GEP with null ba…
dtcxzyw Apr 10, 2025
bbe2e97
[Clang][CodeGen] Fix test failures.
dtcxzyw Apr 10, 2025
c287e15
[Clang][CodeGen] Add helper `isUnderlyingBasePointerConstantNull`
dtcxzyw Apr 10, 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
7 changes: 7 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,13 @@ Potentially Breaking Changes
C/C++ Language Potentially Breaking Changes
-------------------------------------------

- New LLVM optimizations have been implemented that optimize pointer arithmetic on
null pointers more aggressively. As part of this, clang has implemented a special
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)

C++ Specific Potentially Breaking Changes
-----------------------------------------

Expand Down
37 changes: 27 additions & 10 deletions clang/lib/CodeGen/CGExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4778,13 +4778,25 @@ EmitExtVectorElementExpr(const ExtVectorElementExpr *E) {
Base.getBaseInfo(), TBAAAccessInfo());
}

bool CodeGenFunction::isUnderlyingBasePointerConstantNull(const Expr *E) {
const Expr *UnderlyingBaseExpr = E->IgnoreParens();
while (auto *BaseMemberExpr = dyn_cast<MemberExpr>(UnderlyingBaseExpr))
UnderlyingBaseExpr = BaseMemberExpr->getBase()->IgnoreParens();
return getContext().isSentinelNullExpr(UnderlyingBaseExpr);
}

LValue CodeGenFunction::EmitMemberExpr(const MemberExpr *E) {
if (DeclRefExpr *DRE = tryToConvertMemberExprToDeclRefExpr(*this, E)) {
EmitIgnoredExpr(E->getBase());
return EmitDeclRefLValue(DRE);
}

Expr *BaseExpr = E->getBase();
// Check whether the underlying base pointer is a constant null.
// If so, we do not set inbounds flag for GEP to avoid breaking some
// old-style offsetof idioms.
bool IsInBounds = !getLangOpts().PointerOverflowDefined &&
!isUnderlyingBasePointerConstantNull(BaseExpr);
// If this is s.x, emit s as an lvalue. If it is s->x, emit s as a scalar.
LValue BaseLV;
if (E->isArrow()) {
Expand All @@ -4806,7 +4818,7 @@ LValue CodeGenFunction::EmitMemberExpr(const MemberExpr *E) {

NamedDecl *ND = E->getMemberDecl();
if (auto *Field = dyn_cast<FieldDecl>(ND)) {
LValue LV = EmitLValueForField(BaseLV, Field);
LValue LV = EmitLValueForField(BaseLV, Field, IsInBounds);
setObjCGCLValueClass(getContext(), E, LV);
if (getLangOpts().OpenMP) {
// If the member was explicitly marked as nontemporal, mark it as
Expand Down Expand Up @@ -4892,12 +4904,15 @@ unsigned CodeGenFunction::getDebugInfoFIndex(const RecordDecl *Rec,
/// Get the address of a zero-sized field within a record. The resulting
/// address doesn't necessarily have the right type.
static Address emitAddrOfZeroSizeField(CodeGenFunction &CGF, Address Base,
const FieldDecl *Field) {
const FieldDecl *Field,
bool IsInBounds) {
CharUnits Offset = CGF.getContext().toCharUnitsFromBits(
CGF.getContext().getFieldOffset(Field));
if (Offset.isZero())
return Base;
Base = Base.withElementType(CGF.Int8Ty);
if (!IsInBounds)
return CGF.Builder.CreateConstByteGEP(Base, Offset);
return CGF.Builder.CreateConstInBoundsByteGEP(Base, Offset);
}

Expand All @@ -4906,16 +4921,16 @@ static Address emitAddrOfZeroSizeField(CodeGenFunction &CGF, Address Base,
///
/// The resulting address doesn't necessarily have the right type.
static Address emitAddrOfFieldStorage(CodeGenFunction &CGF, Address base,
const FieldDecl *field) {
const FieldDecl *field, bool IsInBounds) {
if (isEmptyFieldForLayout(CGF.getContext(), field))
return emitAddrOfZeroSizeField(CGF, base, field);
return emitAddrOfZeroSizeField(CGF, base, field, IsInBounds);

const RecordDecl *rec = field->getParent();

unsigned idx =
CGF.CGM.getTypes().getCGRecordLayout(rec).getLLVMFieldNo(field);

if (CGF.getLangOpts().PointerOverflowDefined)
if (!IsInBounds)
return CGF.Builder.CreateConstGEP2_32(base, 0, idx, field->getName());

return CGF.Builder.CreateStructGEP(base, idx, field->getName());
Expand Down Expand Up @@ -4953,8 +4968,8 @@ static bool hasAnyVptr(const QualType Type, const ASTContext &Context) {
return false;
}

LValue CodeGenFunction::EmitLValueForField(LValue base,
const FieldDecl *field) {
LValue CodeGenFunction::EmitLValueForField(LValue base, const FieldDecl *field,
bool IsInBounds) {
LValueBaseInfo BaseInfo = base.getBaseInfo();

if (field->isBitField()) {
Expand All @@ -4977,7 +4992,7 @@ LValue CodeGenFunction::EmitLValueForField(LValue base,
(!getDebugInfo() || !rec->hasAttr<BPFPreserveAccessIndexAttr>())) {
if (Idx != 0) {
// For structs, we GEP to the field that the record layout suggests.
if (getLangOpts().PointerOverflowDefined)
if (!IsInBounds)
Addr = Builder.CreateConstGEP2_32(Addr, 0, Idx, field->getName());
else
Addr = Builder.CreateStructGEP(Addr, Idx, field->getName());
Expand Down Expand Up @@ -5090,7 +5105,7 @@ LValue CodeGenFunction::EmitLValueForField(LValue base,
if (!IsInPreservedAIRegion &&
(!getDebugInfo() || !rec->hasAttr<BPFPreserveAccessIndexAttr>()))
// For structs, we GEP to the field that the record layout suggests.
addr = emitAddrOfFieldStorage(*this, addr, field);
addr = emitAddrOfFieldStorage(*this, addr, field, IsInBounds);
else
// Remember the original struct field index
addr = emitPreserveStructAccess(*this, base, addr, field);
Expand Down Expand Up @@ -5134,7 +5149,9 @@ CodeGenFunction::EmitLValueForFieldInitialization(LValue Base,
if (!FieldType->isReferenceType())
return EmitLValueForField(Base, Field);

Address V = emitAddrOfFieldStorage(*this, Base.getAddress(), Field);
Address V = emitAddrOfFieldStorage(
*this, Base.getAddress(), Field,
/*IsInBounds=*/!getLangOpts().PointerOverflowDefined);

// Make sure that the address is pointing to the right type.
llvm::Type *llvmType = ConvertTypeForMem(FieldType);
Expand Down
5 changes: 4 additions & 1 deletion clang/lib/CodeGen/CodeGenFunction.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4459,7 +4459,8 @@ class CodeGenFunction : public CodeGenTypeCache {
const ObjCIvarDecl *Ivar);
llvm::Value *EmitIvarOffsetAsPointerDiff(const ObjCInterfaceDecl *Interface,
const ObjCIvarDecl *Ivar);
LValue EmitLValueForField(LValue Base, const FieldDecl *Field);
LValue EmitLValueForField(LValue Base, const FieldDecl *Field,
bool IsInBounds = true);
LValue EmitLValueForLambdaField(const FieldDecl *Field);
LValue EmitLValueForLambdaField(const FieldDecl *Field,
llvm::Value *ThisValue);
Expand Down Expand Up @@ -4556,6 +4557,8 @@ class CodeGenFunction : public CodeGenTypeCache {
const CXXRecordDecl *RD);

bool isPointerKnownNonNull(const Expr *E);
/// Check whether the underlying base pointer is a constant null.
bool isUnderlyingBasePointerConstantNull(const Expr *E);

/// Create the discriminator from the storage address and the entity hash.
llvm::Value *EmitPointerAuthBlendDiscriminator(llvm::Value *StorageAddress,
Expand Down
2 changes: 1 addition & 1 deletion clang/test/CodeGen/catch-nullptr-and-nonzero-offset-in-offsetof-idiom.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ struct S {

// CHECK-LABEL: @get_offset_of_y_naively(
// CHECK-NEXT: entry:
// CHECK-NEXT: ret i64 ptrtoint (ptr getelementptr inbounds nuw ([[STRUCT_S:%.*]], ptr null, i32 0, i32 1) to i64)
// CHECK-NEXT: ret i64 ptrtoint (ptr getelementptr ([[STRUCT_S:%.*]], ptr null, i32 0, i32 1) to i64)
//
uintptr_t get_offset_of_y_naively(void) {
return ((uintptr_t)(&(((struct S *)0)->y)));
Expand Down
72 changes: 71 additions & 1 deletion clang/test/CodeGenCXX/catch-nullptr-and-nonzero-offset-in-offsetof-idiom.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,82 @@ struct S {

// CHECK-LABEL: @_Z23get_offset_of_y_naivelyv(
// CHECK-NEXT: entry:
// CHECK-NEXT: ret i64 ptrtoint (ptr getelementptr inbounds nuw ([[STRUCT_S:%.*]], ptr null, i32 0, i32 1) to i64)
// CHECK-NEXT: ret i64 ptrtoint (ptr getelementptr ([[STRUCT_S:%.*]], ptr null, i32 0, i32 1) to i64)
//
uintptr_t get_offset_of_y_naively() {
return ((uintptr_t)(&(((S *)nullptr)->y)));
}

struct Empty {};

struct T {
int a;
S s;
[[no_unique_address]] Empty e1;
int b;
[[no_unique_address]] Empty e2;
};

// CHECK-LABEL: @_Z30get_offset_of_y_naively_nestedv(
// CHECK-NEXT: entry:
// CHECK-NEXT: ret i64 ptrtoint (ptr getelementptr ([[STRUCT_S:%.*]], ptr getelementptr ([[STRUCT_T:%.*]], ptr null, i32 0, i32 1), i32 0, i32 1) to i64)
//
uintptr_t get_offset_of_y_naively_nested() {
return ((uintptr_t)(&(((T *)nullptr)->s.y)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good to have tests for other null pointer constants like 0, but there's a question of how deep down the rabbit hole we want the extension to go. I think it makes sense to allow it only for null pointer constants, not just any constant expression which converts to a null pointer. But maybe that's too restrictive?

Suggested tests:

nullptr_t{} // Should probably be accepted because this is a valid null pointer constant
constexpr void *null_ptr = nullptr; // Should probably be rejected, null_ptr is not a null pointer constant
0 // Should be accepted because this is a valid null pointer constant
(void *)0 // Should only be accepted in C because it's a valid null pointer constant there, but not in C++

and you should also ensure the C tests cover these cases as well.

}

// CHECK-LABEL: @_Z42get_offset_of_y_naively_nested_with_parensv(
// CHECK-NEXT: entry:
// CHECK-NEXT: ret i64 ptrtoint (ptr getelementptr ([[STRUCT_S:%.*]], ptr getelementptr ([[STRUCT_T:%.*]], ptr null, i32 0, i32 1), i32 0, i32 1) to i64)
//
uintptr_t get_offset_of_y_naively_nested_with_parens() {
return ((uintptr_t)(&((((T *)nullptr)->s).y)));
}

// CHECK-LABEL: @_Z26get_offset_of_zero_storagev(
// CHECK-NEXT: entry:
// CHECK-NEXT: ret i64 ptrtoint (ptr getelementptr (i8, ptr null, i64 16) to i64)
//
uintptr_t get_offset_of_zero_storage() {
return ((uintptr_t)(&(((T *)nullptr)->e2)));
}

namespace std { typedef decltype(__nullptr) nullptr_t; }
// CHECK-LABEL: @_Z29get_offset_of_y_integral_zerov(
// CHECK-NEXT: entry:
// CHECK-NEXT: ret i64 ptrtoint (ptr getelementptr ([[STRUCT_S:%.*]], ptr null, i32 0, i32 1) to i64)
//
uintptr_t get_offset_of_y_integral_zero() {
return ((uintptr_t)(&(((S *)0)->y)));
}

// CHECK-LABEL: @_Z37get_offset_of_y_integral_zero_voidptrv(
// CHECK-NEXT: entry:
// CHECK-NEXT: ret i64 ptrtoint (ptr getelementptr ([[STRUCT_S:%.*]], ptr null, i32 0, i32 1) to i64)
//
uintptr_t get_offset_of_y_integral_zero_voidptr() {
return ((uintptr_t)(&(((S *)(void*)0)->y)));
}

// CHECK-LABEL: @_Z25get_offset_of_y_nullptr_tv(
// CHECK-NEXT: entry:
// CHECK-NEXT: ret i64 ptrtoint (ptr getelementptr ([[STRUCT_S:%.*]], ptr null, i32 0, i32 1) to i64)
//
uintptr_t get_offset_of_y_nullptr_t() {
return ((uintptr_t)(&(((S *)std::nullptr_t{})->y)));
}

// CHECK-LABEL: @_Z32get_offset_of_y_nullptr_constantv(
// CHECK-NEXT: entry:
// CHECK-NEXT: [[NULL:%.*]] = alloca ptr, align 8
// CHECK-NEXT: store ptr null, ptr [[NULL]], align 8
// CHECK-NEXT: ret i64 ptrtoint (ptr getelementptr inbounds nuw ([[STRUCT_S:%.*]], ptr null, i32 0, i32 1) to i64)
//
uintptr_t get_offset_of_y_nullptr_constant() {
constexpr void *null = nullptr;
return ((uintptr_t)(&(((S *)null)->y)));
}

// CHECK-LABEL: @_Z27get_offset_of_y_via_builtinv(
// CHECK-NEXT: entry:
// CHECK-NEXT: ret i64 4
Expand Down