* Making structs with variable-sized arrays unsized?
@ 2020-09-18 19:33 Linus Torvalds
2020-09-18 20:41 ` Luc Van Oostenryck
0 siblings, 1 reply; 5+ messages in thread
From: Linus Torvalds @ 2020-09-18 19:33 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: Sparse Mailing-list
Luc,
we've been making kernel structures use flexible arrays as a
cleanliness thing, but it turns out that it doesn't find bugs that it
_should_ find.
We have that nice "struct_size()" macro to determine the size of the
flexible structure given the number of elements, which uses "offsetof
+ n*members". But sadly standard C still allows the (nonsensical)
'sizeof()' to be used - and I merged another fix for that just today.
Ok, so that's a C standard problem, but it's something that sparse
*could* warn about.
Comments? Appended is a kind of test-case for odd situations that
sparse happily and silently generates nonsensical code for (just
tested with test-linearize).
Linus
---
struct bad {
unsigned long long a;
char b[];
};
// An option to warn about this?
static struct bad array[5];
int odd(struct bad *a);
int not_nice(struct bad p[2]);
int please_fix(struct bad *p);
void weird(struct bad *dst, const struct bad *src);
// The layout is odd
// The code does "info->align_size = 0" for unsized arrays, but it
still works?
int odd(struct bad *a)
{
return __alignof__(*a);
}
// Arrays of flexible-array structures are pretty nonsensical
// Plus we don't even optimize the constant return. Sad.
int not_nice(struct bad p[2])
{
return (void *)(p+1) - (void *)p;
}
// This should at least have an option to warn
int please_fix(struct bad *p)
{
return sizeof(*p);
}
void weird(struct bad *dst, const struct bad *src)
{
*dst = *src;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Making structs with variable-sized arrays unsized?
2020-09-18 19:33 Making structs with variable-sized arrays unsized? Linus Torvalds
@ 2020-09-18 20:41 ` Luc Van Oostenryck
2020-09-18 20:49 ` Linus Torvalds
0 siblings, 1 reply; 5+ messages in thread
From: Luc Van Oostenryck @ 2020-09-18 20:41 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Sparse Mailing-list
On Fri, Sep 18, 2020 at 12:33:56PM -0700, Linus Torvalds wrote:
> Luc,
> we've been making kernel structures use flexible arrays as a
> cleanliness thing, but it turns out that it doesn't find bugs that it
> _should_ find.
>
> We have that nice "struct_size()" macro to determine the size of the
> flexible structure given the number of elements, which uses "offsetof
> + n*members". But sadly standard C still allows the (nonsensical)
> 'sizeof()' to be used - and I merged another fix for that just today.
>
> Ok, so that's a C standard problem, but it's something that sparse
> *could* warn about.
Yes, sure.
I think that sparse treats flexible arrays exactly as if zero-sized,
without the notion of 'incomplete type' and without check that it is
the last member, so without any warnings.
This, I think, explains the results in your tests here under.
I'll look to add some warnings for array declaration and sizeof()
(explicit or implicit).
> Comments? Appended is a kind of test-case for odd situations that
> sparse happily and silently generates nonsensical code for (just
> tested with test-linearize).
Thanks, these tests make a lot of sense.
It should be noted, though, that test-linearize gives exactly the
same result as GCC & clang (sparse IR 100% matches x86 & ARM64 here).
I also have 2 questions here under.
> struct bad {
> unsigned long long a;
> char b[];
> };
...
> // The layout is odd
> // The code does "info->align_size = 0" for unsized arrays, but it
> still works?
> int odd(struct bad *a)
> {
> return __alignof__(*a);
> }
This returns 8. What's odd here?
The 0 align_size is only for the member 'b' and shouldn't have any
effect on the alignment of the whole struct. What am I missing?
> // Arrays of flexible-array structures are pretty nonsensical
> // Plus we don't even optimize the constant return. Sad.
> int not_nice(struct bad p[2])
> {
> return (void *)(p+1) - (void *)p;
> }
I don't understand what you mean by 'optimize the constant return'.
test-linearize returns the only possible sensical answer (if the size
of the structure is accepted to be 8):
not_nice:
.L2:
<entry-point>
ret.32 $8
What could be optimized here?
-- Luc
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Making structs with variable-sized arrays unsized?
2020-09-18 20:41 ` Luc Van Oostenryck
@ 2020-09-18 20:49 ` Linus Torvalds
2020-09-18 21:04 ` Luc Van Oostenryck
2020-09-30 23:28 ` Luc Van Oostenryck
0 siblings, 2 replies; 5+ messages in thread
From: Linus Torvalds @ 2020-09-18 20:49 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: Sparse Mailing-list
On Fri, Sep 18, 2020 at 1:41 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> I also have 2 questions here under.
>
> > struct bad {
> > unsigned long long a;
> > char b[];
> > };
> ...
> > // The layout is odd
> > // The code does "info->align_size = 0" for unsized arrays, but it
> > still works?
> > int odd(struct bad *a)
> > {
> > return __alignof__(*a);
> > }
>
> This returns 8. What's odd here?
The fact that it works correctly.
> The 0 align_size is only for the member 'b' and shouldn't have any
> effect on the alignment of the whole struct. What am I missing?
I wrote that code by looking at the sparse source, and _expected_ it
to return the wrong value.
Because the sparse code does
/*
* Unsized arrays cause us to not align the resulting
* structure size
*/
if (base_size < 0) {
info->align_size = 0;
base_size = 0;
}
so I expected that when base_size < 0, we'd drop the _previous_
alignment we saved.
But what I suspect goes on is that base_size is actually 0, not < 0.
But I didn't verify.
> > // Arrays of flexible-array structures are pretty nonsensical
> > // Plus we don't even optimize the constant return. Sad.
> > int not_nice(struct bad p[2])
> > {
> > return (void *)(p+1) - (void *)p;
> > }
>
> I don't understand what you mean by 'optimize the constant return'.
> test-linearize returns the only possible sensical answer (if the size
> of the structure is accepted to be 8):
> not_nice:
> .L2:
> <entry-point>
> ret.32 $8
That's not what I see. I see
not_nice:
.L2:
<entry-point>
add.64 %r3 <- %arg1, $8
sub.64 %r5 <- %r3, %arg1
trunc.32 %r6 <- (64) %r5
ret.32 %r6
which is rather different and not exactly optimal.
That wasn't what I _intended_ to look for, obviously. I expected the
code you quote.
I wonder why it works for you but not me.
Linus
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Making structs with variable-sized arrays unsized?
2020-09-18 20:49 ` Linus Torvalds
@ 2020-09-18 21:04 ` Luc Van Oostenryck
2020-09-30 23:28 ` Luc Van Oostenryck
1 sibling, 0 replies; 5+ messages in thread
From: Luc Van Oostenryck @ 2020-09-18 21:04 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Sparse Mailing-list
On Fri, Sep 18, 2020 at 01:49:46PM -0700, Linus Torvalds wrote:
> On Fri, Sep 18, 2020 at 1:41 PM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > I also have 2 questions here under.
> >
> > > struct bad {
> > > unsigned long long a;
> > > char b[];
> > > };
> > ...
> > > // The layout is odd
> > > // The code does "info->align_size = 0" for unsized arrays, but it
> > > still works?
> > > int odd(struct bad *a)
> > > {
> > > return __alignof__(*a);
> > > }
> >
> > This returns 8. What's odd here?
>
> The fact that it works correctly.
>
> > The 0 align_size is only for the member 'b' and shouldn't have any
> > effect on the alignment of the whole struct. What am I missing?
>
> I wrote that code by looking at the sparse source, and _expected_ it
> to return the wrong value.
>
> Because the sparse code does
>
> /*
> * Unsized arrays cause us to not align the resulting
> * structure size
> */
> if (base_size < 0) {
> info->align_size = 0;
> base_size = 0;
> }
>
> so I expected that when base_size < 0, we'd drop the _previous_
> alignment we saved.
>
> But what I suspect goes on is that base_size is actually 0, not < 0.
> But I didn't verify.
OK, I see. I'll check this.
> > > // Arrays of flexible-array structures are pretty nonsensical
> > > // Plus we don't even optimize the constant return. Sad.
> > > int not_nice(struct bad p[2])
> > > {
> > > return (void *)(p+1) - (void *)p;
> > > }
> >
> > I don't understand what you mean by 'optimize the constant return'.
> > test-linearize returns the only possible sensical answer (if the size
> > of the structure is accepted to be 8):
> > not_nice:
> > .L2:
> > <entry-point>
> > ret.32 $8
>
> That's not what I see. I see
>
> not_nice:
> .L2:
> <entry-point>
> add.64 %r3 <- %arg1, $8
> sub.64 %r5 <- %r3, %arg1
> trunc.32 %r6 <- (64) %r5
> ret.32 %r6
>
> which is rather different and not exactly optimal.
>
> That wasn't what I _intended_ to look for, obviously. I expected the
> code you quote.
>
> I wonder why it works for you but not me.
My fault, sorry. By error, I didn't run these tests on the head but in
one of my branches that coincidentally had some patches doing some
reassociation which then triggers the optimization.
-- Luc
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Making structs with variable-sized arrays unsized?
2020-09-18 20:49 ` Linus Torvalds
2020-09-18 21:04 ` Luc Van Oostenryck
@ 2020-09-30 23:28 ` Luc Van Oostenryck
1 sibling, 0 replies; 5+ messages in thread
From: Luc Van Oostenryck @ 2020-09-30 23:28 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Sparse Mailing-list
On Fri, Sep 18, 2020 at 01:49:46PM -0700, Linus Torvalds wrote:
> On Fri, Sep 18, 2020 at 1:41 PM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > I also have 2 questions here under.
> >
> > > struct bad {
> > > unsigned long long a;
> > > char b[];
> > > };
> > ...
> > > // The layout is odd
> > > // The code does "info->align_size = 0" for unsized arrays, but it
> > > still works?
> > > int odd(struct bad *a)
> > > {
> > > return __alignof__(*a);
> > > }
> >
> > This returns 8. What's odd here?
>
> The fact that it works correctly.
For info, it was just a coincidence. The returned value correspond
to the value 'max_align' which was not reset. This is now fixed in
the series I just posted.
Best regards,
-- Luc
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-30 23:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-18 19:33 Making structs with variable-sized arrays unsized? Linus Torvalds
2020-09-18 20:41 ` Luc Van Oostenryck
2020-09-18 20:49 ` Linus Torvalds
2020-09-18 21:04 ` Luc Van Oostenryck
2020-09-30 23:28 ` Luc Van Oostenryck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).