[Clang] Fix 'counted_by' for nested struct pointers

[Cydox]

Fixes #110385

Fix counted_by attribute for cases where the flexible array member is accessed through struct pointer inside another struct:

struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};

__builtin_dynamic_object_size(p->growable->array, 0);

This commit makes sure that if the StructBase is both a MemberExpr and a pointer, it is treated as a pointer. Otherwise clang will generate to code to access the address of p->growable intead of loading the value of p->growable->length.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jan Hendrik Farr (Cydox)

Changes

Fixes #110385

Fix counted_by attribute for cases where the flexible array member is accessed through struct pointer inside another struct:

struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};

__builtin_dynamic_object_size(p->growable->array, 0);

This commit makes sure that if the StructBase is both a MemberExpr and a pointer, it is treated as a pointer. Otherwise clang will generate to code to access the address of p->growable intead of loading the value of p->growable->length.

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/CGExpr.cpp (+4-4)
• (added) clang/test/CodeGen/attr-counted-by-pr110385.c (+62)

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index df4994ba9af6e1..2875cf18d4f6c9 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -1165,15 +1165,15 @@ llvm::Value *CodeGenFunction::EmitLoadOfCountedByField(
     Res = EmitDeclRefLValue(DRE).getPointer(*this);
     Res = Builder.CreateAlignedLoad(ConvertType(DRE->getType()), Res,
                                     getPointerAlign(), "dre.load");
-  } else if (const MemberExpr *ME = dyn_cast<MemberExpr>(StructBase)) {
-    LValue LV = EmitMemberExpr(ME);
-    Address Addr = LV.getAddress();
-    Res = Addr.emitRawPointer(*this);
   } else if (StructBase->getType()->isPointerType()) {
     LValueBaseInfo BaseInfo;
     TBAAAccessInfo TBAAInfo;
     Address Addr = EmitPointerWithAlignment(StructBase, &BaseInfo, &TBAAInfo);
     Res = Addr.emitRawPointer(*this);
+  } else if (const MemberExpr *ME = dyn_cast<MemberExpr>(StructBase)) {
+    LValue LV = EmitMemberExpr(ME);
+    Address Addr = LV.getAddress();
+    Res = Addr.emitRawPointer(*this);
   } else {
     return nullptr;
   }
