[v1] efi: fix -Wmissing-variable-declarations warning

[PATCH] efi: fix -Wmissing-variable-declarations warning

Posted by Justin Stitt 2 years, 5 months ago

When building x86/defconfig with Clang-18 I encounter the following warnings:
| arch/x86/platform/efi/efi.c:934:23: warning: no previous extern declaration for non-static variable 'efi_attr_fw_vendor' [-Wmissing-variable-declarations]
|   934 | struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
| arch/x86/platform/efi/efi.c:935:23: warning: no previous extern declaration for non-static variable 'efi_attr_runtime' [-Wmissing-variable-declarations]
|   935 | struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
| arch/x86/platform/efi/efi.c:936:23: warning: no previous extern declaration for non-static variable 'efi_attr_config_table' [-Wmissing-variable-declarations]
|   936 | struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);

These variables are not externally declared anywhere (AFAIK) so let's
add the static keyword and ensure we follow the ODR.

Link: https://github.com/ClangBuiltLinux/linux/issues/1920
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
When building x86/defconfig with Clang-18 I encounter the following warnings:
| arch/x86/platform/efi/efi.c:934:23: warning: no previous extern declaration for non-static variable 'efi_attr_fw_vendor' [-Wmissing-variable-declarations]
|   934 | struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
| arch/x86/platform/efi/efi.c:935:23: warning: no previous extern declaration for non-static variable 'efi_attr_runtime' [-Wmissing-variable-declarations]
|   935 | struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
| arch/x86/platform/efi/efi.c:936:23: warning: no previous extern declaration for non-static variable 'efi_attr_config_table' [-Wmissing-variable-declarations]
|   936 | struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
---
Note: build-tested.
---
 arch/x86/platform/efi/efi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e9f99c56f3ce..30c354c52ad4 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -931,9 +931,9 @@ EFI_ATTR_SHOW(fw_vendor);
 EFI_ATTR_SHOW(runtime);
 EFI_ATTR_SHOW(config_table);
 
-struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
-struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
-struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
+static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
+static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
+static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
 
 umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
 {

---
base-commit: 706a741595047797872e669b3101429ab8d378ef
change-id: 20230829-missingvardecl-efi-59306aacfed4

Best regards,
--
Justin Stitt <justinstitt@google.com>

Re: [PATCH] efi: fix -Wmissing-variable-declarations warning

Posted by Ard Biesheuvel 2 years, 5 months ago

Hi Justin,

On Wed, 30 Aug 2023 at 00:54, Justin Stitt <justinstitt@google.com> wrote:
>
> When building x86/defconfig with Clang-18 I encounter the following warnings:
> | arch/x86/platform/efi/efi.c:934:23: warning: no previous extern declaration for non-static variable 'efi_attr_fw_vendor' [-Wmissing-variable-declarations]
> |   934 | struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> | arch/x86/platform/efi/efi.c:935:23: warning: no previous extern declaration for non-static variable 'efi_attr_runtime' [-Wmissing-variable-declarations]
> |   935 | struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> | arch/x86/platform/efi/efi.c:936:23: warning: no previous extern declaration for non-static variable 'efi_attr_config_table' [-Wmissing-variable-declarations]
> |   936 | struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
>
> These variables are not externally declared anywhere (AFAIK)

They are:

drivers/firmware/efi/efi.c:extern __weak struct kobj_attribute
efi_attr_fw_vendor;
drivers/firmware/efi/efi.c:extern __weak struct kobj_attribute efi_attr_runtime;
drivers/firmware/efi/efi.c:extern __weak struct kobj_attribute
efi_attr_config_table;

> so let's
> add the static keyword and ensure we follow the ODR.
>

One Definition Rule, right? Better to spell that out.

> Link: https://github.com/ClangBuiltLinux/linux/issues/1920
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> When building x86/defconfig with Clang-18 I encounter the following warnings:
> | arch/x86/platform/efi/efi.c:934:23: warning: no previous extern declaration for non-static variable 'efi_attr_fw_vendor' [-Wmissing-variable-declarations]
> |   934 | struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> | arch/x86/platform/efi/efi.c:935:23: warning: no previous extern declaration for non-static variable 'efi_attr_runtime' [-Wmissing-variable-declarations]
> |   935 | struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> | arch/x86/platform/efi/efi.c:936:23: warning: no previous extern declaration for non-static variable 'efi_attr_config_table' [-Wmissing-variable-declarations]
> |   936 | struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);

This is duplicated. Is this a b4 fail?

