[PATCH -next v4 2/2] x86/boot/compressed: Add "-Wall" flag to Makefile

Li Zetao posted 2 patches 3 years, 5 months ago
[PATCH -next v4 2/2] x86/boot/compressed: Add "-Wall" flag to Makefile
Posted by Li Zetao 3 years, 5 months ago
Compressed/Makefile does not have "-Wall" flag, this is the old problem of
x86 not sharing makefiles. Fix by adding "-Wall" flag to Makefile. But when
"-Wall" flag added to Makefile, a few extra warnings were found.

1.
In file included from arch/x86/boot/compressed/misc.c:15:
  In file included from arch/x86/boot/compressed/misc.h:24:
  In file included from ./include/linux/elf.h:6:
  In file included from ./arch/x86/include/asm/elf.h:8:
  In file included from ./include/linux/thread_info.h:60:
  ./arch/x86/include/asm/thread_info.h:175:13: warning: calling
  "__builtin_frame_address" with a nonzero argument is unsafe
  [-Wframe-address]
    oldframe = __builtin_frame_address(1);
               ^~~~~~~~~~~~~~~~~~~~~~~~~~

./arch/x86/include/asm/thread_info.h:177:11: warning: calling
  "__builtin_frame_address" with a nonzero argument is unsafe
  [-Wframe-address]
    frame = __builtin_frame_address(2);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~

This warning is disabled in the main Makefile for this reason so we
should just be able to disable it, adding "frame-address" flag to
Makefile.

2.
arch/x86/boot/compressed/kaslr.c:627:6: warning: unused variable
  "i" [-Wunused-variable]
    int i;
        ^

This happens when CONFIG_MEMORY_HOTREMOVE or CONFIG_ACPI are "n".
Fix by adding "-std=gnu11" flag to Makefile, and we should put
the variable "i" within the for loop.

3.
arch/x86/boot/compressed/acpi.c:23:1: warning: unused function
  "__efi_get_rsdp_addr" [-Wunused-function]

This happens when CONFIG_EFI is disabled for the reason that
function "__efi_get_rsdp_addr" is only called in efi_get_rsdp_addr
when CONFIG_EFI enable. So function "__efi_get_rsdp_addr" should
not be defined when CONFIG_EFI is disabled.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2: Patch is new
v2 -> v3: Resolve extra warnings after "-Wall" flag added
v3 -> v4: Put this patch at the end

 arch/x86/boot/compressed/Makefile | 3 ++-
 arch/x86/boot/compressed/acpi.c   | 5 +++--
 arch/x86/boot/compressed/kaslr.c  | 3 +--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 3a261abb6d15..8918a8306dff 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -35,7 +35,7 @@ targets := vmlinux vmlinux.bin vmlinux.bin.gz vmlinux.bin.bz2 vmlinux.bin.lzma \
 # be valid.
 KBUILD_CFLAGS := -m$(BITS) -O2 $(CLANG_FLAGS)
 KBUILD_CFLAGS += -fno-strict-aliasing -fPIE
-KBUILD_CFLAGS += -Wundef
+KBUILD_CFLAGS += -Wundef -Wall -std=gnu11
 KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
 cflags-$(CONFIG_X86_32) := -march=i386
 cflags-$(CONFIG_X86_64) := -mcmodel=small -mno-red-zone
@@ -44,6 +44,7 @@ KBUILD_CFLAGS += -mno-mmx -mno-sse
 KBUILD_CFLAGS += -ffreestanding -fshort-wchar
 KBUILD_CFLAGS += -fno-stack-protector
 KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
+KBUILD_CFLAGS += $(call cc-disable-warning, frame-address)
 KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
 KBUILD_CFLAGS += -Wno-pointer-sign
 KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 21febd9f21ab..c062a8230e9c 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -19,10 +19,10 @@
  */
 struct mem_vector immovable_mem[MAX_NUMNODES*2];
 
