[RFC]: zephyr: CMakeLists: disable packed-related warnings

[LaurentiuM1234]

This is an attempt to reduce the compilation warnings on i.MX93 which is a 64-bit platform.

Frankly I doubt this fix is the way to go, but it's a good opportunity to open a discussion. There's a couple of questions that I have:

1. Does "struct coherent" actually need to be packed?
2. Why doesn't this issue appear on 32-bit platforms?
3. Can the issue actually happen?

Ideally, we should try and fix this as it's become very difficult to find the compilation warnings caused by the introduction of new features with the incredible amount of already existing compilation warnings.

EDIT: example of such a compilation warning:
warning: taking address of packed member of 'struct coherent' may result in an unaligned pointer value

[lgirdwood]

Its better we keep this, what errors do you see, there may be other/better ways to resolve ? i.e. a snippet from the CC errors.

[LaurentiuM1234]

It's mostly stuff like:

/zep_workspace/sof/src/include/sof/coherent.h: In function '__coherent_init_thread':
/zep_workspace/sof/src/include/sof/coherent.h:433:19: warning: taking address of packed member of 'struct coherent' may result in an unaligned pointer value [-Waddress-of-packed-member]
  433 |         list_init(&c->list);

[lyakh]

1. no, no idea why struct coherent has to be packed, I'd say it doesn't. It was that way from day one of the coherent API
2. we currently have only one used of the coherent API: src/lib/ams.c - is ams used at all, are there plans to use it? Does it really need and benefit from the coherent API? @kfrydryx
3. removing that warning seems like not the right fix for this. I found this explanation on [1] why this warning is useful:

When a program accesses a misaligned member of a packed struct, the compiler generates whatever code is necessary to read or write the correct value.

If the address of a misaligned member is stored in a pointer object, dereferencing that pointer doesn't give the compiler an opportunity to generate that extra code.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628

[marc-hb]

64bits warnings can be found in any daily build, example:
https://github.com/thesofproject/sof/actions/runs/6985888340/job/19010712330

[marc-hb]

Frankly I doubt this fix is the way to go,

This warning is indeed important in other configurations. It should be at most disabled for LP64 only with something like this:

--- a/.github/workflows/zephyr.yml
+++ b/.github/workflows/zephyr.yml
@@ -88,7 +88,7 @@ jobs:
           # four 32bits parameters.
           cd workspace && ./sof/zephyr/docker-run.sh /bin/sh -c \
              'ln -s  /opt/toolchains/zephyr-sdk-*  ~/;
-              west build --board mimx93_evk_a55 sof/app'
+              west build --board mimx93_evk_a55 sof/app -- -DEXTRA_CFLAGS=-Wno...'

But even that looks bad.

[marc-hb]

Does "struct coherent" actually need to be packed?

Good question. There seems to be a trend to pack everything by default in SOF without justification. It makes sense of course for IPC, storage and other protocols but is it really needed here? The DSP cores have obviously all the same int and pointer sizes.

It's often possible to pack "by hand", without using an attribute
http://www.catb.org/esr/structure-packing/

Have you tried removing _packed, does is help? How big is the cache line? Feel free to submit a draft PR without it for testing if needed.

Commit 8188387 was especially suspicious because it added a k_mutex from Zephyr, the size and alignment of which seem opaque / not contractually defined.

Another option could be to move SOF objects with well defined sizes at the start of the struct and move the mutex at the end?

[marc-hb]

Some answers thanks to pahole zephyr.elf

32 bits MTL:

struct coherent {
        union {
                struct {
                        struct k_spinlock lock;          /*     0     4 */
                        k_spinlock_key_t key;            /*     4     4 */
                };                                       /*     0     8 */
                struct k_mutex     mutex;                /*     0    20 */
        };                                               /*     0    20 */
        uint8_t                    sleep_allowed;        /*    20     1 */
        uint8_t                    shared;               /*    21     1 */
        uint16_t                   core;                 /*    22     2 */
        struct list_item           list;                 /*    24     8 */

        /* size: 64, cachelines: 1, members: 5 */
        /* padding: 32 */
} __attribute__((__aligned__(64)));

When trying to run pahole on the output of the mimx93_evk_a55 I discovered that coherent.h is not actually use there cause it's single core and not using AMS or anything actually depending on coherent. This makes all this warning spam quite ironic: that .h function is not even in use in that configuration. So maybe a temporary solution is to stop including coherent.h when it's not needed? This postpones fixing coherent.h but not including stuff that's not in use it's the Right Thing anyway and would unblock progress and silence a ton of spam.