> ---
> Note: build-tested.
> ---
>  arch/x86/platform/efi/efi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index e9f99c56f3ce..30c354c52ad4 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -931,9 +931,9 @@ EFI_ATTR_SHOW(fw_vendor);
>  EFI_ATTR_SHOW(runtime);
>  EFI_ATTR_SHOW(config_table);
>
> -struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> -struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> -struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> +static struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> +static struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> +static struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
>
>  umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
>  {
>

This won't work.

Those variables are referenced via weak references in generic code.
The idea is that the weak references resolve to NULL pointers on
architectures other than x86, terminating the array early and hiding
the non-existent variables.

Making them static in arch/x86/platform/efi/efi.c means that these
references will remain unsatisfied, and so the variables will no
longer be exposed on x86 either.

Re: [PATCH] efi: fix -Wmissing-variable-declarations warning

Posted by Andy Shevchenko 2 years, 5 months ago

On Wed, Aug 30, 2023 at 2:04 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> On Wed, 30 Aug 2023 at 00:54, Justin Stitt <justinstitt@google.com> wrote:

...

> > When building x86/defconfig with Clang-18 I encounter the following warnings:
> > | arch/x86/platform/efi/efi.c:934:23: warning: no previous extern declaration for non-static variable 'efi_attr_fw_vendor' [-Wmissing-variable-declarations]
> > |   934 | struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > | arch/x86/platform/efi/efi.c:935:23: warning: no previous extern declaration for non-static variable 'efi_attr_runtime' [-Wmissing-variable-declarations]
> > |   935 | struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > | arch/x86/platform/efi/efi.c:936:23: warning: no previous extern declaration for non-static variable 'efi_attr_config_table' [-Wmissing-variable-declarations]
> > |   936 | struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> >
> > These variables are not externally declared anywhere (AFAIK)
>
> They are:
>
> drivers/firmware/efi/efi.c:extern __weak struct kobj_attribute
> efi_attr_fw_vendor;
> drivers/firmware/efi/efi.c:extern __weak struct kobj_attribute efi_attr_runtime;
> drivers/firmware/efi/efi.c:extern __weak struct kobj_attribute
> efi_attr_config_table;
>
> > so let's add the static keyword and ensure we follow the ODR.

> This won't work.
>
> Those variables are referenced via weak references in generic code.
> The idea is that the weak references resolve to NULL pointers on
> architectures other than x86, terminating the array early and hiding
> the non-existent variables.
>
> Making them static in arch/x86/platform/efi/efi.c means that these
> references will remain unsatisfied, and so the variables will no
> longer be exposed on x86 either.

So it means that we have no definitions in the header for these, right?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] efi: fix -Wmissing-variable-declarations warning

Posted by Ard Biesheuvel 2 years, 5 months ago

On Wed, 30 Aug 2023 at 15:25, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Wed, Aug 30, 2023 at 2:04 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > On Wed, 30 Aug 2023 at 00:54, Justin Stitt <justinstitt@google.com> wrote:
>
> ...
>
> > > When building x86/defconfig with Clang-18 I encounter the following warnings:
> > > | arch/x86/platform/efi/efi.c:934:23: warning: no previous extern declaration for non-static variable 'efi_attr_fw_vendor' [-Wmissing-variable-declarations]
> > > |   934 | struct kobj_attribute efi_attr_fw_vendor = __ATTR_RO(fw_vendor);
> > > | arch/x86/platform/efi/efi.c:935:23: warning: no previous extern declaration for non-static variable 'efi_attr_runtime' [-Wmissing-variable-declarations]
> > > |   935 | struct kobj_attribute efi_attr_runtime = __ATTR_RO(runtime);
> > > | arch/x86/platform/efi/efi.c:936:23: warning: no previous extern declaration for non-static variable 'efi_attr_config_table' [-Wmissing-variable-declarations]
> > > |   936 | struct kobj_attribute efi_attr_config_table = __ATTR_RO(config_table);
> > >
> > > These variables are not externally declared anywhere (AFAIK)
> >
> > They are:
> >
> > drivers/firmware/efi/efi.c:extern __weak struct kobj_attribute
> > efi_attr_fw_vendor;
> > drivers/firmware/efi/efi.c:extern __weak struct kobj_attribute efi_attr_runtime;
> > drivers/firmware/efi/efi.c:extern __weak struct kobj_attribute
> > efi_attr_config_table;
> >
> > > so let's add the static keyword and ensure we follow the ODR.
>
> > This won't work.
> >
> > Those variables are referenced via weak references in generic code.
> > The idea is that the weak references resolve to NULL pointers on
> > architectures other than x86, terminating the array early and hiding
> > the non-existent variables.
> >
> > Making them static in arch/x86/platform/efi/efi.c means that these
> > references will remain unsatisfied, and so the variables will no
> > longer be exposed on x86 either.
>
> So it means that we have no definitions in the header for these, right?
>

Indeed.

If there are better ways of fixing this that don't involve weak
references, I am also fine with that, but just moving the existing
extern declarations into linux/efi.h is probably the easiest approach
here.