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 cal
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`)
`(F
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
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.
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) {
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
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;
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 #110
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-
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-co
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
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
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 a
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 f
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 @@ Cod
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_dyna
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
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
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 ma
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) + s
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/c
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/l
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(
>
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
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
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
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
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
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
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
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
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;
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://list
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
> #inclu
@@ -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).
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
_
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
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 me
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:
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 t
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:/
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
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
@@ -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/ll
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
@@ -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
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
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 origin
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
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
@@ -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
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
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
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:
```
ma
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: #
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
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 size
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
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
59 matches
Mail list logo
