Re: [PATCH][next] tty: tty_buffer: Avoid hundreds of -Wflex-array-member-not-at-end warnings

[Gustavo A. R. Silva]

On 05/02/25 16:06, Greg Kroah-Hartman wrote:
> On Wed, Feb 05, 2025 at 03:51:35PM +1030, Gustavo A. R. Silva wrote:
> > -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
> > getting ready to enable it, globally.
> >
> > So, in order to avoid ending up with a flexible-array member in the
> > middle of other structs, we use the `struct_group_tagged()` helper
> > to create a new tagged `struct tty_buffer_hdr`. This structure
> > groups together all the members of the flexible `struct tty_buffer`
> > except the flexible array.
> >
> > As a result, the array is effectively separated from the rest of the
> > members without modifying the memory layout of the flexible structure.
> > We then change the type of the middle struct member currently causing
> > trouble from `struct tty_buffer` to `struct tty_buffer_hdr`.
> >
> > We also want to ensure that when new members need to be added to the
> > flexible structure, they are always included within the newly created
> > tagged struct. For this, we use `static_assert()`. This ensures that the
> > memory layout for both the flexible structure and the new tagged struct
> > is the same after any changes.
> >
> > This approach avoids having to implement `struct tty_buffer_hdr` as a
> > completely separate structure, thus preventing having to maintain two
> > independent but basically identical structures, closing the door to
> > potential bugs in the future.
>
> Why not just have a separate structure and embed that in the places it
> is used?  No duplication should be needed or am I missing something?

The duplication of members in struct tty_buffer (all of them except the
flexible array) and the new separate struct looks like this:

diff --git a/include/linux/tty_buffer.h b/include/linux/tty_buffer.h
index 31125e3be3c5..6f47a6ea5a05 100644
--- a/include/linux/tty_buffer.h
+++ b/include/linux/tty_buffer.h
@@ -22,6 +22,19 @@ struct tty_buffer {
        u8 data[] __aligned(sizeof(unsigned long));
 };

+struct tty_buffer_hdr {
+        union {
+                struct tty_buffer *next;
+                struct llist_node free;
+        };
+        unsigned int used;
+        unsigned int size;
+        unsigned int commit;
+        unsigned int lookahead;         /* Lazy update on recv, can become less than "read" */
+        unsigned int read;
+        bool flags;
+};
+
 static inline u8 *char_buf_ptr(struct tty_buffer *b, unsigned int ofs)
 {
        return b->data + ofs;
@@ -37,7 +50,7 @@ struct tty_bufhead {
        struct work_struct work;
        struct mutex       lock;
        atomic_t           priority;
-       struct tty_buffer sentinel;
+       struct tty_buffer_hdr sentinel;
        struct llist_head free;         /* Free queue head */
        atomic_t           mem_used;    /* In-use buffers excluding free list */
        int                mem_limit;

> Did you change anything in this structure?  By reformatting it, it's
> hard to tell what happened, so please don't do that :(

Yes - the above change is the reason why I reformatted the whole thing.

> I don't mind that, it would make this all much simpler and more obvious
> over time, and the tty layer needs all the "simplification" it can get
> :)

If the above changes are better for you then I'll send a new patch. :)

Thanks
-Gustavo