+#ifdef CONFIG_EFI
 static acpi_physical_address
 __efi_get_rsdp_addr(unsigned long cfg_tbl_pa, unsigned int cfg_tbl_len)
 {
-#ifdef CONFIG_EFI
 	unsigned long rsdp_addr;
 
 	/*
@@ -41,9 +41,10 @@ __efi_get_rsdp_addr(unsigned long cfg_tbl_pa, unsigned int cfg_tbl_len)
 		return (acpi_physical_address)rsdp_addr;
 
 	debug_putstr("Error getting RSDP address.\n");
-#endif
+
 	return 0;
 }
+#endif
 
 static acpi_physical_address efi_get_rsdp_addr(void)
 {
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index e476bcbd9b42..4abc9c42cf4d 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -625,7 +625,6 @@ static bool process_mem_region(struct mem_vector *region,
 			       unsigned long minimum,
 			       unsigned long image_size)
 {
-	int i;
 	/*
 	 * If no immovable memory found, or MEMORY_HOTREMOVE disabled,
 	 * use @region directly.
@@ -645,7 +644,7 @@ static bool process_mem_region(struct mem_vector *region,
 	 * If immovable memory found, filter the intersection between
 	 * immovable memory and @region.
 	 */
-	for (i = 0; i < num_immovable_mem; i++) {
+	for (int i = 0; i < num_immovable_mem; i++) {
 		u64 start, end, entry_end, region_end;
 		struct mem_vector entry;
 
-- 
2.34.1
Re: [PATCH -next v4 2/2] x86/boot/compressed: Add "-Wall" flag to Makefile
Posted by Borislav Petkov 3 years, 5 months ago
On Tue, Oct 11, 2022 at 01:29:04AM +0000, Li Zetao wrote:
> This warning is disabled in the main Makefile for this reason so we

Who's "we"?

Please use passive voice in your commit message: no "we" or "I", etc,
and describe your changes in imperative mood.

Also, pls read section "2) Describe your changes" in
Documentation/process/submitting-patches.rst for more details.

Also, see section "Changelog" in
Documentation/process/maintainer-tip.rst

Bottom line is: personal pronouns are ambiguous in text, especially with
so many parties/companies/etc developing the kernel so let's avoid them
please.

In any case, it is getting there but this is not how you structure a
patchset like this. The proper order is:

1st patch: Remove unused variables

2nd patch: Fix -Wframe-address warning and explain why

3rd patch: Fix arch/x86/boot/compressed/acpi.c:23:1: warning: unused function "__efi_get_rsdp_addr" [-Wunused-function]

Btw, I also get with -Wall

In function ‘find_trampoline_placement’,
    inlined from ‘paging_prepare’ at arch/x86/boot/compressed/pgtable_64.c:135:35:
arch/x86/boot/compressed/pgtable_64.c:60:30: warning: array subscript 0 is outside array bounds of ‘short unsigned int[0]’ [-Warray-bounds]
   60 |                 ebda_start = *(unsigned short *)0x40e << 4;
      |                              ^~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/boot/compressed/pgtable_64.c:61:30: warning: array subscript 0 is outside array bounds of ‘short unsigned int[0]’ [-Warray-bounds]
   61 |                 bios_start = *(unsigned short *)0x413 << 10;
      |                              ^~~~~~~~~~~~~~~~~~~~~~~~
In function ‘bios_get_rsdp_addr’,
    inlined from ‘get_rsdp_addr’ at arch/x86/boot/compressed/acpi.c:163:8:
arch/x86/boot/compressed/acpi.c:130:19: warning: array subscript 0 is outside array bounds of ‘u16[0]’ {aka ‘short unsigned int[]’} [-Warray-bounds]
  130 |         address = *(u16 *)ACPI_EBDA_PTR_LOCATION;

That should be a separate patch too.

Nth patch(es): Fix more warnings after building at least

	"allnoconfig" "defconfig" "allmodconfig" "allyesconfig"

for both 32-bit and 64-bit. A bunch of randconfigs on a big machine
would help a lot.

N + kth patch: Add -std=gnu11

X. Final patch: Add -Wall to compressed/Makefile.

In that order so that no warnings are seen when doing random builds.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH -next v4 2/2] x86/boot/compressed: Add "-Wall" flag to Makefile
Posted by Nick Desaulniers 3 years, 5 months ago
On Mon, Oct 10, 2022 at 6:32 PM Li Zetao <lizetao1@huawei.com> wrote:
>
> Compressed/Makefile does not have "-Wall" flag, this is the old problem of
> x86 not sharing makefiles. Fix by adding "-Wall" flag to Makefile. But when
> "-Wall" flag added to Makefile, a few extra warnings were found.
>
> 1.
> In file included from arch/x86/boot/compressed/misc.c:15:
>   In file included from arch/x86/boot/compressed/misc.h:24:
>   In file included from ./include/linux/elf.h:6:
>   In file included from ./arch/x86/include/asm/elf.h:8:
>   In file included from ./include/linux/thread_info.h:60:
>   ./arch/x86/include/asm/thread_info.h:175:13: warning: calling
>   "__builtin_frame_address" with a nonzero argument is unsafe
>   [-Wframe-address]
>     oldframe = __builtin_frame_address(1);
>                ^~~~~~~~~~~~~~~~~~~~~~~~~~
>
> ./arch/x86/include/asm/thread_info.h:177:11: warning: calling
>   "__builtin_frame_address" with a nonzero argument is unsafe
>   [-Wframe-address]
>     frame = __builtin_frame_address(2);
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This warning is disabled in the main Makefile for this reason so we
> should just be able to disable it, adding "frame-address" flag to
> Makefile.
>
> 2.
> arch/x86/boot/compressed/kaslr.c:627:6: warning: unused variable
>   "i" [-Wunused-variable]
>     int i;
>         ^
>
> This happens when CONFIG_MEMORY_HOTREMOVE or CONFIG_ACPI are "n".
> Fix by adding "-std=gnu11" flag to Makefile, and we should put
> the variable "i" within the for loop.
>
> 3.
> arch/x86/boot/compressed/acpi.c:23:1: warning: unused function
>   "__efi_get_rsdp_addr" [-Wunused-function]
>
> This happens when CONFIG_EFI is disabled for the reason that
> function "__efi_get_rsdp_addr" is only called in efi_get_rsdp_addr
> when CONFIG_EFI enable. So function "__efi_get_rsdp_addr" should
> not be defined when CONFIG_EFI is disabled.
>
> Signed-off-by: Li Zetao <lizetao1@huawei.com>

I gave this a quick spin with clang to ensure that I didn't observe
new warnings produced via -Wall:

$ make LLVM=1 -j128 -s defconfig all
$ make LLVM=1 ARCH=i386 -j128 -s defconfig all

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
> v1 -> v2: Patch is new
> v2 -> v3: Resolve extra warnings after "-Wall" flag added
> v3 -> v4: Put this patch at the end
>
>  arch/x86/boot/compressed/Makefile | 3 ++-
>  arch/x86/boot/compressed/acpi.c   | 5 +++--
>  arch/x86/boot/compressed/kaslr.c  | 3 +--
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 3a261abb6d15..8918a8306dff 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -35,7 +35,7 @@ targets := vmlinux vmlinux.bin vmlinux.bin.gz vmlinux.bin.bz2 vmlinux.bin.lzma \
>  # be valid.
>  KBUILD_CFLAGS := -m$(BITS) -O2 $(CLANG_FLAGS)
>  KBUILD_CFLAGS += -fno-strict-aliasing -fPIE
> -KBUILD_CFLAGS += -Wundef
> +KBUILD_CFLAGS += -Wundef -Wall -std=gnu11
>  KBUILD_CFLAGS += -DDISABLE_BRANCH_PROFILING
>  cflags-$(CONFIG_X86_32) := -march=i386
>  cflags-$(CONFIG_X86_64) := -mcmodel=small -mno-red-zone
> @@ -44,6 +44,7 @@ KBUILD_CFLAGS += -mno-mmx -mno-sse
>  KBUILD_CFLAGS += -ffreestanding -fshort-wchar
>  KBUILD_CFLAGS += -fno-stack-protector
>  KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
> +KBUILD_CFLAGS += $(call cc-disable-warning, frame-address)
>  KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
>  KBUILD_CFLAGS += -Wno-pointer-sign
>  KBUILD_CFLAGS += $(call cc-option,-fmacro-prefix-map=$(srctree)/=)
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> index 21febd9f21ab..c062a8230e9c 100644
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -19,10 +19,10 @@
>   */
>  struct mem_vector immovable_mem[MAX_NUMNODES*2];
>
> +#ifdef CONFIG_EFI
>  static acpi_physical_address
>  __efi_get_rsdp_addr(unsigned long cfg_tbl_pa, unsigned int cfg_tbl_len)
>  {
> -#ifdef CONFIG_EFI
>         unsigned long rsdp_addr;
>
>         /*
> @@ -41,9 +41,10 @@ __efi_get_rsdp_addr(unsigned long cfg_tbl_pa, unsigned int cfg_tbl_len)
>                 return (acpi_physical_address)rsdp_addr;
>
>         debug_putstr("Error getting RSDP address.\n");
> -#endif
> +
>         return 0;
>  }
> +#endif
>
>  static acpi_physical_address efi_get_rsdp_addr(void)
>  {
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index e476bcbd9b42..4abc9c42cf4d 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -625,7 +625,6 @@ static bool process_mem_region(struct mem_vector *region,
>                                unsigned long minimum,
>                                unsigned long image_size)
>  {
> -       int i;
>         /*
>          * If no immovable memory found, or MEMORY_HOTREMOVE disabled,
>          * use @region directly.
> @@ -645,7 +644,7 @@ static bool process_mem_region(struct mem_vector *region,
>          * If immovable memory found, filter the intersection between
>          * immovable memory and @region.
>          */
> -       for (i = 0; i < num_immovable_mem; i++) {
> +       for (int i = 0; i < num_immovable_mem; i++) {
>                 u64 start, end, entry_end, region_end;
>                 struct mem_vector entry;
>
> --
> 2.34.1
>


-- 
Thanks,
~Nick Desaulniers