Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable -Wmissing-variable-declarations for W=1 #1920

Open
nickdesaulniers opened this issue Aug 21, 2023 · 6 comments
Open

enable -Wmissing-variable-declarations for W=1 #1920

nickdesaulniers opened this issue Aug 21, 2023 · 6 comments
Labels
[BUG] linux A bug that should be fixed in the mainline kernel. good first issue Good for newcomers [PATCH] Exists There is a patch that fixes this issue

Comments

@nickdesaulniers
Copy link
Member

I had sent a patch for this, but didn't have time to follow through due to the srso patch set causing a fair amount of breakage for us that week.

https://lore.kernel.org/llvm/20230807-missing_proto-v2-1-3ae2e188bb0c@google.com/

We should work through cleaning up some of the observed issues first before resending the above.

@nickdesaulniers nickdesaulniers added good first issue Good for newcomers [BUG] linux A bug that should be fixed in the mainline kernel. [PATCH] Exists There is a patch that fixes this issue labels Aug 21, 2023
@JustinStitt
Copy link
Collaborator

JustinStitt commented Aug 29, 2023

A good amount of these are caused by variables with register storage.

llvm/llvm-project#64509 should solve some of these warnings with Clang, right?

@JustinStitt
Copy link
Collaborator

JustinStitt commented Aug 29, 2023

Can someone help me understand what to do in order to fix some of these warnings?

Here's a case I found from an x86/defconfig build:

