-Wmissing-braces in drivers/firmware/efi/libstub/efi-stub-helper.c

[nathanchance]

After commit 2e5006b7634f ("efi/libstub: measure loaded initrd info into the TPM") in -next:

drivers/firmware/efi/libstub/efi-stub-helper.c:646:2: warning: suggest braces around initialization of subobject [-Wmissing-braces]
        "Linux initrd",
        ^~~~~~~~~~~~~~
        {             }
1 warning generated.

The full variable:

static const struct {
        efi_tcg2_event_t        event_data;
        efi_tcg2_tagged_event_t tagged_event;
        u8                      tagged_event_data[];
} initrd_tcg2_event = {
        {
                sizeof(initrd_tcg2_event) + sizeof("Linux initrd"),
                {
                        sizeof(initrd_tcg2_event.event_data.event_header),
                        EFI_TCG2_EVENT_HEADER_VERSION,
                        9,
                        EV_EVENT_TAG,
                },
        },
        {
                INITRD_EVENT_TAG_ID,
                sizeof("Linux initrd"),
        },
        "Linux initrd",
};

Is clang getting something wrong here? GCC accepts the current form without any warnings. The suggestion resolves it but I want to make sure something is not getting missed here.

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 0bd01da1f0df..a15d30c494b4 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -643,7 +643,7 @@ static const struct {
                INITRD_EVENT_TAG_ID,
                sizeof("Linux initrd"),
        },
-       "Linux initrd",
+       { "Linux initrd" },
 };

 void efi_measure_initrd(unsigned long load_addr, unsigned long load_size)

[bwendling]

It's unclear to me. The standard does mention initializing an array of unknown size in an aggregate, but doesn't appear explicit whether it should be surrounded by braces. I could see a case for both. This is something a language lawyer needs to comment on.

[nathanchance]

Oddly enough, I don't even see where tagged_event_data is actually used... I see a comment about it in struct efi_tcg2_event in https://git.kernel.org/efi/efi/c/4da87c51705815fe1fbd41cc61640bb80da5bc54 but there is nowhere that it is called so I guess it is just there for size.

It seems like the [] is intentional, as trying to change it to u8 * such as:

diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 0bd01da1f0df..b91679817725 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -628,7 +628,7 @@ efi_status_t efi_load_initrd_cmdline(efi_loaded_image_t *image,
 static const struct {
        efi_tcg2_event_t        event_data;
        efi_tcg2_tagged_event_t tagged_event;
-       u8                      tagged_event_data[];
+       u8                      *tagged_event_data;
 } initrd_tcg2_event = {
        {
                sizeof(initrd_tcg2_event) + sizeof("Linux initrd"),

breaks the build with both compilers (GCC below):

$ make -skj"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- distclean defconfig drivers/firmware/efi/
drivers/firmware/efi/libstub/efi-stub-helper.stub.o: absolute symbol references not allowed in the EFI stub
...

cc @ardbiesheuvel, would you be opposed to a patch adding the braces like in my comment above?

[ardbiesheuvel]

I don't mind adding the braces if GCC and Clang both accept it. I just don't see why initializing a char[] using a string literal suddenly needs an additional set of braces when the char[] is a member of a compound type.

As for the u8[] to u8* change: this is obviously incorrect. This struct is defined in the TCG PC client spec, and constitutes a API contract between the EFI stub and the firmware, so we cannot change that.

[nathanchance]

GCC does not warn with or without braces, clang warns without: https://godbolt.org/z/3YcP5sbKq

All of GCC's documentation and nearly all of clang's tests show braces around the initializer so it seems like that is what should be happening; it looks like not having braces is probably an undocumented GCC extension.

I do see one commit in the history that changes how clang handles flexible array initialization to explicitly match GCC but the change notes that there will still be a warning in these cases so it does not sound like clang is wrong here.:

llvm/llvm-project@07d8e3a

https://llvm.org/PR3618

If I am reading it right, section 6.7.2.1 of the C standard does cover initialization of flexible member arrays but it seems like they are not allowed in the way; reading the GCC docs, it seems like this is an extension so I guess we have to go on what GCC allows.

[nickdesaulniers]

@jrtc27 mentioned on IRC:

clang/test/Sema/zero-initializer.c are the cases it's trying to catch
the special case of string literals seems to be unintended fallout

[ardbiesheuvel]

I've fixed this up in the original patch. Should appear in -next tomorrow.

[nathanchance]

Thanks a lot Ard!

[nathanchance]

Now fixed in -next.