Making structs with variable-sized arrays unsized?

Making structs with variable-sized arrays unsized?

[Linus Torvalds]

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;
    }

Re: Making structs with variable-sized arrays unsized?

[Luc Van Oostenryck]

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

Re: Making structs with variable-sized arrays unsized?

[Linus Torvalds]

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

Re: Making structs with variable-sized arrays unsized?

[Luc Van Oostenryck]

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

Re: Making structs with variable-sized arrays unsized?

[Luc Van Oostenryck]

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