diff --git a/clang/test/CodeGen/attr-counted-by-pr110385.c b/clang/test/CodeGen/attr-counted-by-pr110385.c
new file mode 100644
index 00000000000000..e120dcc583578d
--- /dev/null
+++ b/clang/test/CodeGen/attr-counted-by-pr110385.c
@@ -0,0 +1,62 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 4
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -O2 -Wno-missing-declarations -emit-llvm -o - %s | FileCheck %s
+
+// See #110385
+// Based on reproducer from Kees Cook:
+// https://lore.kernel.org/all/202409170436.C3C6E7F7A@keescook/
+
+struct variable {
+        int a;
+        int b;
+        int length;
+        short array[] __attribute__((counted_by(length)));
+};
+
+struct bucket {
+        int a;
+        struct variable *growable;
+        int b;
+};
+
+struct bucket2 {
+        int a;
+        struct variable growable;
+};
+
+void init(void * __attribute__((pass_dynamic_object_size(0))));
+
+// CHECK-LABEL: define dso_local void @test1(
+// CHECK-SAME: ptr nocapture noundef readonly [[FOO:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[GROWABLE:%.*]] = getelementptr inbounds nuw i8, ptr [[FOO]], i64 8
+// CHECK-NEXT:    [[TMP0:%.*]] = load ptr, ptr [[GROWABLE]], align 8, !tbaa [[TBAA2:![0-9]+]]
+// CHECK-NEXT:    [[ARRAY:%.*]] = getelementptr inbounds nuw i8, ptr [[TMP0]], i64 12
+// CHECK-NEXT:    [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[TMP0]], i64 8
+// CHECK-NEXT:    [[DOT_COUNTED_BY_LOAD:%.*]] = load i32, ptr [[DOT_COUNTED_BY_GEP]], align 4
+// CHECK-NEXT:    [[TMP1:%.*]] = sext i32 [[DOT_COUNTED_BY_LOAD]] to i64
+// CHECK-NEXT:    [[TMP2:%.*]] = shl nsw i64 [[TMP1]], 1
+// CHECK-NEXT:    [[TMP3:%.*]] = icmp sgt i32 [[DOT_COUNTED_BY_LOAD]], -1
+// CHECK-NEXT:    [[TMP4:%.*]] = select i1 [[TMP3]], i64 [[TMP2]], i64 0
+// CHECK-NEXT:    tail call void @init(ptr noundef nonnull [[ARRAY]], i64 noundef [[TMP4]]) #[[ATTR2:[0-9]+]]
+// CHECK-NEXT:    ret void
+//
+void test1(struct bucket *foo) {
+        init(foo->growable->array);
+}
+
+// CHECK-LABEL: define dso_local void @test2(
+// CHECK-SAME: ptr noundef [[FOO:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[ARRAY:%.*]] = getelementptr inbounds nuw i8, ptr [[FOO]], i64 16
+// CHECK-NEXT:    [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[FOO]], i64 12
+// CHECK-NEXT:    [[DOT_COUNTED_BY_LOAD:%.*]] = load i32, ptr [[DOT_COUNTED_BY_GEP]], align 4
+// CHECK-NEXT:    [[TMP0:%.*]] = sext i32 [[DOT_COUNTED_BY_LOAD]] to i64
+// CHECK-NEXT:    [[TMP1:%.*]] = shl nsw i64 [[TMP0]], 1
+// CHECK-NEXT:    [[TMP2:%.*]] = icmp sgt i32 [[DOT_COUNTED_BY_LOAD]], -1
+// CHECK-NEXT:    [[TMP3:%.*]] = select i1 [[TMP2]], i64 [[TMP1]], i64 0
+// CHECK-NEXT:    tail call void @init(ptr noundef nonnull [[ARRAY]], i64 noundef [[TMP3]]) #[[ATTR2]]
+// CHECK-NEXT:    ret void
+//
+void test2(struct bucket2 *foo) {
+        init(foo->growable.array);
+}

[llvmbot]

@llvm/pr-subscribers-clang-codegen

Author: Jan Hendrik Farr (Cydox)

Changes

Fixes #110385

Fix counted_by attribute for cases where the flexible array member is accessed through struct pointer inside another struct:

struct variable {
        int a;
        int b;
        int length;
        short array[] __attribute__((counted_by(length)));
};

struct bucket {
        int a;
        struct variable *growable;
        int b;
};

__builtin_dynamic_object_size(p->growable->array, 0);

This commit makes sure that if the StructBase is both a MemberExpr and a pointer, it is treated as a pointer. Otherwise clang will generate to code to access the address of p->growable intead of loading the value of p->growable->length.

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/CGExpr.cpp (+4-4)
• (added) clang/test/CodeGen/attr-counted-by-pr110385.c (+62)

diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index df4994ba9af6e1..2875cf18d4f6c9 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -1165,15 +1165,15 @@ llvm::Value *CodeGenFunction::EmitLoadOfCountedByField(
     Res = EmitDeclRefLValue(DRE).getPointer(*this);
     Res = Builder.CreateAlignedLoad(ConvertType(DRE->getType()), Res,
                                     getPointerAlign(), "dre.load");
-  } else if (const MemberExpr *ME = dyn_cast<MemberExpr>(StructBase)) {
-    LValue LV = EmitMemberExpr(ME);
-    Address Addr = LV.getAddress();
-    Res = Addr.emitRawPointer(*this);
   } else if (StructBase->getType()->isPointerType()) {
     LValueBaseInfo BaseInfo;
     TBAAAccessInfo TBAAInfo;
     Address Addr = EmitPointerWithAlignment(StructBase, &BaseInfo, &TBAAInfo);
     Res = Addr.emitRawPointer(*this);
+  } else if (const MemberExpr *ME = dyn_cast<MemberExpr>(StructBase)) {
+    LValue LV = EmitMemberExpr(ME);
+    Address Addr = LV.getAddress();
+    Res = Addr.emitRawPointer(*this);
   } else {
     return nullptr;
   }
