Linux-Sparse Archive on lore.kernel.org
 help / color / Atom feed
* 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, back to index

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

Linux-Sparse Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-sparse/0 linux-sparse/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-sparse linux-sparse/ https://lore.kernel.org/linux-sparse \
		linux-sparse@vger.kernel.org
	public-inbox-index linux-sparse

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-sparse


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git