Linux-Sparse Archive on lore.kernel.org
 help / color / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Cc: Sparse Mailing-list <linux-sparse@vger.kernel.org>
Subject: Re: [PATCH 13/13] flex-array: warn when a flexible array member has some padding
Date: Thu, 1 Oct 2020 09:34:37 -0700
Message-ID: <CAHk-=wiyAWuHQuDPBC290V-Bx5Eyu1izPj9iSSN0hdkSM2a4iQ@mail.gmail.com> (raw)
In-Reply-To: <20200930231828.66751-14-luc.vanoostenryck@gmail.com>

On Wed, Sep 30, 2020 at 4:18 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> If some padding is added because of the presence of a flexible
> array member, the size of the structure will be greater than
> the offset of this flexible array which can cause some
> problems if the assumption is made that these 2 size must be
> identical (which is easy to do since such flexible arrays are
> conceptually 'after' the structure itself).

The warning seems pointless, and the explanation above is wrong.

Flexible array padding is normal and good. IOW, if you have

    struct odd_struct {
        char c;
        unsigned int flex[];
    };

then the flexible array - and the structure - very much should be
aligned on 'unsigned int', and both the offset of the flex-array and
the (bogus) size of the structure is 4.

So this is a normal case and nothing wrong with it, and the above is
the "flexible array caused padding" one (but sizeof and offsetof
match).

And the case that causes sizeof() and offsetof() to not match is
normal too: but is not that the flexible array member caused padding,
but that *other* members did.

IOW, maybe you have a structure like this:

    struct other {
        uint64_t a;
        uint32_t b;
        char flex[];
    };

and now "offsetof(flex)" is 12, but "sizeof(struct other)" is 16,
because the flex-array itself has no real alignment requirement and
will just be laid out after the 12 bytes of a/b, but the structure has
8-byte alignment due to 'a'.

So I don't think the warning is interesting, because this is a
perfectly normal condition too.

And I don't think your explanation for the warning makes sense,
because you say "padding is added because of the presence of a
flexible array member", but that's not at all what is going on. The
padding is added because of *other* members.

Anyway, the above is just an example of why "sizeof()" itself makes no
sense on these things. A "sizeof()" of a structure with a flexible
array member is inherently pointless. You can't use it for anything
really valid, and it doesn't have any sensible meaning.

But I don't think that has anything to do with warning about padding.
The padding is right - it's the sizeof() itself that is nonsensical.

So in the kernel, we would

 - start warning about 'sizeof(flex_struct)'

 - make our "struct_size(s, m, N)" construct use
"offsetof(m)+N*sizeof(*m)" instead of using sizeof().

Of course, it may well be that we end up with trouble just because we
end up _needing_ sizeof() for some reason. I can't think of any sane
situation off the top of my head, but who knows what odd macros etc we
might have that end up doing sizeof() as part of things..

                  Linus

  reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-30 23:18 [PATCH 00/13] add warnings for flexible arrays Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 01/13] flex-array: add testcases Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 02/13] flex-array: factor out common part of lay_out_{struct,union}() Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 03/13] flex-array: do not lay out invalid struct members Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 04/13] flex-array: flexible array members have zero size and alignment is OK Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 05/13] flex-array: detect structures with a flexible array member Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 06/13] flex-array: warn on flexible arrays in unions Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 07/13] flex-array: warn if flexible array is not last Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 08/13] flex-array: identify structures with a flexible array member Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 09/13] flex-array: add helper has_flexible_array() Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 10/13] flex-array: warn when using sizeof() on a flexible array Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 11/13] flex-array: warn an arrays containing " Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 12/13] flex-array: warn on flexible array in nested aggregate types Luc Van Oostenryck
2020-09-30 23:18 ` [PATCH 13/13] flex-array: warn when a flexible array member has some padding Luc Van Oostenryck
2020-10-01 16:34   ` Linus Torvalds [this message]
2020-10-01 19:17     ` Luc Van Oostenryck
2020-10-01 19:27       ` Linus Torvalds
2020-10-01 19:41         ` Luc Van Oostenryck
2020-10-01 19:51           ` Luc Van Oostenryck
2020-10-01 16:36 ` [PATCH 00/13] add warnings for flexible arrays Linus Torvalds

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHk-=wiyAWuHQuDPBC290V-Bx5Eyu1izPj9iSSN0hdkSM2a4iQ@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=linux-sparse@vger.kernel.org \
    --cc=luc.vanoostenryck@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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