[PATCH 8/9] clang: efi payload: Silence unaligned access warning

[Ilias Apalodimas]

Hi

On Thu, 6 Apr 2023 at 09:16, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 4/6/23 01:48, Tom Rini wrote:
> > When building with clang we see this warning:
> > field guid within 'struct efi_hii_keyboard_layout' is less aligned than 'efi_guid_t' and is usually due to 'struct efi_hii_keyboard_layout' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> >
> > Which is correct and true as EFI payloads are by specification allowed
> > to do unaligned access. And so to silence the warning here and only
> > here, we make use of #pragma to push/pop turning off the warning.
> >
> > Signed-off-by: Tom Rini <trini at konsulko.com>
> > ---
> > Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>
> > Cc: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > ---
> >   include/efi_api.h | 12 ++++++++++++
> >   1 file changed, 12 insertions(+)
> >
> > diff --git a/include/efi_api.h b/include/efi_api.h
> > index dc6e5ce236c9..f3bcbf593bea 100644
> > --- a/include/efi_api.h
> > +++ b/include/efi_api.h
> > @@ -1168,9 +1168,21 @@ struct efi_key_descriptor {
> >       u16 affected_attribute;
> >   } __packed;
> >
> > +/* The warniing:
> > + * field guid within 'struct efi_hii_keyboard_layout' is less aligned than 'efi_guid_t' and is usually due to 'struct efi_hii_keyboard_layout' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> > + * is intentional due to EFI requiring unaligned access to be supported.
> > + */
> >   struct efi_hii_keyboard_layout {
> >       u16 layout_length;
> > +#ifdef CONFIG_CC_IS_CLANG
> > +#pragma clang diagnostic push
> > +#pragma clang diagnostic ignored "-Wunaligned-access"
> > +#endif
> >       efi_guid_t guid;
> > +#ifdef CONFIG_CC_IS_CLANG
> > +#pragma clang diagnostic pop
> > +#endif
>
> I don't think that the clang warning should be suppressed. We should
> replace efi_guid_t by u8[16] instead. This will force us to take the
> missing alignment into account when accessing the component in future.
>
> -       efi_guid_t guid;
> +       u8 guid[16];

I think what Heinrich suggests is fine, the EFI spec itself says that
efi_guid must be 8-byte aligned unless otherwise specified.
But we have another issue here.  We aren't supposed to include structs
that contain flexible length arrays into another array or struct.
Quoting the C spec [0]
Such a structure (and any union containing, possibly recursively, a
member that is such a structure) shall not be a member of a structure
or an
element of an array.  IOW efi_hii_keyboard_layout should not include
struct efi_key_descriptor descriptors[]; since we include it at
efi_hii_keyboard_package

Regards
/Ilias

[0] https://www.dii.uchile.cl/~daespino/files/Iso_C_1999_definition.pdf
chapter 6.7.2.1

>
> Best regards
>
> Heinrich
>
> > +
> >       u32 layout_descriptor_string_offset;
> >       u8 descriptor_count;
> >       struct efi_key_descriptor descriptors[];
>