Re: [RESEND PATCH v2 2/6] efi_loader: Initial HII database protocols

[Laszlo Ersek <lersek@redhat.com>]

On 01/08/19 16:12, Gao, Liming wrote:

> Last, EFI_HII_KEYBOARD_PACKAGE_HDR structure definition doesn't follow UEFI spec. I remember we ever meet with the compiler issue for below style. GCC49 may complaint it. I need to double confirm. 
> typedef struct {
>   EFI_HII_PACKAGE_HEADER  Header;
>   UINT16                  LayoutCount;
>   EFI_HII_KEYBOARD_LAYOUT Layout[];
> } EFI_HII_KEYBOARD_PACKAGE_HDR;

This is called "flexible array member", and it was introduced in ISO
C99. It is not part of the earlier C standards, and I quite expect
several of the toolchains currently supported by edk2 to reject it.


Code written against earlier releases of the ISO C standard than C99
would use a construct colloquially called the "struct hack", such as

  typedef struct {
    EFI_HII_PACKAGE_HEADER  Header;
    UINT16                  LayoutCount;
    EFI_HII_KEYBOARD_LAYOUT Layout[1];
  } EFI_HII_KEYBOARD_PACKAGE_HDR;

indexing "Layout" with a subscript >= 1. Needless to say, that was
always undefined behavior :) C99 introduced the flexible array member
precisely for covering the "struct hack" use case with a well-defined
construct.

There is no portable, pre-C99 way to actually spell out the Layout field
in the structure definition, with the intended use case. The most
portable approach I can think of would be:

- explain the trailing (nameless) array in a comment,
- instruct programmers to write manual pointer-to-unsigned-char arithmetic,
- once the necessary element is located, copy it into an object actually
declared with the element type, and access it there.

In edk2 we sometimes do steps #1 and #2, which is OK. But, even in those
cases, we almost never do step #3 (because it's both cumbersome and
wastes cycles) -- instead, we favor type-punning.

Whenever I see that, I tell myself, "we disable the effective type rules
with '-fno-strict-aliasing', so this should be fine, in practice. I hope
anyway." :)

Thanks,
Laszlo