[Clang][CodeGen] Respect -fwrapv-pointer when emitting struct GEPs #134269

dtcxzyw · 2025-04-03T16:24:37Z

This patch turns off inbounds/nuw flags for member accesses when -fwrapv-pointer is set.
Note: There are many calls to CGBuilderTy::CreateStructGEP, and most of them are safe. So I think it is probably overkill to handle this in CreateStructGEP.

Closes #132449.

It is required by #130734.

llvmbot · 2025-04-03T16:25:16Z

@llvm/pr-subscribers-clang

Author: Yingwei Zheng (dtcxzyw)

Changes

This patch turns off inbounds/nuw flags for member accesses when -fwrapv-pointer is set.
Note: There are many calls to CGBuilderTy::CreateStructGEP, and most of them are safe. So I think it is probably overkill to handle this in CreateStructGEP.

Closes #132449.

It is required by #130734.

Full diff: https://github.com/llvm/llvm-project/pull/134269.diff

2 Files Affected:

(modified) clang/lib/CodeGen/CGExpr.cpp (+9-2)
(modified) clang/test/CodeGen/pointer-overflow.c (+21)

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 3d3a111f0514a..d32591ed896d0 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -4922,6 +4922,9 @@ static Address emitAddrOfFieldStorage(CodeGenFunction &CGF, Address base,
   unsigned idx =
     CGF.CGM.getTypes().getCGRecordLayout(rec).getLLVMFieldNo(field);
 
+  if (CGF.getLangOpts().PointerOverflowDefined)
+    return CGF.Builder.CreateConstGEP2_32(base, 0, idx, field->getName());
+
   return CGF.Builder.CreateStructGEP(base, idx, field->getName());
 }
 