arch/x86/ia32/audit.c:6:10: warning: no previous extern declaration for non-static variable 'ia32_dir_class' [-Wmissing-variable-declarations]
    6 | unsigned ia32_dir_class[] = {

so it says "no previous extern declaration" so it seems the fix would be to add static and call it a day? right?

arch/x86/ia32/audit.c:6:17: warning: unused variable 'ia32_dir_class' [-Wunused-variable]
    6 | static unsigned ia32_dir_class[] = {
      |                 ^~~~~~~~~~~~~~

Well now we get an unused variable warning and it seems there is an extern:

$ grep "ia32_dir_class"

...
arch/x86/kernel/audit_64.c
66:     extern __u32 ia32_dir_class[];
73:     audit_register_class(AUDIT_CLASS_DIR_WRITE_32, ia32_dir_class);
...

So what am I to make of this? Is the warning wrong? Is there a different solution that I should be going for here?

I wonder if it's caused by the differing types __u32 vs unsigned.

cc @nathanchance

edit: Ah, it looks like CONFIG_IA32_EMULATION=y is required to expose the extern.

@nathanchance
Copy link
Member

It seems like in that case, the declarations in arch/x86/kernel/audit_64.c should be hoisted into a header file (perhaps arch/x86/include/asm/audit.h?) that is included in both arch/x86/kernel/audit_64.c and arch/x86/ia32/audit.c. It also seems like the type of ia32_dir_class should be changed from unsigned to __u32?

@JustinStitt
Copy link
Collaborator

JustinStitt commented Aug 29, 2023

@nathanchance should they be hiding:

#ifdef CONFIG_IA32_EMULATION
	extern __u32 ia32_dir_class[];
	extern __u32 ia32_write_class[];
	extern __u32 ia32_read_class[];
	extern __u32 ia32_chattr_class[];
	extern __u32 ia32_signal_class[];
	audit_register_class(AUDIT_CLASS_WRITE_32, ia32_write_class);
	audit_register_class(AUDIT_CLASS_READ_32, ia32_read_class);
	audit_register_class(AUDIT_CLASS_DIR_WRITE_32, ia32_dir_class);
	audit_register_class(AUDIT_CLASS_CHATTR_32, ia32_chattr_class);
	audit_register_class(AUDIT_CLASS_SIGNAL_32, ia32_signal_class);
#endif

behind this config or have this in a header like you said (without the ifdef)

@nathanchance
Copy link
Member

I don't think the declarations need to be put under an #ifdef in the header file, if that is what you are asking?

@JustinStitt
Copy link
Collaborator

I don't think the declarations need to be put under an #ifdef in the header file, if that is what you are asking?

Yeah, that's what I'm wodering. Why are they in the ifdef at all. I'll test moving them out.

intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Aug 29, 2023
…lass

When building x86 defconfig with Clang-18 I get the following warnings:
| arch/x86/ia32/audit.c:6:10: warning: no previous extern declaration for non-static variable 'ia32_dir_class' [-Wmissing-variable-declarations]
|     6 | unsigned ia32_dir_class[] = {
| arch/x86/ia32/audit.c:11:10: warning: no previous extern declaration for non-static variable 'ia32_chattr_class' [-Wmissing-variable-declarations]
|    11 | unsigned ia32_chattr_class[] = {
| arch/x86/ia32/audit.c:16:10: warning: no previous extern declaration for non-static variable 'ia32_write_class' [-Wmissing-variable-declarations]
|    16 | unsigned ia32_write_class[] = {
| arch/x86/ia32/audit.c:21:10: warning: no previous extern declaration for non-static variable 'ia32_read_class' [-Wmissing-variable-declarations]
|    21 | unsigned ia32_read_class[] = {
| arch/x86/ia32/audit.c:26:10: warning: no previous extern declaration for non-static variable 'ia32_signal_class' [-Wmissing-variable-declarations]
|    26 | unsigned ia32_signal_class[] = {

These warnings occur due to their respective extern declarations being
scoped inside of audit_classes_init as well as only being enabled with
`CONFIG_IA32_EMULATION=y`:
| static int __init audit_classes_init(void)
| {
| #ifdef CONFIG_IA32_EMULATION
| 	extern __u32 ia32_dir_class[];
| 	extern __u32 ia32_write_class[];
| 	extern __u32 ia32_read_class[];
| 	extern __u32 ia32_chattr_class[];
| 	audit_register_class(AUDIT_CLASS_WRITE_32, ia32_write_class);
| 	audit_register_class(AUDIT_CLASS_READ_32, ia32_read_class);
| 	audit_register_class(AUDIT_CLASS_DIR_WRITE_32, ia32_dir_class);
| 	audit_register_class(AUDIT_CLASS_CHATTR_32, ia32_chattr_class);
| #endif
| 	audit_register_class(AUDIT_CLASS_WRITE, write_class);
| 	audit_register_class(AUDIT_CLASS_READ, read_class);
| 	audit_register_class(AUDIT_CLASS_DIR_WRITE, dir_class);
| 	audit_register_class(AUDIT_CLASS_CHATTR, chattr_class);
| 	return 0;
| }

Lift the extern declarations to their own header and resolve scoping
issues (and thus fix the warnings).

Moreover, change __u32 to unsigned so that we match the definitions:
| unsigned ia32_dir_class[] = {
| #include <asm-generic/audit_dir_write.h>
| ~0U
| };
|
| unsigned ia32_chattr_class[] = {
| #include <asm-generic/audit_change_attr.h>
| ~0U
| };
| ...

This patch is similar to commit:
0e5e3d4 ("x86/audit: Fix a -Wmissing-prototypes warning for ia32_classify_syscall()") [1]

Link: https://lore.kernel.org/all/20200516123816.2680-1-b.thiel@posteo.de/ [1]
Link: ClangBuiltLinux#1920
Signed-off-by: Justin Stitt <justinstitt@google.com>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Aug 29, 2023
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: ClangBuiltLinux#1920
Signed-off-by: Justin Stitt <justinstitt@google.com>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Aug 29, 2023
Hi all,

I was looking to get some help on solving this -Wmissing-variable-declarations
warning as there is some hope to turn it on for W=1 soon [1].

When building x86/defconfig with Clang-18 I encounter the following warning:
| init/main.c:189:13: warning: no previous extern declaration for non-static variable 'envp_init' [-Wmissing-variable-declarations]
|   189 | const char *envp_init[MAX_INIT_ENVS+2] = { "HOME=/", "TERM=linux", NULL, };
|       |             ^
| init/main.c:189:7: note: declare 'static' if the variable is not intended to be used outside of this translation unit
|   189 | const char *envp_init[MAX_INIT_ENVS+2] = { "HOME=/", "TERM=linux", NULL, };
|       |       ^

It seems like the obvious solution is to just add the `static` keyword
and be done with it. I suspect, however, that it is not so simple for
the following reasons:

Firstly, `envp_init` is surrounded by two other variables that have been
explicitly marked as `static` which leads me to believe that this one
was intentionally _not_ marked as static for some reason:
| static const char *argv_init[MAX_INIT_ARGS+2] = { "init", NULL, };
| static const char *envp_init[MAX_INIT_ENVS+2] = { "HOME=/", "TERM=linux", NULL, };
| static const char *panic_later, *panic_param;

Secondly, there exists this `extern` declaration for `envp_init`:
| init/do_mounts_initrd.c
| 90:     extern char *envp_init[];

This one is tricky because it seems like I can rename (effectively
remove) this extern symbol entirely and still build a kernel image.

If it truly is the case that this extern declaration works then why does
Clang produce the warning at all?

FWIW, I've tried moving `extern char *envp_init[];` to top-level scope
inside of `do_mounts_initrd.c` which did _not_ work in fixing the
warning.

So all in all, it looks like just adding `static` fixes the warning
(which it does) but I am not sure about the other ramifications of this
patch especially considering the second point I brought up above
regarding the extern declaration already existing (but seemingly not
doing anything).

Any help here would be appreciated!

Link: ClangBuiltLinux#1920 [1]
Signed-off-by: Justin Stitt <justinstitt@google.com>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Aug 30, 2023
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 symbols are declared extern in drivers/firmware/efi/efi.c. Move
the declarations to linux/efi.h to resolve these warnings.

Also, trim a trailing space from efi_set_variable_t typedef.

Link: ClangBuiltLinux#1920
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Justin Stitt <justinstitt@google.com>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Aug 30, 2023
…lass

When building x86 defconfig with Clang-18 I get the following warnings:

  | arch/x86/ia32/audit.c:6:10: warning: no previous extern declaration for non-static variable 'ia32_dir_class' [-Wmissing-variable-declarations]
  |     6 | unsigned ia32_dir_class[] = {
  | arch/x86/ia32/audit.c:11:10: warning: no previous extern declaration for non-static variable 'ia32_chattr_class' [-Wmissing-variable-declarations]
  |    11 | unsigned ia32_chattr_class[] = {
  | arch/x86/ia32/audit.c:16:10: warning: no previous extern declaration for non-static variable 'ia32_write_class' [-Wmissing-variable-declarations]
  |    16 | unsigned ia32_write_class[] = {
  | arch/x86/ia32/audit.c:21:10: warning: no previous extern declaration for non-static variable 'ia32_read_class' [-Wmissing-variable-declarations]
  |    21 | unsigned ia32_read_class[] = {
  | arch/x86/ia32/audit.c:26:10: warning: no previous extern declaration for non-static variable 'ia32_signal_class' [-Wmissing-variable-declarations]
  |    26 | unsigned ia32_signal_class[] = {

These warnings occur due to their respective extern declarations being
scoped inside of audit_classes_init as well as only being enabled with
`CONFIG_IA32_EMULATION=y`:

  | static int __init audit_classes_init(void)
  | {
  | #ifdef CONFIG_IA32_EMULATION
  |	extern __u32 ia32_dir_class[];
  |	extern __u32 ia32_write_class[];
  |	extern __u32 ia32_read_class[];
  |	extern __u32 ia32_chattr_class[];
  |	audit_register_class(AUDIT_CLASS_WRITE_32, ia32_write_class);
  |	audit_register_class(AUDIT_CLASS_READ_32, ia32_read_class);
  |	audit_register_class(AUDIT_CLASS_DIR_WRITE_32, ia32_dir_class);
  |	audit_register_class(AUDIT_CLASS_CHATTR_32, ia32_chattr_class);
  | #endif
  |	audit_register_class(AUDIT_CLASS_WRITE, write_class);
  |	audit_register_class(AUDIT_CLASS_READ, read_class);
  |	audit_register_class(AUDIT_CLASS_DIR_WRITE, dir_class);
  |	audit_register_class(AUDIT_CLASS_CHATTR, chattr_class);
  |	return 0;
  | }

Lift the extern declarations to their own header and resolve scoping
issues (and thus fix the warnings).

Moreover, change __u32 to unsigned so that we match the definitions:

  | unsigned ia32_dir_class[] = {
  | #include <asm-generic/audit_dir_write.h>
  | ~0U
  | };
  |
  | unsigned ia32_chattr_class[] = {
  | #include <asm-generic/audit_change_attr.h>
  | ~0U
  | };
  | ...

This patch is similar to commit:

  0e5e3d4 ("x86/audit: Fix a -Wmissing-prototypes warning for ia32_classify_syscall()") [1]

Signed-off-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/all/20200516123816.2680-1-b.thiel@posteo.de/ [1]
Link: ClangBuiltLinux#1920
Link: https://lore.kernel.org/r/20230829-missingvardecl-audit-v1-1-34efeb7f3539@google.com
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Aug 30, 2023
Hi all,

I was looking to get some help on solving this -Wmissing-variable-declarations
warning as there is some hope to turn it on for W=1 soon [1].

When building x86/defconfig with Clang-18 I encounter the following warning:
| init/main.c:189:13: warning: no previous extern declaration for non-static variable 'envp_init' [-Wmissing-variable-declarations]
|   189 | const char *envp_init[MAX_INIT_ENVS+2] = { "HOME=/", "TERM=linux", NULL, };
|       |             ^
| init/main.c:189:7: note: declare 'static' if the variable is not intended to be used outside of this translation unit
|   189 | const char *envp_init[MAX_INIT_ENVS+2] = { "HOME=/", "TERM=linux", NULL, };
|       |       ^

It seems like the obvious solution is to just add the `static` keyword
and be done with it. I suspect, however, that it is not so simple for
the following reasons:

Firstly, `envp_init` is surrounded by two other variables that have been
explicitly marked as `static` which leads me to believe that this one
was intentionally _not_ marked as static for some reason:
| static const char *argv_init[MAX_INIT_ARGS+2] = { "init", NULL, };
| static const char *envp_init[MAX_INIT_ENVS+2] = { "HOME=/", "TERM=linux", NULL, };
| static const char *panic_later, *panic_param;

Secondly, there exists this `extern` declaration for `envp_init`:
| init/do_mounts_initrd.c
| 90:     extern char *envp_init[];

This one is tricky because it seems like I can rename (effectively
remove) this extern symbol entirely and still build a kernel image.

If it truly is the case that this extern declaration works then why does
Clang produce the warning at all?

FWIW, I've tried moving `extern char *envp_init[];` to top-level scope
inside of `do_mounts_initrd.c` which did _not_ work in fixing the
warning.

So all in all, it looks like just adding `static` fixes the warning
(which it does) but I am not sure about the other ramifications of this
patch especially considering the second point I brought up above
regarding the extern declaration already existing (but seemingly not
doing anything).

Any help here would be appreciated!

Link: ClangBuiltLinux#1920 [1]
Signed-off-by: Justin Stitt <justinstitt@google.com>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Aug 30, 2023
When building x86/defconfig with Clang-18 I encounter the following warning:
| init/main.c:189:13: warning: no previous extern declaration for non-static variable 'envp_init' [-Wmissing-variable-declarations]
|   189 | const char *envp_init[MAX_INIT_ENVS+2] = { "HOME=/", "TERM=linux", NULL, };
| init/main.c:189:7: note: declare 'static' if the variable is not intended to be used outside of this translation unit
|   189 | const char *envp_init[MAX_INIT_ENVS+2] = { "HOME=/", "TERM=linux", NULL, };

Move the extern declaration to a header file so that the compiler can find it
when compiling init/main.c.

Add `const` qualifier to extern declaration so that the types match and
we follow the One-Definition Rule (ODR).

Link: ClangBuiltLinux#1920
Signed-off-by: Justin Stitt <justinstitt@google.com>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Sep 5, 2023
When building x86/defconfig with Clang-18 I encounter the following warning:
| init/main.c:189:13: warning: no previous extern declaration for non-static variable 'envp_init' [-Wmissing-variable-declarations]
|   189 | const char *envp_init[MAX_INIT_ENVS+2] = { "HOME=/", "TERM=linux", NULL, };
| init/main.c:189:7: note: declare 'static' if the variable is not intended to be used outside of this translation unit
|   189 | const char *envp_init[MAX_INIT_ENVS+2] = { "HOME=/", "TERM=linux", NULL, };

Make `envp_init` static and provide `handle_initrd` its own copy.

This silences the warning and makes the code more readable as you no
longer have to track down extern definitions inside of `handle_initrd`.
It is now more self-contained.

Link: ClangBuiltLinux#1920
Signed-off-by: Justin Stitt <justinstitt@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[BUG] linux A bug that should be fixed in the mainline kernel. good first issue Good for newcomers [PATCH] Exists There is a patch that fixes this issue
Projects
None yet
Development

No branches or pull requests

3 participants