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

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

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