[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote: Changing the `struct_size` macro in the kernel [1] would likely be an unreasonable amount of work. To quote Kees from the kernel mailing list [2]: > [...] if we want to change struct_size(), then we must (via allmodconfig builds) determine all the places in the kernel where the calculated size changes, and audit those for safety. Additionally I think the kernel's code is not unreasonable as this is a common way to allocate structs with flexible array members. It's even how the example in the C11 standard does it. So it's likely that other projects are also expecting the same behavior. I think it would be way easier to get clang to follow what the kernel currently expects. While I think clang's current behavior is not quite correct and the maximum size should be calculated differently [3], that doesn't solve the compatibility with the linux kernel for all cases. So ideally we should introduce the behavior that this PR calls for: ``` sizeof(struct s) + p->count * sizeof(*p->array)) ``` via an option. I see a few ways this could be accomplished: 1. a global `-f` flag 2. adding the flag as the third bit of the `type` parameter to `__bdos` 3. add a separate builtin I prefer option 2. Should this be coordinated with gcc? Currently they don't implement `counted_by` for this case at all, but I don't know if they have plans to do so. I'm happy to adjust this PR to hide it behind an option. [1] https://github.com/torvalds/linux/blob/b983b271662bd6104d429b0fd97afba760bf/include/linux/overflow.h#L354-L373 [2] https://lore.kernel.org/linux-bcachefs/202410040958.C19D3B9E48@keescook/ [3] https://lore.kernel.org/linux-bcachefs/ZwNb-_UPL9BPSg9N@archlinux/#t https://github.com/llvm/llvm-project/pull/111015 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote: ## Picture of the sizes involved to scale: ``` <-malloc-max--> <-malloc-min---> <--sizeof(posix_acl)---> <-FAME-><(FAME)> ``` `FAME` = flexible array member element (aka `struct posix_acl_entry`) `(FAME)` = hypothetical 2nd `FAME` ## Based on the C standard what do we know if `a_count` = 1? ### size of object `acl` points to The size of the `acl` object is at least big enough to hold a single `FAME` (`malloc-min`). We also know that the largest size of the `acl` object is 1 byte smaller than would be necessary to hold 2 `FAME`s (`malloc-max`), because if it was larger `a_count` should be 2 (or larger). ### size of the flexible array element We also know that the size of the flexible array member itself is always `a_count` multiplied by `sizeof(struct posix_acl_entry)`. No matter how much extra padding there is beyond the last `FAME` that would be padding in the struct, and not part of the flexible array member. ## What should `__bdos` return? Here's what `__bdos` should be returning according to the documentation: https://github.com/llvm/llvm-project/blob/3b88805ca20018ae202afd3aea39f4fa856a8c64/clang/docs/LanguageExtensions.rst?plain=1#L5502-L5507 ### `__bdos(acl, 0)`: What is "the least `n` [...] such that accesses to `(const char*)ptr + n` and beyond are **known** to be out of bounds?" `n = malloc-max = offsetof(struct posix_acl, a_entries) + (acl->a_count + 1) * sizeof(struct posix_acl_entry) - 1 = 43` If `alloc_size` is known that might be smaller. ### `__bdos(acl, 2)`: What is "the greatest `n` [...] such that accesses to `(const char*)ptr + i` are *known* to be in bounds, for 0 <= `i` < `n`?" `n = malloc-min = offsetof(struct posix_acl, a_entries) + acl->a_count * sizeof(struct posix_acl_entry) = 36` If `alloc_size` is known that might be bigger. ### `__bdos(acl->a_entries, 0)` and `__bdos(acl->a_entries, 2)`: Both should be: `n = acl->a_count * sizeof(struct posix_acl_entry)` https://github.com/llvm/llvm-project/pull/111015 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
https://github.com/Cydox converted_to_draft https://github.com/llvm/llvm-project/pull/111015 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote:
> AIUI, GCC considers this to be a bug in the face of `counted_by`. The reason
> GCC returns "unknown" (`SIZE_MAX`) in the case of a pointer like that is
> because (prior to `counted_by`) if the pointer wasn't to local storage, it
> could no know if it was a singleton or not. i.e. it may be pointing into a
> larger array of objects, so it cannot know the size. (This is most clear for
> `char *` variables, for example.)
> This shows how GCC will adjust the `__bdos` when it is certain of the object
> being a singleton, but it still missing the "counted_by requires a singleton"
> logic. (See also `-Wflexible-array-member-not-at-end` in GCC). It also shows
> that Clang's `__bdos` can be lied to.
You can also lie to gcc if you do `__bdos(p->array, 1)`:
https://godbolt.org/z/ME5bd7nr9
```C
// gcc and clang: 40
void local_storage_lies_fam(void)
{
struct foo local = { .counter = 10 };
struct foo *p = &local;
emit_length(__builtin_dynamic_object_size(p->array, 1));
}
```
I did some more digging and at least the comments in the tests from gcc seem to
confirm the difference in behavior between lookind at `__bdos` of the FAM vs
the whole struct:
Whole struct only uses the size from a known call to malloc, while bdos on FAM
uses the counted_by attribute.
https://github.com/gcc-mirror/gcc/blob/3f10a2421c2b9c41e7c1b1c0d956743709f5d0be/gcc/testsuite/gcc.dg/flex-array-counted-by-4.c#L113-L141:
```C
p->foo = index + SIZE_BUMP;
/* When checking the observed access p->array, we have info on both
observered allocation and observed access,
A.1 from observed allocation:
allocated_size - offsetof (struct annotated, array[0])
A.2 from the counted-by attribute:
p->foo * sizeof (char)
We always use the latest value that is hold by the counted-by field.
*/
EXPECT(__builtin_dynamic_object_size(p->array, 0),
(p->foo) * sizeof(char));
EXPECT(__builtin_dynamic_object_size(p->array, 1),
(p->foo) * sizeof(char));
EXPECT(__builtin_dynamic_object_size(p->array, 2),
(p->foo) * sizeof(char));
EXPECT(__builtin_dynamic_object_size(p->array, 3),
(p->foo) * sizeof(char));
/* When checking the pointer p, we only have info on the observed
allocation. So, the object size info can only been obtained from
the call to malloc. */
EXPECT(__builtin_dynamic_object_size(p, 0), allocated_size);
EXPECT(__builtin_dynamic_object_size(p, 1), allocated_size);
EXPECT(__builtin_dynamic_object_size(p, 2), allocated_size);
EXPECT(__builtin_dynamic_object_size(p, 3), allocated_size);
```
clang on the other hand will always do it's counted_by calculation if the
attribute is there. Both for bdos on the FAM, as well as bdos on the whole
struct.
https://github.com/llvm/llvm-project/pull/111015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
Cydox wrote:
> > 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`.
>
> Can you give a more complete example? I just tried the following, and I see
> an lvaluetorvalue cast.
>
> ```
> int f(const void *p) { return __builtin_dynamic_object_size(p, 0); }
> ```
```C
// test2.c
#include
#include
#include
struct variable {
int a;
int b;
int length;
short array[] __attribute__((counted_by(length)));
};
int main(int argc, char *argv[])
{
struct variable *v;
v = malloc(sizeof(struct variable) + sizeof(short) * 32);
v->length = 32;
printf("%zu\n", __builtin_dynamic_object_size(v, 0));
return 0;
}
```
I added this `StructBase->dump`:
```
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 4aca60685f37..7a06819f1a67 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -1162,6 +1162,8 @@ llvm::Value *CodeGenFunction::EmitLoadOfCountedByField(
if (!StructBase || StructBase->HasSideEffects(getContext()))
return nullptr;
+ StructBase->dump();
+
llvm::Value *Res = nullptr;
if (StructBase->getType()->isPointerType()) {
LValueBaseInfo BaseInfo;
```
```
$ clang test2.c
DeclRefExpr 0x34b302c8 'struct variable *' lvalue Var 0x34b2fce8 'v' 'struct
variable *'
$ ./a.out
76
```
In my testing I also added a print statement that printed a dump when both
`StructBase->getType()->isPointerType()` and `StructBase->isLValue()` were true
and there were a bunch of hits when compiling the kernel.
https://github.com/llvm/llvm-project/pull/110497
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110437)
Cydox wrote: @bwendling please review https://github.com/llvm/llvm-project/pull/110437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110437)
https://github.com/Cydox created
https://github.com/llvm/llvm-project/pull/110437
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.
>From 26aaa04f5570d05f15e0ac9882870467b89c2690 Mon Sep 17 00:00:00 2001
From: Jan Hendrik Farr
Date: Sun, 29 Sep 2024 21:38:13 +0200
Subject: [PATCH] [Clang] Fix 'counted_by' for nested struct pointers
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.
---
clang/lib/CodeGen/CGExpr.cpp | 8 ++--
clang/test/CodeGen/attr-counted-by-pr110385.c | 40 +++
2 files changed, 44 insertions(+), 4 deletions(-)
create mode 100644 clang/test/CodeGen/attr-counted-by-pr110385.c
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(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(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 00..49a08c5965ef94
--- /dev/null
+++ b/clang/test/CodeGen/attr-counted-by-pr110385.c
@@ -0,0 +1,40 @@
+// 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;
+};
+
+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);
+}
___
c
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110437)
https://github.com/Cydox updated
https://github.com/llvm/llvm-project/pull/110437
>From e1ef2156ed4a0f3ccd1d6dca97faa014284e4605 Mon Sep 17 00:00:00 2001
From: Jan Hendrik Farr
Date: Sun, 29 Sep 2024 21:38:13 +0200
Subject: [PATCH] [Clang] Fix 'counted_by' for nested struct pointers
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.
---
clang/lib/CodeGen/CGExpr.cpp | 8 ++--
clang/test/CodeGen/attr-counted-by-pr110385.c | 40 +++
2 files changed, 44 insertions(+), 4 deletions(-)
create mode 100644 clang/test/CodeGen/attr-counted-by-pr110385.c
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(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(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 00..49a08c5965ef94
--- /dev/null
+++ b/clang/test/CodeGen/attr-counted-by-pr110385.c
@@ -0,0 +1,40 @@
+// 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;
+};
+
+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);
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Emit load of GEP after EmitMemberExpr (PR #110487)
Cydox wrote: Interesting. You compiled with `-O2`, I compiled without optimization. When I compile it with `-O2`, I now get `0`. I've uploaded a gist with the disassembly generated with ``` clang -mllvm --x86-asm-syntax=intel -S test.c -o ./test-no-optimize.S ``` and ``` clang -O2 -mllvm --x86-asm-syntax=intel -S test.c -o ./test-optimize-O2.S ``` Can you do the same? https://gist.github.com/Cydox/8b48a7d36dbcf6376d5a2d44afe5cf57 https://github.com/llvm/llvm-project/pull/110487 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Emit load of GEP after EmitMemberExpr (PR #110487)
Cydox wrote: @bwendling I think you accidentally compiled the wrong branch. Looks like that is in `builtin_get_counted_by`, but this PR is in `bdos-member-expr-error` https://github.com/llvm/llvm-project/pull/110487 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Disable use of the counted_by attribute for whole struct pointers (PR #112636)
https://github.com/Cydox approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/112636 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Emit load of GEP after EmitMemberExpr (PR #110487)
Cydox wrote: No worries https://github.com/llvm/llvm-project/pull/110487 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote: >From page 113 and 114 of: >https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf "As a special case, the last element of a structure with more than one named member may have an incomplete array type; this is called a flexible array member. In most situations, the flexible array member is ignored. In particular, the size of the structure is as if the flexible array member were omitted except that it may have more trailing padding than the omission would imply. However, when a . (or ->) operator has a left operand that is (a pointer to) a structure with a flexible array member and the right operand names that member, **it behaves as if that member were replaced with the longest array (with the same element type) that would not make the structure larger than the object being accessed**; the offset of the array shall remain that of the flexible array member, even if this would differ from that of the replacement array. If this array would have no elements, it behaves as if it had one element but the behavior is undefined if any attempt is made to access that element or to generate a pointer one past it." [emphasis added] The struct object that has an FAM behaves like if the FAM was replaced with the largest array that would not the object larger than it is (if the . or -> operator is used). Clang's behavior is currently returning just the size of the object that it would be if the FAM was replaced with that largest possible array (and the object only behaves like that when the . or -> operator is used). So when `__builtin_dynamic_object_size(acl, 0)` returns 36 in the reproducer that is definitely not according to the standard. 1. The standard makes clear that the size of the object `acl` points to is `>=` than the struct it behaves like. 2. There is not even a `.` or `->` operator involved here. GCC's behavior thus makes sense. It does not use `__counted_by` attributes in the determination of `__builtin_dynamic_object_size(acl, 0)` at all. It either uses an allocation size known from the context or returns the maximum possible value to indicate that it does not know the size of the object. Returning `sizeof(struct s) + p->count * sizeof(*p->array))` instead of the INT_MAX would be a compromise as we definitely know that the object is at most that large. https://github.com/llvm/llvm-project/pull/111015 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote:
After looking at the assembly produced by gcc more, it actually looks like it's
using the allocation size if it's known in the current context (for example if
the struct was just malloced in the same function) and otherwise returns
INT_MAX for the __bdos of a struct containing a flexible array member. It's
only returning the size based on the __counted_by attribute of you ask it for
the __bdos of the flexible array member itself.
```C
int test(struct posix_acl *acl) {
return __builtin_dynamic_object_size(acl, 0);
}
```
actually compiles to
```
test:
mov eax, -1
ret
```
using gcc (trunk) on compiler explorer.
So the call to kmemdup in this function:
```C
struct posix_acl *
posix_acl_clone(const struct posix_acl *acl, gfp_t flags)
{
struct posix_acl *clone = NULL;
if (acl) {
int size = offsetof(struct posix_acl, a_entries) + acl->a_count
*
sizeof(struct posix_acl_entry);
clone = kmemdup(acl, size, flags);
if (clone)
refcount_set(&clone->a_refcount, 1);
}
return clone;
}
EXPORT_SYMBOL_GPL(posix_acl_clone);
```
won't actually be bounds-checked currently when compiling the kernel with gcc.
Compiling with clang will make this bounds-checked using the lower bound of
possible sizes (max(sizeof(struct s), offsetof(struct s, array) + p->count *
sizeof(*p->array)))
https://github.com/llvm/llvm-project/pull/111015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote:
So, we would actually get gcc's behavior with this patch:
```
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index c864714182e0..21ffe7b46a6e 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -1049,25 +1049,7 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr
*E, unsigned Type,
Value *Res = FAMSize;
if (isa(Base)) {
-// The whole struct is specificed in the __bdos.
-const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(OuterRD);
-
-// Get the offset of the FAM.
-llvm::Constant *FAMOffset = ConstantInt::get(ResType, Offset, IsSigned);
-Value *OffsetAndFAMSize =
-Builder.CreateAdd(FAMOffset, Res, "", !IsSigned, IsSigned);
-
-// Get the full size of the struct.
-llvm::Constant *SizeofStruct =
-ConstantInt::get(ResType, Layout.getSize().getQuantity(), IsSigned);
-
-// max(sizeof(struct s),
-// offsetof(struct s, array) + p->count * sizeof(*p->array))
-Res = IsSigned
- ? Builder.CreateBinaryIntrinsic(llvm::Intrinsic::smax,
- OffsetAndFAMSize, SizeofStruct)
- : Builder.CreateBinaryIntrinsic(llvm::Intrinsic::umax,
- OffsetAndFAMSize, SizeofStruct);
+return nullptr;
}
// A negative \p IdxInst or \p CountedByInst means that the index lands
```
https://github.com/llvm/llvm-project/pull/111015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote:
Also see these comments in the main function of the same test file:
```C
int main ()
{
struct annotated *p, *q;
p = alloc_buf_more (10);
q = alloc_buf_less (10);
/* When checking the access p->array, we only have info on the counted-by
value. */
EXPECT(__builtin_dynamic_object_size(p->array, 0), p->foo * sizeof(char));
EXPECT(__builtin_dynamic_object_size(p->array, 1), p->foo * sizeof(char));
EXPECT(__builtin_dynamic_object_size(p->array, 2), p->foo * sizeof(char));
EXPECT(__builtin_dynamic_object_size(p->array, 3), p->foo * sizeof(char));
/* When checking the pointer p, we have no observed allocation nor observed
access, therefore, we cannot determine the size info here. */
EXPECT(__builtin_dynamic_object_size(p, 0), -1);
EXPECT(__builtin_dynamic_object_size(p, 1), -1);
EXPECT(__builtin_dynamic_object_size(p, 2), 0);
EXPECT(__builtin_dynamic_object_size(p, 3), 0);
/* When checking the access p->array, we only have info on the counted-by
value. */
EXPECT(__builtin_dynamic_object_size(q->array, 0), q->foo * sizeof(char));
EXPECT(__builtin_dynamic_object_size(q->array, 1), q->foo * sizeof(char));
EXPECT(__builtin_dynamic_object_size(q->array, 2), q->foo * sizeof(char));
EXPECT(__builtin_dynamic_object_size(q->array, 3), q->foo * sizeof(char));
/* When checking the pointer p, we have no observed allocation nor observed
access, therefore, we cannot determine the size info here. */
EXPECT(__builtin_dynamic_object_size(q, 0), -1);
EXPECT(__builtin_dynamic_object_size(q, 1), -1);
EXPECT(__builtin_dynamic_object_size(q, 2), 0);
EXPECT(__builtin_dynamic_object_size(q, 3), 0);
DONE ();
}
```
https://github.com/llvm/llvm-project/pull/111015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote: > The point of __counted_by is precisely to supplement the normal standard > rules: specifically, if you have counted_by, the size of the flexible array > is precisely the size specified by the attribute. Not whatever size is > implied by the access. Otherwise, it would be illegal for __bdos to use the > counted_by attribute at all. The size of the array can't change based on how > __bdos is queried. Let's say we simply define a `count` that is inconsistent with the size of the object as undefined behavior. I agree that the size of the flexible array member when the __counted_by attribute is used is exactly count multiplied by size of each element. And this PR changes none of that. ```C __builtin_dynamic_object_size(acl->a_entries) ``` Still does exactly that and therefore bounds-checking even an off-by-1 scenario will work correctly. This PR only changes the behavior of: ```C __builtin_dynamic_object_size(acl) ``` Here you are not asking for the size of the FAM, but of the whole struct object. The size of that object is whatever you gave to `malloc`. **There are multiple possible sizes you could have given to `malloc` that are consistent with the same `count`**, because the standard says that "it behaves as if that member were replaced with the longest array (with the same element type) that would not make the structure larger than the object being accessed". As long as the elements of the array are larger than 1 byte there are multiple `malloc` sizes that have the same correct `count`. This is different when you ask for the size of the FAM itself, each size of the FAM only has a single correct value of `count`. This is why I believe the gcc behavior is correct. When it knows the size given to `malloc` it uses that. When it doesn't know that it simply returns INT_MAX. When you ask gcc for the `__bdos` of the FAM it will use the `count` to calculate the size. > `sizeof(struct s) + p->count * sizeof(*p->array))` is a weird compromise: > it's not unbounded, but it's larger than the size specified by the standard. I agree that it is a weird compromise, I'm thinking clang should to with the gcc behavior here instead. --- However: > but it's larger than the size specified by the standard Can you tell me what the size specified by the standard is? Let's say for these two examples: (1) ``` struct posix_acl *acl = malloc(sizeof(struct posix_acl) + sizeof(struct posix_acl_entry) * 1); acl.a_count = 1; ``` (2) ``` struct posix_acl *acl = malloc(offsetof(struct posix_acl, a_entries) + sizeof(struct posix_acl_entry) * 1); acl.a_count = 1; ``` What is the size of the object `acl` points to according to the C standard? My answers: (1): 40 (2): 36 https://github.com/llvm/llvm-project/pull/111015 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote:
I'm gonna write a better explanation with some drawings of my arguments in a
bit, but to start of with, clang's answer is inconsistent.
If you rewrite the reproducer in the issue to this version without the
`__attribute__((counted_by(a_count)))`, and compile with `-O2`, clang says that
the size is now 40 instead of 36:
```C
#include
#include
typedef struct {
int counter;
} atomic_t;
typedef struct refcount_struct {
atomic_t refs;
} refcount_t;
struct callback_head {
struct callback_head *next;
void (*func)(struct callback_head *head);
} __attribute__((aligned(sizeof(void *;
#define rcu_head callback_head
typedef struct {
uid_t val;
} kuid_t;
typedef struct {
gid_t val;
} kgid_t;
struct posix_acl_entry {
short e_tag;
unsigned short e_perm;
union {
kuid_t e_uid;
kgid_t e_gid;
};
};
struct posix_acl {
refcount_t a_refcount;
struct rcu_head a_rcu;
unsigned int a_count;
struct posix_acl_entry a_entries[];
};
int main() {
unsigned int count = 1;
struct posix_acl *acl;
acl = malloc(sizeof(struct posix_acl) +
sizeof(struct posix_acl_entry) * count);
acl->a_count = count;
acl->a_entries[0].e_tag = 0x1;
printf("%zu\n", __builtin_dynamic_object_size(acl, 0));
printf("%zu\n", __builtin_dynamic_object_size(acl->a_entries, 0));
return 0;
}
```
```
$ clang -O2 test.c
$ ./a.out
40
```
Compiling the original reproducer in #111009 (only change is the counted_by
attribute) you get this:
```
$ clang -O2 test.c
$ ./a.out
36
```
https://github.com/llvm/llvm-project/pull/111015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote: > My default stance would be that gcc and the Linux code in question are wrong. > We could reconsider if strict checking is impractical for Linux, but I'd > expect kernel devs to prefer catching accesses one past the end of the array. If you access the array none of this really matters, as you wouldn't be dealing with `__builtin_dynamic_object_size(acl, 0)`, but instead `__builtin_dynamic_object_size(acl->a_entries, 0)`, which with this fix still computes count multiplied with the size of an array element. https://github.com/llvm/llvm-project/pull/111015 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote: I slightly changed my mind. Was gonna write that gcc is definitely right, now I'm more at a 50/50. I got two arguments, one from a standards perspective, the other from practical/safety perspective. ## Standards argument: When you do ```C acl = malloc(sizeof(struct posix_acl) + sizeof(struct posix_acl_entry) * 1); acl.a_count = 1; ``` The size of the object that got created there is in fact 40 (feel free to correct me here). The description of `malloc` in the standard says: "The malloc function allocates space for an object whose size is specified by size and whose value is indeterminate." But you could also do ```C acl = malloc(offsetof(struct posix_acl, a_entries) + sizeof(struct posix_acl_entry) * 1); ``` in which case the size of that object is 36. For __bdos we don't know which situation we're in though. ## Practical/Safety argument: It comes down to these 4 cases. A ``` struct posix_acl *acl = malloc(sizeof(struct posix_acl) + sizeof(struct posix_acl_entry) * 1); acl.a_count = 1; memcpy(somewhere, acl, sizeof(struct posix_acl) + sizeof(struct posix_acl_entry) * 1) ``` B ``` struct posix_acl *acl = malloc(sizeof(struct posix_acl) + sizeof(struct posix_acl_entry) * 1); acl.a_count = 1; memcpy(somewhere, acl, offsetof(struct posix_acl, a_entries) + sizeof(struct posix_acl_entry) * 1) ``` C ``` struct posix_acl *acl = malloc(offsetof(struct posix_acl, a_entries) + sizeof(struct posix_acl_entry) * 1); acl.a_count = 1; memcpy(somewhere, acl, sizeof(struct posix_acl) + sizeof(struct posix_acl_entry) * 1) ``` D ``` struct posix_acl *acl = malloc(offsetof(struct posix_acl, a_entries) + sizeof(struct posix_acl_entry) * 1); acl.a_count = 1; memcpy(somewhere, acl, offsetof(struct posix_acl, a_entries) + sizeof(struct posix_acl_entry) * 1) ``` Only C is undefined behavior. With gcc's behavior none of these cases fail the bounds-check, but C is undefined behavior. With clang's behvaior A and C fail the bounds-check, even though A is perfectly safe. So it comes down to whether you want false positives or false negatives. Sticking to the current behavior will lead to random crashes of otherwise safe code that might only trigger occasionally. Code like this will probably lurk for quite a while, especially because gcc has different behavior. Changing the behavior on the other hand will mean not all cases of C might be caught. Which is the correct way to go here is neither obvious nor my call to make. I think we could find a way to scan the kernel for all the A cases and convert them to B or D. I would like to know if any of you disagree or agree with the correct size of the object from the standards perspective. https://github.com/llvm/llvm-project/pull/111015 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Disable use of the counted_by attribute for whole struct pointers (PR #112636)
Cydox wrote: Also see previous discussion here: https://github.com/llvm/llvm-project/pull/111015 https://github.com/llvm/llvm-project/pull/112636 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Disable use of the counted_by attribute for whole struct pointers (PR #112636)
Cydox wrote: I was forgetting about the padding when thinking about this part of the quote: > it behaves as if that member were replaced with the longest array (with the > same element type) that would not make **the structure** larger than the > object being accessed https://github.com/llvm/llvm-project/pull/112636 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Disable use of the counted_by attribute for whole struct pointers (PR #112636)
Cydox wrote: > For the record, I think the most correct definition, in terms of "this is how > much memory you should allocate for a struct with a flexible array member" is > this: > > ```c > max( > sizeof(struct S), // always at least the size of the struct itself > round_up( > alignof(struct S), // size must be a multiple of alignment > offsetof(struct S, fam) + count * sizeof(((struct S *)0)->fam[0]) > ) > ) > ``` I mean it would be useful to round up to the alignment for when you wanne have an array of the structs, but I'm not sure this is actually required by the standard. Do you have more justification for the alignment requirement on structs containing FAMs? I would argue the minimum "legal" amount to allocate is: ```C max( sizeof(struct S), offsetof(struct S, fam) + count * sizeof(((struct S *)0)->fam[0]) ) ``` https://github.com/llvm/llvm-project/pull/112636 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Disable use of the counted_by attribute for whole struct pointers (PR #112636)
Cydox wrote: I think you're correct https://github.com/llvm/llvm-project/pull/112636 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Disable use of the counted_by attribute for whole struct pointers (PR #112636)
Cydox wrote: But for now, I think we should merge #112786 into 19.1.3 so that the we can have that be the cutoff in the kernel. https://github.com/llvm/llvm-project/pull/112636 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Disable use of the counted_by attribute for whole struct pointers (PR #112636)
Cydox wrote: The expression can be simplified to: ```C round_up( alignof(struct S) offsetof(struct S, fam) + count * sizeof(((struct S *)0)->fam[0]) ) ``` As the padding at the end of the structure is always smaller than the alignof. So `round_up(alignof(struct S), offsetof(struct S, fam)) = sizeof(struct S)` https://github.com/llvm/llvm-project/pull/112636 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Disable use of the counted_by attribute for whole struct pointers (PR #112636)
Cydox wrote: @bwendling What do you think about that? https://github.com/llvm/llvm-project/pull/112636 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Emit load of GEP after EmitMemberExpr (PR #110487)
Cydox wrote: Fixes the kernel issue for me. I closed #110437 in favor of this one https://github.com/llvm/llvm-project/pull/110487 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110437)
Cydox wrote: Closing in favor of #110487 https://github.com/llvm/llvm-project/pull/110437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110437)
https://github.com/Cydox closed https://github.com/llvm/llvm-project/pull/110437 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Emit load of GEP after EmitMemberExpr (PR #110487)
Cydox wrote:
Wait, this introduces a regression when the inner struct is directly nested
without using a pointer like so:
Without this change the code below will return 64, with my fix it will also
return 64, with this fix it will SEGFAULT.
```C
#include
#include
#include
struct variable {
int a;
int b;
int length;
short array[] __attribute__((counted_by(length)));
};
struct bucket {
int a;
struct variable growable;
//struct variable *growable;
//int b;
};
int main(int argc, char *argv[])
{
struct bucket *p;
struct variable *v;
p = malloc(sizeof(*p) + sizeof(*p->growable.array) * 32);
p->growable.length = 32;
//printf("%zu\n", __builtin_dynamic_object_size(v->array, 1));
//p->growable = v;
printf("%zu\n", __builtin_dynamic_object_size(p->growable.array, 1));
return 0;
}
```
https://github.com/llvm/llvm-project/pull/110487
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
https://github.com/Cydox created
https://github.com/llvm/llvm-project/pull/110497
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.
>From 15b69329a97706ada7d5e8cbeb76508aa55b418f Mon Sep 17 00:00:00 2001
From: Jan Hendrik Farr
Date: Sun, 29 Sep 2024 21:38:13 +0200
Subject: [PATCH] [Clang] Fix 'counted_by' for nested struct pointers
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.
---
clang/lib/CodeGen/CGExpr.cpp | 8 +--
clang/test/CodeGen/attr-counted-by-pr110385.c | 62 +++
2 files changed, 66 insertions(+), 4 deletions(-)
create mode 100644 clang/test/CodeGen/attr-counted-by-pr110385.c
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(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(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 00..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) {
+
[clang] [Clang][CodeGen] Emit load of GEP after EmitMemberExpr (PR #110487)
Cydox wrote: Reopened my fix as #110497 and also adjusted the test to also check for the case of a nested struct without indirection. https://github.com/llvm/llvm-project/pull/110487 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Emit load of GEP after EmitMemberExpr (PR #110487)
Cydox wrote:
> I went ahead and used the `EmitPointerWithAlignment` for all (it worked
> without needing special casing). I'll look into doing this in Sema as a
> potential cleanup.
With that change now the compiler segfaults when compiling the example C file I
posted above:
> ```c
> #include
> #include
> #include
>
> struct variable {
> int a;
> int b;
> int length;
> short array[] __attribute__((counted_by(length)));
> };
>
> struct bucket {
> int a;
> struct variable growable;
> //struct variable *growable;
> //int b;
> };
>
> int main(int argc, char *argv[])
> {
> struct bucket *p;
> struct variable *v;
>
> p = malloc(sizeof(*p) + sizeof(*p->growable.array) * 32);
> p->growable.length = 32;
>
>
> //printf("%zu\n", __builtin_dynamic_object_size(v->array, 1));
>
> //p->growable = v;
> printf("%zu\n", __builtin_dynamic_object_size(p->growable.array, 1));
>
> return 0;
> }
> ```
https://github.com/llvm/llvm-project/pull/110487
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Emit load of GEP after EmitMemberExpr (PR #110487)
@@ -1160,23 +1162,10 @@ llvm::Value *CodeGenFunction::EmitLoadOfCountedByField(
if (!StructBase || StructBase->HasSideEffects(getContext()))
return nullptr;
- llvm::Value *Res = nullptr;
- if (const auto *DRE = dyn_cast(StructBase)) {
-Res = EmitDeclRefLValue(DRE).getPointer(*this);
-Res = Builder.CreateAlignedLoad(ConvertType(DRE->getType()), Res,
-getPointerAlign(), "dre.load");
- } else if (const MemberExpr *ME = dyn_cast(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 {
-return nullptr;
- }
+ LValueBaseInfo BaseInfo;
+ TBAAAccessInfo TBAAInfo;
+ Address Addr = EmitPointerWithAlignment(StructBase, &BaseInfo, &TBAAInfo);
Cydox wrote:
In the example C file that causes the compiler to segfault, `StructBase` is not
a pointer here, so this fails.
Here's a dump of StructBase:
```
MemberExpr 0x1240b890 'struct variable' lvalue ->growable 0x1240ac20
`-ImplicitCastExpr 0x1240b878 'struct bucket *'
`-DeclRefExpr 0x1240b858 'struct bucket *' lvalue Var 0x1240afe8 'p' 'struct
bucket *'
```
So you have to distinguish between the MemberExpr and the pointer case right
here, at which point you are basically back to my PR (although I guess the case
for `DeclRefExpr` can be removed additionally; that does break tests, so I'll
check carefully it the behavior is still correct with that case removed)
https://github.com/llvm/llvm-project/pull/110487
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
Cydox wrote: Removed the special case for DeclRefExpr, as this can also be handled by the pointer case. A few were adjusted, but none of them changed any behavior. That additional commit is not necessary to fix anything, it's just cleanup. https://github.com/llvm/llvm-project/pull/110497 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
https://github.com/Cydox updated
https://github.com/llvm/llvm-project/pull/110497
>From 15b69329a97706ada7d5e8cbeb76508aa55b418f Mon Sep 17 00:00:00 2001
From: Jan Hendrik Farr
Date: Sun, 29 Sep 2024 21:38:13 +0200
Subject: [PATCH 1/2] [Clang] Fix 'counted_by' for nested struct pointers
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.
---
clang/lib/CodeGen/CGExpr.cpp | 8 +--
clang/test/CodeGen/attr-counted-by-pr110385.c | 62 +++
2 files changed, 66 insertions(+), 4 deletions(-)
create mode 100644 clang/test/CodeGen/attr-counted-by-pr110385.c
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(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(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 00..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]
[clang] [Clang][CodeGen] Emit load of GEP after EmitMemberExpr (PR #110487)
Cydox wrote:
> I reverted my last commit. This leaves the original patch, which seems to
> work. @efriedma-quic, would you be okay with this patch while I work to
> improve the code in follow-up?
The original (and current) patch in this PR still introduces a regression. So
it should not be merged in my opinion.
Look at the following C file (`test.c`):
```C
#include
#include
#include
struct variable {
int a;
int b;
int length;
short array[] __attribute__((counted_by(length)));
};
struct bucket {
int a;
struct variable growable;
};
int main(int argc, char *argv[])
{
struct bucket *p;
p = malloc(sizeof(*p) + sizeof(*p->growable.array) * 32);
p->growable.length = 32;
printf("%zu\n", __builtin_dynamic_object_size(p->growable.array, 1));
return 0;
}
```
Compiling this file with clang 19.1.0 and running it:
```bash
$ clang --version
clang version 19.1.0 (Fedora 19.1.0-1.fc41)
Target: x86_64-redhat-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Configuration file: /etc/clang/x86_64-redhat-linux-gnu-clang.cfg
$ clang test.c
$ ./a.out
64
```
Correctly prints `64`.
But compiling and running it with the originial (and current) patch in this PR:
```bash
$ clang --version
clang version 20.0.0git (g...@github.com:llvm/llvm-project.git
2de76472e0d1417b64da5aa2c1138eb365685d3a)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /home/jan/llvm-project/build2/bin
$ clang test.c
$ ./a.out
Segmentation fault (core dumped)
```
Will result in a binary that crashes with a segmentation fault.
---
My PR #110497 adds a test case for this scenario and does not have the same
issue:
https://github.com/Cydox/llvm-project/blob/15b69329a97706ada7d5e8cbeb76508aa55b418f/clang/test/CodeGen/attr-counted-by-pr110385.c#L61
https://github.com/llvm/llvm-project/pull/110487
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
Cydox wrote: Alright I think I'm getting what you mean with skipping over the LValueToRValue cast. I'll try to put a better fix together... https://github.com/llvm/llvm-project/pull/110497 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
Cydox wrote: > 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 https://github.com/llvm/llvm-project/pull/110497 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
Cydox wrote: I'm gonna push something real quick to see if I understand were you want us to go. Didn't check if this breaks any tests yet. https://github.com/llvm/llvm-project/pull/110497 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
https://github.com/Cydox updated
https://github.com/llvm/llvm-project/pull/110497
>From 15b69329a97706ada7d5e8cbeb76508aa55b418f Mon Sep 17 00:00:00 2001
From: Jan Hendrik Farr
Date: Sun, 29 Sep 2024 21:38:13 +0200
Subject: [PATCH 1/3] [Clang] Fix 'counted_by' for nested struct pointers
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.
---
clang/lib/CodeGen/CGExpr.cpp | 8 +--
clang/test/CodeGen/attr-counted-by-pr110385.c | 62 +++
2 files changed, 66 insertions(+), 4 deletions(-)
create mode 100644 clang/test/CodeGen/attr-counted-by-pr110385.c
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(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(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 00..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]
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
Cydox wrote: @efriedma-quic do you mean somthing along those lines? https://github.com/llvm/llvm-project/pull/110497 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
@@ -10,6 +10,7 @@ // //===--===// +#include Cydox wrote: Yeah, this is just a quick commit to show you where my mind is going right now. Won't be in any final commit https://github.com/llvm/llvm-project/pull/110497 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
https://github.com/Cydox updated
https://github.com/llvm/llvm-project/pull/110497
>From 15b69329a97706ada7d5e8cbeb76508aa55b418f Mon Sep 17 00:00:00 2001
From: Jan Hendrik Farr
Date: Sun, 29 Sep 2024 21:38:13 +0200
Subject: [PATCH 1/4] [Clang] Fix 'counted_by' for nested struct pointers
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.
---
clang/lib/CodeGen/CGExpr.cpp | 8 +--
clang/test/CodeGen/attr-counted-by-pr110385.c | 62 +++
2 files changed, 66 insertions(+), 4 deletions(-)
create mode 100644 clang/test/CodeGen/attr-counted-by-pr110385.c
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(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(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 00..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]
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
@@ -1164,15 +1163,15 @@ llvm::Value *CodeGenFunction::EmitLoadOfCountedByField(
return nullptr;
llvm::Value *Res = nullptr;
- if (StructBase->isLValue()) {
-LValue LV = EmitLValue(StructBase);
-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 {
Cydox wrote:
should I add a check for `StructBase->isLValue()` and add an `else` statement
that returns `nullptr`?
https://github.com/llvm/llvm-project/pull/110497
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
Cydox wrote: So, this doesn't fully work yet. When compiling the kernel it seems like there are cases were `StructBase` is now both an LValue and a pointer. Still investigating. https://github.com/llvm/llvm-project/pull/110497 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
Cydox wrote: 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). https://github.com/llvm/llvm-project/pull/110497 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
Cydox wrote: Now this passes all tests again, also compiles the kernel without issue. https://github.com/llvm/llvm-project/pull/110497 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
https://github.com/Cydox updated
https://github.com/llvm/llvm-project/pull/110497
>From 15b69329a97706ada7d5e8cbeb76508aa55b418f Mon Sep 17 00:00:00 2001
From: Jan Hendrik Farr
Date: Sun, 29 Sep 2024 21:38:13 +0200
Subject: [PATCH 1/5] [Clang] Fix 'counted_by' for nested struct pointers
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.
---
clang/lib/CodeGen/CGExpr.cpp | 8 +--
clang/test/CodeGen/attr-counted-by-pr110385.c | 62 +++
2 files changed, 66 insertions(+), 4 deletions(-)
create mode 100644 clang/test/CodeGen/attr-counted-by-pr110385.c
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(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(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 00..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]
[clang] [Clang] Fix 'counted_by' for nested struct pointers (PR #110497)
@@ -1164,15 +1163,15 @@ llvm::Value *CodeGenFunction::EmitLoadOfCountedByField(
return nullptr;
llvm::Value *Res = nullptr;
- if (StructBase->isLValue()) {
-LValue LV = EmitLValue(StructBase);
-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 {
Cydox wrote:
added that extra check
https://github.com/llvm/llvm-project/pull/110497
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
https://github.com/Cydox created
https://github.com/llvm/llvm-project/pull/111015
Fixes: #111009
Change the behavior of __bdos to be in-line with gcc by changing the behvaior
from:
```
max(sizeof(struct s), offsetof(struct s, array) + p->count * sizeof(*p->array))
```
to:
```
sizeof(struct s) + p->count * sizeof(*p->array))
```
>From 0f03f52e68a0dd1bbe01a9eefa9337ae54e57586 Mon Sep 17 00:00:00 2001
From: Jan Hendrik Farr
Date: Thu, 3 Oct 2024 17:41:43 +0200
Subject: [PATCH] [Clang] Fix __builtin_dynamic_object_size off by 4
Fixes: #111009
Change the behavior of __bdos to be in-line with gcc by changing the
behvaior from:
max(sizeof(struct s), offsetof(struct s, array) + p->count * sizeof(*p->array))
to:
sizeof(struct s) + p->count * sizeof(*p->array))
---
clang/lib/CodeGen/CGBuiltin.cpp | 16 +-
clang/test/CodeGen/attr-counted-by-pr111009.c | 61 ++
clang/test/CodeGen/attr-counted-by.c | 183 --
3 files changed, 148 insertions(+), 112 deletions(-)
create mode 100644 clang/test/CodeGen/attr-counted-by-pr111009.c
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index c864714182e019..f12f8d4bfb1571 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -919,8 +919,7 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E,
unsigned Type,
// 2) bdos of the whole struct, including the flexible array:
//
// __builtin_dynamic_object_size(p, 1) ==
- //max(sizeof(struct s),
- //offsetof(struct s, array) + p->count * sizeof(*p->array))
+ //sizeof(struct s) + p->count * sizeof(*p->array))
//
ASTContext &Ctx = getContext();
const Expr *Base = E->IgnoreParenImpCasts();
@@ -1052,22 +1051,13 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr
*E, unsigned Type,
// The whole struct is specificed in the __bdos.
const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(OuterRD);
-// Get the offset of the FAM.
-llvm::Constant *FAMOffset = ConstantInt::get(ResType, Offset, IsSigned);
-Value *OffsetAndFAMSize =
-Builder.CreateAdd(FAMOffset, Res, "", !IsSigned, IsSigned);
// Get the full size of the struct.
llvm::Constant *SizeofStruct =
ConstantInt::get(ResType, Layout.getSize().getQuantity(), IsSigned);
-// max(sizeof(struct s),
-// offsetof(struct s, array) + p->count * sizeof(*p->array))
-Res = IsSigned
- ? Builder.CreateBinaryIntrinsic(llvm::Intrinsic::smax,
- OffsetAndFAMSize, SizeofStruct)
- : Builder.CreateBinaryIntrinsic(llvm::Intrinsic::umax,
- OffsetAndFAMSize, SizeofStruct);
+// Add full size of struct and fam size
+Res = Builder.CreateAdd(SizeofStruct, Res, "", !IsSigned, IsSigned);
}
// A negative \p IdxInst or \p CountedByInst means that the index lands
diff --git a/clang/test/CodeGen/attr-counted-by-pr111009.c
b/clang/test/CodeGen/attr-counted-by-pr111009.c
new file mode 100644
index 00..87db75cd4a4ee8
--- /dev/null
+++ b/clang/test/CodeGen/attr-counted-by-pr111009.c
@@ -0,0 +1,61 @@
+// 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 #111009
+// Based on reproducer from Thorsten Blum:
+//
https://lore.kernel.org/linux-kernel/3d0816d1-0807-4d37-8d5f-3c55ca910...@linux.dev/
+
+typedef unsigned int uid_t;
+typedef unsigned int gid_t;
+
+typedef struct {
+int counter;
+} atomic_t;
+
+typedef struct refcount_struct {
+atomic_t refs;
+} refcount_t;
+
+struct callback_head {
+struct callback_head *next;
+void (*func)(struct callback_head *head);
+} __attribute__((aligned(sizeof(void *;
+#define rcu_head callback_head
+
+typedef struct {
+uid_t val;
+} kuid_t;
+
+typedef struct {
+gid_t val;
+} kgid_t;
+
+struct posix_acl_entry {
+short e_tag;
+unsigned short e_perm;
+union {
+kuid_t e_uid;
+kgid_t e_gid;
+};
+};
+
+struct posix_acl {
+refcount_t a_refcount;
+struct rcu_head a_rcu;
+unsigned int a_count;
+struct posix_acl_entry a_entries[] __attribute__((counted_by(a_count)));
+};
+
+// CHECK-LABEL: define dso_local range(i32 32, 25) i32 @test(
+// CHECK-SAME: ptr nocapture noundef readonly [[FOO:%.*]]) local_unnamed_addr
#[[ATTR0:[0-9]+]] {
+// CHECK-NEXT: entry:
+// CHECK-NEXT:[[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr
[[FOO]], i64 24
+// CHECK-NEXT:[[DOT_COUNTED_BY_LOAD:%.*]] = load i32, ptr
[[DOT_COUNTED_BY_GEP]], align 4
+// CHECK-NEXT:[[TMP0:%.*]] = shl i32 [[DOT_COUNTED_BY_LOAD]], 3
+// CHECK-NEXT:[[CONV:%.*]] = add i32 [[TMP0]], 32
+// CHECK-NEXT:ret i32 [[CONV]]
+//
+int test(struct posix_acl *foo) {
+return __bui
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote: cc @nathanchance @kees @bwendling @efriedma-quic https://github.com/llvm/llvm-project/pull/111015 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
Cydox wrote:
I'm not 100% sure if the gcc or the clang behavior is currently correct.
However, I'm gonna argue that gcc has it correct.
gcc currently says that the __bdos of struct containing a flexible array member
is:
```
sizeof() + sizeof() *
```
clang however does the following:
```
max(sizeof(), offsetof() +
sizeof() * )
```
The kernel assumes the gcc behvaior in places like linux/fs/posix_acl.c:
```C
struct posix_acl *
posix_acl_clone(const struct posix_acl *acl, gfp_t flags)
{
struct posix_acl *clone = NULL;
if (acl) {
int size = sizeof(struct posix_acl) + acl->a_count *
sizeof(struct posix_acl_entry);
clone = kmemdup(acl, size, flags);
if (clone)
refcount_set(&clone->a_refcount, 1);
}
return clone;
}
EXPORT_SYMBOL_GPL(posix_acl_clone);
```
This is the code that triggers the problem in [1]. The way I see it, this code
should work, as you also allocate `struct posix_acl` with the same
`sizeof(struct posix_acl) + acl->a_count * sizeof(struct posix_acl_entry)` as
an argument to malloc (or in-kernel equivalent).
Based on the C standard the size of that object is the size passed to
malloc. See bottom of page 348 [2].
[1]
https://lore.kernel.org/linux-kernel/3d0816d1-0807-4d37-8d5f-3c55ca910...@linux.dev/
[2] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf
https://github.com/llvm/llvm-project/pull/111015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Fix __builtin_dynamic_object_size off by 4 (PR #111015)
https://github.com/Cydox updated
https://github.com/llvm/llvm-project/pull/111015
>From 0f03f52e68a0dd1bbe01a9eefa9337ae54e57586 Mon Sep 17 00:00:00 2001
From: Jan Hendrik Farr
Date: Thu, 3 Oct 2024 17:41:43 +0200
Subject: [PATCH 1/2] [Clang] Fix __builtin_dynamic_object_size off by 4
Fixes: #111009
Change the behavior of __bdos to be in-line with gcc by changing the
behvaior from:
max(sizeof(struct s), offsetof(struct s, array) + p->count * sizeof(*p->array))
to:
sizeof(struct s) + p->count * sizeof(*p->array))
---
clang/lib/CodeGen/CGBuiltin.cpp | 16 +-
clang/test/CodeGen/attr-counted-by-pr111009.c | 61 ++
clang/test/CodeGen/attr-counted-by.c | 183 --
3 files changed, 148 insertions(+), 112 deletions(-)
create mode 100644 clang/test/CodeGen/attr-counted-by-pr111009.c
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index c864714182e019..f12f8d4bfb1571 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -919,8 +919,7 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr *E,
unsigned Type,
// 2) bdos of the whole struct, including the flexible array:
//
// __builtin_dynamic_object_size(p, 1) ==
- //max(sizeof(struct s),
- //offsetof(struct s, array) + p->count * sizeof(*p->array))
+ //sizeof(struct s) + p->count * sizeof(*p->array))
//
ASTContext &Ctx = getContext();
const Expr *Base = E->IgnoreParenImpCasts();
@@ -1052,22 +1051,13 @@ CodeGenFunction::emitFlexibleArrayMemberSize(const Expr
*E, unsigned Type,
// The whole struct is specificed in the __bdos.
const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(OuterRD);
-// Get the offset of the FAM.
-llvm::Constant *FAMOffset = ConstantInt::get(ResType, Offset, IsSigned);
-Value *OffsetAndFAMSize =
-Builder.CreateAdd(FAMOffset, Res, "", !IsSigned, IsSigned);
// Get the full size of the struct.
llvm::Constant *SizeofStruct =
ConstantInt::get(ResType, Layout.getSize().getQuantity(), IsSigned);
-// max(sizeof(struct s),
-// offsetof(struct s, array) + p->count * sizeof(*p->array))
-Res = IsSigned
- ? Builder.CreateBinaryIntrinsic(llvm::Intrinsic::smax,
- OffsetAndFAMSize, SizeofStruct)
- : Builder.CreateBinaryIntrinsic(llvm::Intrinsic::umax,
- OffsetAndFAMSize, SizeofStruct);
+// Add full size of struct and fam size
+Res = Builder.CreateAdd(SizeofStruct, Res, "", !IsSigned, IsSigned);
}
// A negative \p IdxInst or \p CountedByInst means that the index lands
diff --git a/clang/test/CodeGen/attr-counted-by-pr111009.c
b/clang/test/CodeGen/attr-counted-by-pr111009.c
new file mode 100644
index 00..87db75cd4a4ee8
--- /dev/null
+++ b/clang/test/CodeGen/attr-counted-by-pr111009.c
@@ -0,0 +1,61 @@
+// 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 #111009
+// Based on reproducer from Thorsten Blum:
+//
https://lore.kernel.org/linux-kernel/3d0816d1-0807-4d37-8d5f-3c55ca910...@linux.dev/
+
+typedef unsigned int uid_t;
+typedef unsigned int gid_t;
+
+typedef struct {
+int counter;
+} atomic_t;
+
+typedef struct refcount_struct {
+atomic_t refs;
+} refcount_t;
+
+struct callback_head {
+struct callback_head *next;
+void (*func)(struct callback_head *head);
+} __attribute__((aligned(sizeof(void *;
+#define rcu_head callback_head
+
+typedef struct {
+uid_t val;
+} kuid_t;
+
+typedef struct {
+gid_t val;
+} kgid_t;
+
+struct posix_acl_entry {
+short e_tag;
+unsigned short e_perm;
+union {
+kuid_t e_uid;
+kgid_t e_gid;
+};
+};
+
+struct posix_acl {
+refcount_t a_refcount;
+struct rcu_head a_rcu;
+unsigned int a_count;
+struct posix_acl_entry a_entries[] __attribute__((counted_by(a_count)));
+};
+
+// CHECK-LABEL: define dso_local range(i32 32, 25) i32 @test(
+// CHECK-SAME: ptr nocapture noundef readonly [[FOO:%.*]]) local_unnamed_addr
#[[ATTR0:[0-9]+]] {
+// CHECK-NEXT: entry:
+// CHECK-NEXT:[[DOT_COUNTED_BY_GEP:%.*]] = getelementptr inbounds i8, ptr
[[FOO]], i64 24
+// CHECK-NEXT:[[DOT_COUNTED_BY_LOAD:%.*]] = load i32, ptr
[[DOT_COUNTED_BY_GEP]], align 4
+// CHECK-NEXT:[[TMP0:%.*]] = shl i32 [[DOT_COUNTED_BY_LOAD]], 3
+// CHECK-NEXT:[[CONV:%.*]] = add i32 [[TMP0]], 32
+// CHECK-NEXT:ret i32 [[CONV]]
+//
+int test(struct posix_acl *foo) {
+return __builtin_dynamic_object_size(foo, 0);
+}
+
diff --git a/clang/test/CodeGen/attr-counted-by.c
b/clang/test/CodeGen/attr-counted-by.c
index 4a130c5e3d401f..706b53d78ee011 100644
--- a/clang/test/CodeGen/attr-counted-by.c
+++ b/clang/test/CodeGen/attr-coun
[clang] [Clang] Disable use of the counted_by attribute for whole struct pointers (PR #112636)
Cydox wrote: We do have to consider though that when `__bdos` is for one of the maximum types (`type & 2 == 0`), it should actually return the largest allowed object that is consistent with the count. https://github.com/llvm/llvm-project/blob/3b88805ca20018ae202afd3aea39f4fa856a8c64/clang/docs/LanguageExtensions.rst?plain=1#L5502-L5507 So I think the correct result for `type & 2 == 0` is actually: ```C round_up( alignof(struct S) offsetof(struct S, fam) + count * sizeof(((struct S *)0)->fam[0]) ) + alignof(struct S) - 1 ``` because all objects that are 1 byte smaller than the calculation in https://github.com/llvm/llvm-project/pull/112636#issuecomment-2436559387 plus `alignof(struct S)` are perfectly legal to have the same count. Using that calculation for the two examples above looks to actually solve the issue: https://godbolt.org/z/Pdx7Mbano But that's just after having a quick look at it, we gotta prove that this will actually give a result >= `struct_size` in the kernel in all cases. https://github.com/llvm/llvm-project/pull/112636 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Disable use of the counted_by attribute for whole struct pointers (PR #112636)
Cydox wrote: > It's probably worthwhile to perform some analysis to see if using that to > calculate the new size results in allocation size changes in the kernel. If > not, then perhaps we could reinstate the "whole struct" code. This can calculation can result in both larger and smaller sizes compared to what the kernel currently does, so we can't reinstate the whole struct code until the `struct_size` macro in the kernel is changed if we want clang to use this calculation. https://github.com/llvm/llvm-project/pull/112636 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Disable use of the counted_by attribute for whole struct pointers (PR #112636)
Cydox wrote: Example of both cases: https://godbolt.org/z/dhzG69sab https://github.com/llvm/llvm-project/pull/112636 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Disable use of the counted_by attribute for whole struct pointers (PR #112636)
Cydox wrote:
> ```
> struct S {
> int foo;
> char fam[N];
> };
> ```
>
> Well, for N = 4 we have `sizeof(struct S) == 8` and for N = 5 we have
> `sizeof(struct S) == 12` (due to alignment padding), therefore N = 4. That
> makes `s->fam[4]` out-of-bounds. Am I wrong?
Using this example, every object of sizes 8 to 11 bytes, inclusive behaves like
the struct defined with N = 4. Therefore `__bdos(ptr, 0)`, where `ptr` is a
pointer to a `struct S` with a `__counted_by` attribute indicating N = 4,
should return 11. `__bdos(ptr, 2)` should return 8.
https://github.com/llvm/llvm-project/pull/112636
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