Anyway, after adding a struct coherent in some random .c file, pahole gets this:

struct coherent {
        union {
                struct {
                        struct k_spinlock lock;          /*     0     0 */
                        k_spinlock_key_t key;            /*     0     4 */
                };                                       /*     0     4 */
                struct k_mutex     mutex;                /*     0    32 */
        };                                               /*     0    32 */
        uint8_t                    sleep_allowed;        /*    32     1 */
        uint8_t                    shared;               /*    33     1 */
        uint16_t                   core;                 /*    34     2 */
        struct list_item           list;                 /*    36    16 */

        /* size: 64, cachelines: 1, members: 5 */
        /* padding: 12 */
} __attribute__((__aligned__(64)));

Why doesn't this issue appear on 32-bit platforms?

Because 24 is 4 bytes aligned whereas 36 is NOT 8 bytes alignedBut 36 is also 8 bytes aligned, so I don't understand

[marc-hb]

Does "struct coherent" actually need to be packed?

"packed" should probably be dropped there. It feels strange to me to misalign fields in a struct named "coherent". Without looking at it in detail, I would intuitively expect something named "coherent" to be "fast and atomic" - assuming misaligned field access is allowed at all.

On 32bits systems, packed currently does nothing. pahole output is the same after dropping it. That's because all fields are naturally aligned... for now; who knows what k_mutex could become? Moving k_mutex last could be more future-proof.

On the 64 bits system mimx93_evk_a55 (which does not actually use coherent.h right now, see above), dropping packed aligns list_item and changes struct coherent to this:

struct coherent {
        union {
                struct {
                        struct k_spinlock lock;          /*     0     0 */
                        k_spinlock_key_t key;            /*     0     4 */
                };                                       /*     0     4 */
                struct k_mutex     mutex;                /*     0    32 */
        };                                               /*     0    32 */
        uint8_t                    sleep_allowed;        /*    32     1 */
        uint8_t                    shared;               /*    33     1 */
        uint16_t                   core;                 /*    34     2 */

        /* XXX 4 bytes hole, try to pack */

        struct list_item           list;                 /*    40    16 */

        /* size: 64, cachelines: 1, members: 5 */
        /* sum members: 52, holes: 1, sum holes: 4 */
        /* padding: 8 */
} __attribute__((__aligned__(64)));

[LaurentiuM1234]

@marc-hb @lyakh thanks for the clarifications ! For now, I submitted #8528 in which we remove the packed attribute as it seems like it's not necessary? (can someone contradict me here please?). If it turns out to be necessary, I have 2 other options:

1. We make the struct list_item the first attribute in the structure such that it will have an 8B-aligned address. Tested and it also silences the warnings. This should work as long as there's no code assuming a certain order in struct coherent.
2. We go with @marc-hb's suggestion for now and only include coherent.h whenever necessary.

[lyakh]

2. We go with @marc-hb's suggestion for now and only include coherent.h whenever necessary.

@LaurentiuM1234 that should anyway be the case, why would one include headers that one doesn't need (*)?

(*) the definition of "need" is a bit special here: you include those and only those headers types, prototypes, definitions and declarations from which are used in this file. The only exception is pointers to structures, for those you can just forward-declare those structures and avoid pulling in respective headers. This definition might be incomplete, but it should be pretty close.

[paulstelian97]

I've been requested to review this, and my two cents are that approaches where the structure remains packed yet you take the address of a field are sloppy at best and outright undefined. So if I'd actually review, I'd only approve of the separate pull request that removes "packed" (which was merged as of me finding this thread).

You can still consider reordering the structure fields on top of having the packed attribute removed, so that you don't have a hole in the middle of the structure, especially if it helps reducing, if not outright removing, the padding at the end.

So: Do not remove warning, remove the need for "packed" and possibly reorder fields to reduce the size of the struct (removing a hole).

[marc-hb]

I think we also have some consensus on not #including unused .h files.

I've never seen a problem with so many, non-mutually exclusive solutions! :-D

[lgirdwood]

@LaurentiuM1234 fix is to remove the packing from struct coherent, which I think is now merged ?

[marc-hb]

I'm 99% sure this can be closed, @LaurentiuM1234 please re-open if not.