diff --git a/clang/test/CodeGen/attr-counted-by-pr110385.c b/clang/test/CodeGen/attr-counted-by-pr110385.c
new file mode 100644
index 00000000000000..e120dcc583578d
--- /dev/null
+++ b/clang/test/CodeGen/attr-counted-by-pr110385.c
@@ -0,0 +1,62 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 4
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -O2 -Wno-missing-declarations -emit-llvm -o - %s | FileCheck %s
+
+// See #110385
+// Based on reproducer from Kees Cook:
+// https://lore.kernel.org/all/202409170436.C3C6E7F7A@keescook/
+
+struct variable {
+        int a;
+        int b;
+        int length;
+        short array[] __attribute__((counted_by(length)));
+};
+
+struct bucket {
+        int a;
+        struct variable *growable;
+        int b;
+};
+
+struct bucket2 {
+        int a;
+        struct variable growable;
+};
+
+void init(void * __attribute__((pass_dynamic_object_size(0))));
+
+// CHECK-LABEL: define dso_local void @test1(
+// CHECK-SAME: ptr nocapture noundef readonly [[FOO:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[GROWABLE:%.*]] = getelementptr inbounds nuw i8, ptr [[FOO]], i64 8
+// CHECK-NEXT:    [[TMP0:%.*]] = load ptr, ptr [[GROWABLE]], align 8, !tbaa [[TBAA2:![0-9]+]]
+// CHECK-NEXT:    [[ARRAY:%.*]] = getelementptr inbounds nuw i8, ptr [[TMP0]], i64 12
+// CHECK-NEXT:    [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[TMP0]], i64 8
+// CHECK-NEXT:    [[DOT_COUNTED_BY_LOAD:%.*]] = load i32, ptr [[DOT_COUNTED_BY_GEP]], align 4
+// CHECK-NEXT:    [[TMP1:%.*]] = sext i32 [[DOT_COUNTED_BY_LOAD]] to i64
+// CHECK-NEXT:    [[TMP2:%.*]] = shl nsw i64 [[TMP1]], 1
+// CHECK-NEXT:    [[TMP3:%.*]] = icmp sgt i32 [[DOT_COUNTED_BY_LOAD]], -1
+// CHECK-NEXT:    [[TMP4:%.*]] = select i1 [[TMP3]], i64 [[TMP2]], i64 0
+// CHECK-NEXT:    tail call void @init(ptr noundef nonnull [[ARRAY]], i64 noundef [[TMP4]]) #[[ATTR2:[0-9]+]]
+// CHECK-NEXT:    ret void
+//
+void test1(struct bucket *foo) {
+        init(foo->growable->array);
+}
+
+// CHECK-LABEL: define dso_local void @test2(
+// CHECK-SAME: ptr noundef [[FOO:%.*]]) local_unnamed_addr #[[ATTR0]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:    [[ARRAY:%.*]] = getelementptr inbounds nuw i8, ptr [[FOO]], i64 16
+// CHECK-NEXT:    [[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr [[FOO]], i64 12
+// CHECK-NEXT:    [[DOT_COUNTED_BY_LOAD:%.*]] = load i32, ptr [[DOT_COUNTED_BY_GEP]], align 4
+// CHECK-NEXT:    [[TMP0:%.*]] = sext i32 [[DOT_COUNTED_BY_LOAD]] to i64
+// CHECK-NEXT:    [[TMP1:%.*]] = shl nsw i64 [[TMP0]], 1
+// CHECK-NEXT:    [[TMP2:%.*]] = icmp sgt i32 [[DOT_COUNTED_BY_LOAD]], -1
+// CHECK-NEXT:    [[TMP3:%.*]] = select i1 [[TMP2]], i64 [[TMP1]], i64 0
+// CHECK-NEXT:    tail call void @init(ptr noundef nonnull [[ARRAY]], i64 noundef [[TMP3]]) #[[ATTR2]]
+// CHECK-NEXT:    ret void
+//
+void test2(struct bucket2 *foo) {
+        init(foo->growable.array);
+}

[efriedma-quic]

