From: Richard Biener <rguenther@suse.de>
To: Kees Cook <keescook@chromium.org>
Cc: Sebastian Huber <sebastian.huber@embedded-brains.de>,
Qing Zhao <qing.zhao@oracle.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [V2][PATCH] gcc-14/changes.html: Deprecate a GCC C extension on flexible array members.
Date: Wed, 8 May 2024 09:25:48 +0200 (CEST) [thread overview]
Message-ID: <6np789q5-3207-p6ss-1ssq-92rrr4r098r9@fhfr.qr> (raw)
In-Reply-To: <202405071225.A48DC8630B@keescook>
[-- Attachment #1: Type: text/plain, Size: 9581 bytes --]
On Tue, 7 May 2024, Kees Cook wrote:
> On Tue, May 07, 2024 at 06:34:19PM +0000, Qing Zhao wrote:
> > On May 7, 2024, at 13:57, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote:
> > > On 07.05.24 16:26, Qing Zhao wrote:
> > > > Hi, Sebastian,
> > > > Thanks for your explanation.
> > > > Our goal is to deprecate the GCC extension on structure
> > > > containing a flexible array member not at the end of another
> > > > structure. In order to achieve this goal, we provided the warning option
> > > > -Wflex-array-member-not-at-end for the users to locate all such
> > > > cases in their source code and update the source code to eliminate
> > > > such cases.
> > >
> > > What is the benefit of deprecating this GCC extension? If GCC
> > > extensions are removed, then it would be nice to enable the associated
> > > warnings by default.
>
> The goal of all of the recent array bounds and flexible array work is to
> make sizing information unambiguous (e.g. via __builtin_object_size(),
> __builtin_dynamic_object_size(), and the array-bounds sanitizer). For
> the compiler to be able to deterministically report size information
> on arrays, we needed to deprecate this case even though it had been
> supported in the past. (Though we also _added_ extensions to support
> for other things, like flexible arrays in unions, and the coming
> __counted_by attribute.)
>
> For example:
>
> struct flex { int length; char data[]; };
> struct mid_flex { int m; struct flex flex_data; int n; int o; };
It might be reasonable to allow tag-less "anonymous" struct that's
"completed" by means of the static initializer of a declared object, thus
struct { int m; struct { int length, char data[]; } flex_data; int n; int
o; } object
= { 3, { 2, { 1, 2 } }, 4, 5 };
The frontend would make the size of data[] static, determined by the
initializer. I _think_ the C standard makes object.flex_data
inter-operate with struct flex from above but struct mid_flex would
be an invalid type declaration and thus there's no way to have
an API with a pointer to such structure(?) which makes the extension
somewhat less useful.
>
> #define SZ(p) __builtin_dynamic_object_size(p, 1)
>
> void foo(struct flex *f, struct mid_flex *m)
> {
> printf("%zu\n", SZ(f));
> printf("%zu\n", SZ(m->flex_data));
> }
>
> int main(void)
> {
> struct mid_flex m = { .flex_data.length = 8 };
> foo(&m->flex_data, &m);
> return 0;
> }
>
> This is printing the size of the same object. But the desired results
> are ambiguous. Does m->flex_data have an unknown size (i.e. SIZE_MAX)
> because it's a flex array, or does it contain 8 bytes, since it overlaps
> with the other structure's trailing 2 ints?
>
> The answer from GCC 13 was neither:
>
> 18446744073709551615
> 4
>
> It considered flex_data to be only the size of it's non-flex-array
> members, but only when there was semantic context that it was part of
> another structure. (Yet more ambiguity.)
>
> In GCC 14, this is "resolved" to be unknown since it is a flex array
> which has no sizing info, and context doesn't matter:
>
> 18446744073709551615
> 18446744073709551615
>
> But this paves the way for the coming 'counted_by' attribute which will
> allow for struct flex above to be defined as:
>
> struct flex { int length; char data[] __attribute__((counted_by(length))); };
>
> At which point GCC can deterministically report the object size.
>
> Hopefully I've captured this all correctly -- Qing can correct me. :)
>
> > >
> > > > We had a long discussion before deciding to deprecating this GCC
> > > > extension. Please see details here:
> > > >
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832
> > > >
> > > > Yes, we do plan to enable this warning by default before final
> > > > deprecation. (Might consider to enable this warning by default in
> > > > GCC15… and then deprecate it in the next release)
> > > >
> > > > Right now, there is an ongoing work in Linux kernel to get rid of
> > > > all such cases. Kees might have more information on this.
> > > >
> > > >
> > > > The static initialization of structures with flexible array members
> > > > will still work as long as the flexible array members are at the end of
> > > > the structures.
> > >
> > > Removing the support for flexible array members in the middle of
> > > compounds will make the static initialization practically infeasible.
> >
> > If the flexible array member is moved to the end of the compounds,
> > the static initialization still work. What’s the issue here?
> >
> > > > My question: is it possible to update your source code to move
> > > > the structure with flexible array member to the end of the containing
> > > > structure?
> > > >
> > > > i.e, in your example, in the struct Thread_Configured_control,
> > > > move the field “Thread_Control Control” to the end of the structure?
> > >
> > > If we move the Thread_Control to the end, how would I add a
> > > configuration defined number of elements at the end?
> >
> > Don’t understand this, why moving the Thread_Control Control” to
> > the end of the containing structure will make this a problem?
> > Could you please explain this with a simplified example?
>
> I found your example at [2] and tried to trim/summarize it here:
>
>
> struct _Thread_Control {
> Objects_Control Object;
> ...
> void *extensions[];
> };
> typedef struct _Thread_Control Thread_Control;
>
> struct Thread_Configured_control {
> Thread_Control Control;
>
> #if CONFIGURE_MAXIMUM_USER_EXTENSIONS > 0
> void *extensions[ CONFIGURE_MAXIMUM_USER_EXTENSIONS + 1 ];
> #endif
> Configuration_Scheduler_node Scheduler_nodes[ _CONFIGURE_SCHEDULER_COUNT ];
> RTEMS_API_Control API_RTEMS;
> #ifdef RTEMS_POSIX_API
> POSIX_API_Control API_POSIX;
> #endif
> #if CONFIGURE_MAXIMUM_THREAD_NAME_SIZE > 1
> char name[ CONFIGURE_MAXIMUM_THREAD_NAME_SIZE ];
> #endif
> #if defined(_CONFIGURE_ENABLE_NEWLIB_REENTRANCY) && \
> !defined(_REENT_THREAD_LOCAL)
> struct _reent Newlib;
> #endif
> };
>
> #define THREAD_INFORMATION_DEFINE( name, api, cls, max ) \
> ...
> static ... \
> Thread_Configured_control \
> name##_Objects[ _Objects_Maximum_per_allocation( max ) ]; \
> ...
>
>
> I don't see any static initialization of struct _Thread_Control::extensions
> nor any member initialization of the name##_Objects, and even then that
> is all legal in any arrangement:
>
> truct flex { int length; char data[]; };
> struct mid_flex { int m; struct flex flex_data; int n; int o; };
> struct end_flex { int m; int n; struct flex flex_data; };
>
> struct flex f = { .length = 2 };
> struct mid_flex m = { .m = 5 };
> struct end_flex e = { .m = 5 };
>
> struct flex fa[4] = { { .length = 2 } };
> struct mid_flex ma[4] = { { .m = 5 } };
> struct end_flex ea[4] = { { .m = 5 } };
>
> These all work.
>
>
> But yes, I see why you can't move Thread_Control trivially to the end. It
> looks like you're depending on the implicit overlapping memory locations
> between struct _Thread_Control and things that include it as the first
> struct member, like struct Thread_Configured_control above:
>
> cpukit/score/src/threaditerate.c: the_thread = (Thread_Control *) information->local_table[ index ];
>
> (In the Linux kernel we found this kind of open casting to be very
> fragile and instead use a horrific wrapper called "container_of"[3] that
> does the pointer math (possibly to an offset of 0 for a direct cast) to
> find the member.)
>
> Anyway, for avoiding the warning, you can just keep using the extension
> and add -Wno-... if it ever ends up in -Wall, or you can redefine struct
> _Thread_Control to avoid having the "extensions" member at all. This is
> what we've done in several cases in Linux. For example if we had this
> again, but made to look more like Thread_Control:
>
> struct flex { int foo; int bar; char data[]; };
> struct mid_flex { struct flex hdr; int n; int o; };
>
> It could be changed to:
>
> struct flex_hdr { int foo; int bar; };
> struct flex { struct flex_hdr hdr; char data[]; };
> struct mid_flex { struct flex_hdr hdr; int n; int o; };
>
> This has some collateral changes needed to reference the struct flex_hdr
> members from struct flex now (f->hdr.foo instead of f->foo). Sometimes
> this can be avoided by using a union, as I did in a recent refactoring
> in Linux: [4]
>
> For more complex cases in Linux we've handled this by using our
> "struct_group"[5] macro, which allows for a union and tagged struct to
> be constructed:
>
> struct flex {
> __struct_group(flex_hdr, hdr,,
> int foo;
> int bar;
> );
> char data[];
> };
> struct mid_flex { struct flex_hdr hdr; int n; int o; };
>
> Then struct flex member names don't have to change, but if anything is
> trying to get at struct flex::data through struct mid_flex::hdr, that'll
> need casting. But it _shouldn't_ since it has "n" and "o".
>
> -Kees
>
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620122.html
> [2] https://github.com/RTEMS/rtems
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/container_of.h#n10
> [4] https://git.kernel.org/linus/896880ff30866f386ebed14ab81ce1ad3710cfc4
> [5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/stddef.h?h=v6.8#n11
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
next prev parent reply other threads:[~2024-05-08 7:25 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-07 14:22 Qing Zhao
2023-08-08 7:55 ` Richard Biener
2023-09-25 18:24 ` Improve -Wflex-array-member-not-at-end changes.html wording |Plus: and warning bug? (was: [V2][PATCH] gcc-14/changes.html: Deprecate a GCC C extension on flexible array members.) Tobias Burnus
2023-09-26 6:49 ` Richard Biener
2023-09-26 7:26 ` Improve -Wflex-array-member-not-at-end changes.html wording |Plus: and warning bug? Tobias Burnus
2023-10-02 3:58 ` Improve -Wflex-array-member-not-at-end changes.html wording |Plus: and warning bug? (was: [V2][PATCH] gcc-14/changes.html: Deprecate a GCC C extension on flexible array members.) Qing Zhao
2023-10-19 20:49 ` Qing Zhao
2023-10-19 23:29 ` Kees Cook
2024-05-04 11:11 ` [V2][PATCH] gcc-14/changes.html: Deprecate a GCC C extension on flexible array members Sebastian Huber
2024-05-06 7:08 ` Richard Biener
2024-05-06 13:29 ` Sebastian Huber
2024-05-06 14:20 ` Qing Zhao
2024-05-07 13:15 ` Sebastian Huber
2024-05-07 14:26 ` Qing Zhao
2024-05-07 17:57 ` Sebastian Huber
2024-05-07 18:34 ` Qing Zhao
2024-05-07 21:52 ` Kees Cook
2024-05-08 7:25 ` Richard Biener [this message]
2024-05-08 19:28 ` Qing Zhao
2024-05-07 20:14 ` Qing Zhao
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=6np789q5-3207-p6ss-1ssq-92rrr4r098r9@fhfr.qr \
--to=rguenther@suse.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=keescook@chromium.org \
--cc=qing.zhao@oracle.com \
--cc=sebastian.huber@embedded-brains.de \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).