@@ -4979,9 +4982,13 @@ LValue CodeGenFunction::EmitLValueForField(LValue base,
     if (!UseVolatile) {
       if (!IsInPreservedAIRegion &&
           (!getDebugInfo() || !rec->hasAttr<BPFPreserveAccessIndexAttr>())) {
-        if (Idx != 0)
+        if (Idx != 0) {
           // For structs, we GEP to the field that the record layout suggests.
-          Addr = Builder.CreateStructGEP(Addr, Idx, field->getName());
+          if (getLangOpts().PointerOverflowDefined)
+            Addr = Builder.CreateConstGEP2_32(Addr, 0, Idx, field->getName());
+          else
+            Addr = Builder.CreateStructGEP(Addr, Idx, field->getName());
+        }
       } else {
         llvm::DIType *DbgInfo = getDebugInfo()->getOrCreateRecordType(
             getContext().getRecordType(rec), rec->getLocation());
diff --git a/clang/test/CodeGen/pointer-overflow.c b/clang/test/CodeGen/pointer-overflow.c
index 9c7821b841980..70e3c855bff90 100644
--- a/clang/test/CodeGen/pointer-overflow.c
+++ b/clang/test/CodeGen/pointer-overflow.c
@@ -10,3 +10,24 @@ void test(void) {
   // DEFAULT: getelementptr inbounds nuw i32, ptr
   // FWRAPV-POINTER: getelementptr i32, ptr
 }
+
+struct S {
+  int a;
+  int b;
+  int c: 10;
+  int d: 10;
+};
+
+int test_struct(struct S* s) {
+  // -fwrapv-pointer should turn off inbounds nuw for struct GEP's
+  return s->b;
+  // DEFAULT: getelementptr inbounds nuw %struct.S, ptr
+  // FWRAPV-POINTER: getelementptr %struct.S, ptr
+}
+
+int test_struct_bitfield(struct S* s) {
+  // -fwrapv-pointer should turn off inbounds nuw for struct GEP's
+  return s->d;
+  // DEFAULT: getelementptr inbounds nuw %struct.S, ptr
+  // FWRAPV-POINTER: getelementptr %struct.S, ptr
+}

llvmbot · 2025-04-03T16:25:16Z

@llvm/pr-subscribers-clang-codegen

Author: Yingwei Zheng (dtcxzyw)

Changes

This patch turns off inbounds/nuw flags for member accesses when -fwrapv-pointer is set.
Note: There are many calls to CGBuilderTy::CreateStructGEP, and most of them are safe. So I think it is probably overkill to handle this in CreateStructGEP.

Closes #132449.

It is required by #130734.

Full diff: https://github.com/llvm/llvm-project/pull/134269.diff

2 Files Affected:

(modified) clang/lib/CodeGen/CGExpr.cpp (+9-2)
(modified) clang/test/CodeGen/pointer-overflow.c (+21)

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 3d3a111f0514a..d32591ed896d0 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -4922,6 +4922,9 @@ static Address emitAddrOfFieldStorage(CodeGenFunction &CGF, Address base,
   unsigned idx =
     CGF.CGM.getTypes().getCGRecordLayout(rec).getLLVMFieldNo(field);
 
+  if (CGF.getLangOpts().PointerOverflowDefined)
+    return CGF.Builder.CreateConstGEP2_32(base, 0, idx, field->getName());
+
   return CGF.Builder.CreateStructGEP(base, idx, field->getName());
 }
 
@@ -4979,9 +4982,13 @@ LValue CodeGenFunction::EmitLValueForField(LValue base,
     if (!UseVolatile) {
       if (!IsInPreservedAIRegion &&
           (!getDebugInfo() || !rec->hasAttr<BPFPreserveAccessIndexAttr>())) {
-        if (Idx != 0)
+        if (Idx != 0) {
           // For structs, we GEP to the field that the record layout suggests.
-          Addr = Builder.CreateStructGEP(Addr, Idx, field->getName());
+          if (getLangOpts().PointerOverflowDefined)
+            Addr = Builder.CreateConstGEP2_32(Addr, 0, Idx, field->getName());
+          else
+            Addr = Builder.CreateStructGEP(Addr, Idx, field->getName());
+        }
       } else {
         llvm::DIType *DbgInfo = getDebugInfo()->getOrCreateRecordType(
             getContext().getRecordType(rec), rec->getLocation());
diff --git a/clang/test/CodeGen/pointer-overflow.c b/clang/test/CodeGen/pointer-overflow.c
index 9c7821b841980..70e3c855bff90 100644
--- a/clang/test/CodeGen/pointer-overflow.c
+++ b/clang/test/CodeGen/pointer-overflow.c
@@ -10,3 +10,24 @@ void test(void) {
   // DEFAULT: getelementptr inbounds nuw i32, ptr
   // FWRAPV-POINTER: getelementptr i32, ptr
 }
+
+struct S {
+  int a;
+  int b;
+  int c: 10;
+  int d: 10;
+};
+
+int test_struct(struct S* s) {
+  // -fwrapv-pointer should turn off inbounds nuw for struct GEP's
+  return s->b;
+  // DEFAULT: getelementptr inbounds nuw %struct.S, ptr
+  // FWRAPV-POINTER: getelementptr %struct.S, ptr
+}
+
+int test_struct_bitfield(struct S* s) {
+  // -fwrapv-pointer should turn off inbounds nuw for struct GEP's
+  return s->d;
+  // DEFAULT: getelementptr inbounds nuw %struct.S, ptr
+  // FWRAPV-POINTER: getelementptr %struct.S, ptr
+}

MaskRay · 2025-04-03T16:31:31Z

Looks good to me, but please allow some time for other reviewers to weigh in.

efriedma-quic · 2025-04-03T16:57:57Z

most of them are safe

What's the distinguishing factor here? Do you consider it "safe" to mark inbounds if the pointer is immediately dereferenced? Or does the pointer have to refer to a known successful allocation? Or something else?

dtcxzyw · 2025-04-04T08:51:19Z

most of them are safe

What's the distinguishing factor here? Do you consider it "safe" to mark inbounds if the pointer is immediately dereferenced? Or does the pointer have to refer to a known successful allocation? Or something else?

Other calls to CGBuilderTy::CreateStructGEP operate on some internal structures like valist/complex/kernel launch params/GPU printf args.

nikic · 2025-04-04T10:20:54Z

Usually we'd want to also insert -fsanitize=pointer-overflow instrumentation in places where we respect -fwrapv-pointer. I'm not entirely sure whether we want to do this here, as it will probably blow up the amount of inserted instrumentation for little benefit...

dtcxzyw · 2025-04-04T12:29:48Z

Usually we'd want to also insert -fsanitize=pointer-overflow instrumentation in places where we respect -fwrapv-pointer. I'm not entirely sure whether we want to do this here, as it will probably blow up the amount of inserted instrumentation for little benefit...

A struct GEP is unlikely to overflow. But a null check should be useful.

shafik

This makes sense to me but this is not my area.

shafik · 2025-04-08T04:01:32Z

clang/test/CodeGen/pointer-overflow.c

+struct S {
+  int a;
+  int b;
+  int c: 10;


I am curious why did you add bit-fields in specifically to this test? Are they particularly different wrt other types of fields like arrays, pointers, member pointers etc that we would expect them to break?

Bitfields are handled by a different code path:

llvm-project/clang/lib/CodeGen/CGExpr.cpp

Line 4977 in 2c1bdd4

Addr = Builder.CreateStructGEP(Addr, Idx, field->getName());

dtcxzyw · 2025-04-09T11:03:37Z

@efriedma-quic Any more comments?

efriedma-quic

LGTM

…lvm#134269) This patch turns off inbounds/nuw flags for member accesses when `-fwrapv-pointer` is set. Closes llvm#132449. It is required by llvm#130734.

[Clang][CodeGen] Respect -fwrapv-pointer when emitting struct GEPs

14dc299

dtcxzyw requested review from nikic, MaskRay, AaronBallman, shafik and efriedma-quic April 3, 2025 16:24

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Apr 3, 2025

MaskRay approved these changes Apr 3, 2025

View reviewed changes

shafik reviewed Apr 8, 2025

View reviewed changes

efriedma-quic approved these changes Apr 9, 2025

View reviewed changes

dtcxzyw merged commit c048073 into llvm:main Apr 9, 2025
12 of 14 checks passed

dtcxzyw deleted the wrapv-pointer-struct-gep branch April 9, 2025 16:48

dtcxzyw mentioned this pull request Apr 10, 2025

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang][CodeGen] Respect -fwrapv-pointer when emitting struct GEPs #134269

[Clang][CodeGen] Respect -fwrapv-pointer when emitting struct GEPs #134269

Uh oh!

dtcxzyw commented Apr 3, 2025

Uh oh!

llvmbot commented Apr 3, 2025

Uh oh!

llvmbot commented Apr 3, 2025

Uh oh!

MaskRay commented Apr 3, 2025

Uh oh!

efriedma-quic commented Apr 3, 2025

Uh oh!

dtcxzyw commented Apr 4, 2025

Uh oh!

nikic commented Apr 4, 2025 •

edited

Loading

Uh oh!

dtcxzyw commented Apr 4, 2025

Uh oh!

shafik left a comment

Uh oh!

shafik Apr 8, 2025

Uh oh!

dtcxzyw Apr 8, 2025

Uh oh!

dtcxzyw commented Apr 9, 2025

Uh oh!

efriedma-quic left a comment

Uh oh!

Uh oh!

Uh oh!

[Clang][CodeGen] Respect -fwrapv-pointer when emitting struct GEPs #134269

[Clang][CodeGen] Respect -fwrapv-pointer when emitting struct GEPs #134269

Uh oh!

Conversation

dtcxzyw commented Apr 3, 2025

Uh oh!

llvmbot commented Apr 3, 2025

Uh oh!

llvmbot commented Apr 3, 2025

Uh oh!

MaskRay commented Apr 3, 2025

Uh oh!

efriedma-quic commented Apr 3, 2025

Uh oh!

dtcxzyw commented Apr 4, 2025

Uh oh!

nikic commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtcxzyw commented Apr 4, 2025

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

shafik Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

dtcxzyw Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

dtcxzyw commented Apr 9, 2025

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nikic commented Apr 4, 2025 •

edited

Loading