(See discussion on #110487)

[Cydox]

Removed the special case for DeclRefExpr, as this can also be handled by the pointer case. A few tests were adjusted, but none of them changed any behavior.

That additional commit is not necessary to fix anything, it's just cleanup.

[efriedma-quic]

StructAccessBase has two possible results: it finds an lvalue that's the base, which can't be peeled apart any further, or it finds a pointer that's the base. It shouldn't be possible to confuse a pointer with a MemberExpr: a MemberExpr is an lvalue, and a pointer is, in this context, an rvalue.

So I think the issue here is that StructAccessBase is returning the wrong result, and trying to paper over that is going to lead to issues that are harder to track down later.

[Cydox]

StructAccessBase has two possible results: it finds an lvalue that's the base, which can't be peeled apart any further, or it finds a pointer that's the base. It shouldn't be possible to confuse a pointer with a MemberExpr: a MemberExpr is an lvalue, and a pointer is, in this context, an rvalue.

To confirm:
Are you saying that StructBase can't be both a MemberExpr and a pointer?

For context: I'm not familiar with the clang/llvm internals before I looked into this issue

[Cydox]

Alright I think I'm getting what you mean with skipping over the LValueToRValue cast. I'll try to put a better fix together...

[bwendling]

The problem we're faced with here is that the Base pointer could point to anywhere within the structure. We already jump through several hoops to get the flexible array member's Decl and the counter's Decl.

So because Base could be a pointer to anywhere in the struct, we have to determine where it's pointing. If it's a pointer to the beginning of the struct (i.e. __builtin_dynamic_object_size(ptr, 0)), then we expect a DeclRefExpr from the visitor. But we could have something like this:

struct s {
  struct s *p;
  /* ... */
} *ptr;

__builtin_dynamic_object_size(ptr->p->p->p->p, 0);

That's caught as a MemberExpr. We can have the __bdos pointing somewhere within the flexible array member. Etc., etc. Notice that we can't rely upon looking at a CastExpr's type to determine exactly what we're looking at.

What this code is therefore trying to do is simply find a starting point (some Expr) from which to build a GEP to the counted_by field. It does that in part by using the CountDecl's outer lexical record context to stop when we've found the correct base (note that the counted_by's outer lexical record context has already been checked to be the same as the flexible array member's outer lexical record context).

I'm not sure what you mean by "or it finds a pointer that's the base". The MemberExpr the visitor returns doesn't have to be a pointer---e.g. ptr->a.b.c.d returning ptr->a.b, where b isn't a pointer---or do you mean something else?

[efriedma-quic]

I'm not sure what you mean by "or it finds a pointer that's the base". The MemberExpr the visitor returns doesn't have to be a pointer---e.g. ptr->a.b.c.d returning ptr->a.b, where b isn't a pointer---or do you mean something else?

In an expression which does not contain "->" operators, StructAccessBase should return an lvalue: the expression which describes the base of the relevant struct. There might be addressof/dereference/array decay/etc. in the middle, but that's the ultimate result. The "or it finds a pointer" bit isn't relevant in this case.

"LHS->member" is equivalent to "(*LHS).member"... and sometimes StructAccessBase wants to return "*LHS". This is where the "or it finds a pointer" bit comes in; there is no expression *LHS in the AST, so it returns a pointer which needs to be dereferenced to turn it into an lvalue.

[Cydox]

I'm gonna push something real quick to see if I understand where you want us to go. Didn't check if this breaks any tests yet.

[Cydox]

@efriedma-quic do you mean somthing along those lines?

[Cydox]

So, this doesn't fully work yet. When compiling the kernel it seems like there are cases where StructBase is now both an LValue and a pointer. Still investigating.

[Cydox]

Yeah so the problem is if you do __builtin_dynamic_object_size(v, 0)

In that case it's a DeclRefExpr, a pointer, and an LValue.

Easy fix is to check if StructBase is a pointer instead of checking for LValue, but then I don't really see the point of this change vs my originial PR to be honest (maybe I'm missing something).

[Cydox]

should I add a check for StructBase->isLValue() and add an else statement that returns nullptr?

[Cydox]

added that extra check

[Cydox]

Now this passes all tests again, also compiles the kernel without issue.

[bwendling]

Thanks for the patch!

[github-actions]

@Cydox Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!