Dear coreboot folks,
*gcc-12* snapshot packages are available in Debian sid/unstable, and build testing coreboot with that shows new array out of bounds warnings. For qemu/i440fx:
``` $ gcc-12 --version gcc-12 (Debian 12-20220313-1) 12.0.1 20220314 (experimental) [master r12-7638-g823b3b79cd2] Copyright (C) 2022 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ make V=1 […] x86_64-linux-gnu-gcc-12 -MMD -Isrc -Isrc/include -Isrc/commonlib/include -Isrc/commonlib/bsd/include -Ibuild -I3rdparty/vboot/firmware/include -include src/include/kconfig.h -include src/include/rules.h -include src/commonlib/bsd/include/commonlib/bsd/compiler.h -I3rdparty -D__BUILD_DIR__="build" -Isrc/arch/x86/include -D__ARCH_x86_32__ -pipe -g -nostdinc -std=gnu11 -nostdlib -Wall -Wundef -Wstrict-prototypes -Wmissing-prototypes -Wwrite-strings -Wredundant-decls -Wno-trigraphs -Wimplicit-fallthrough -Wshadow -Wdate-time -Wtype-limits -Wvla -Wdangling-else -fno-common -ffreestanding -fno-builtin -fomit-frame-pointer -fstrict-aliasing -ffunction-sections -fdata-sections -fno-pie -Wno-packed-not-aligned -fconserve-stack -Wnull-dereference -Wreturn-type -Wlogical-op -Wduplicated-cond -Wno-unused-but-set-variable -Werror -Os -Wno-address-of-packed-member -m32 -Wl,-b,elf32-i386 -Wl,-melf_i386 -m32 -fuse-ld=bfd -fno-stack-protector -Wl,--build-id=none -fno-delete-null-pointer-checks -Wlogical-op -march=i686 -mno-mmx -MT build/bootblock/cpu/x86/lapic/lapic.o -D__BOOTBLOCK__ -c -o build/bootblock/cpu/x86/lapic/lapic.o src/cpu/x86/lapic/lapic.c In file included from src/include/cpu/x86/lapic.h:4, from src/cpu/x86/lapic/lapic.c:5: In function 'read32', inlined from 'xapic_read' at src/include/cpu/x86/lapic.h:13:9, inlined from 'lapic_read' at src/include/cpu/x86/lapic.h:80:10, inlined from 'lapicid' at src/include/cpu/x86/lapic.h:138:21, inlined from 'enable_lapic' at src/cpu/x86/lapic/lapic.c:41:3: src/arch/x86/include/arch/mmio.h:20:16: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 20 | return *((volatile uint32_t *)(addr)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In function 'read32', inlined from 'xapic_read' at src/include/cpu/x86/lapic.h:13:9, inlined from 'lapic_update32' at src/include/cpu/x86/lapic.h:103:11, inlined from 'setup_lapic_interrupts' at src/cpu/x86/lapic/lapic.c:73:2: src/arch/x86/include/arch/mmio.h:20:16: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 20 | return *((volatile uint32_t *)(addr)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In function 'write32', inlined from 'xapic_write' at src/include/cpu/x86/lapic.h:18:2, inlined from 'lapic_update32' at src/include/cpu/x86/lapic.h:106:3, inlined from 'setup_lapic_interrupts' at src/cpu/x86/lapic/lapic.c:73:2: src/arch/x86/include/arch/mmio.h:40:40: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 40 | *((volatile uint32_t *)(addr)) = value; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ In function 'read32', inlined from 'xapic_read' at src/include/cpu/x86/lapic.h:13:9, inlined from 'lapic_update32' at src/include/cpu/x86/lapic.h:103:11, inlined from 'setup_lapic_interrupts' at src/cpu/x86/lapic/lapic.c:77:2: src/arch/x86/include/arch/mmio.h:20:16: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 20 | return *((volatile uint32_t *)(addr)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In function 'write32', inlined from 'xapic_write' at src/include/cpu/x86/lapic.h:18:2, inlined from 'lapic_update32' at src/include/cpu/x86/lapic.h:106:3, inlined from 'setup_lapic_interrupts' at src/cpu/x86/lapic/lapic.c:77:2: src/arch/x86/include/arch/mmio.h:40:40: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 40 | *((volatile uint32_t *)(addr)) = value; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ In function 'read32', inlined from 'xapic_read' at src/include/cpu/x86/lapic.h:13:9, inlined from 'lapic_update32' at src/include/cpu/x86/lapic.h:103:11, inlined from 'setup_lapic_interrupts' at src/cpu/x86/lapic/lapic.c:86:3: src/arch/x86/include/arch/mmio.h:20:16: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 20 | return *((volatile uint32_t *)(addr)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In function 'write32', inlined from 'xapic_write' at src/include/cpu/x86/lapic.h:18:2, inlined from 'lapic_update32' at src/include/cpu/x86/lapic.h:106:3, inlined from 'setup_lapic_interrupts' at src/cpu/x86/lapic/lapic.c:86:3: src/arch/x86/include/arch/mmio.h:40:40: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 40 | *((volatile uint32_t *)(addr)) = value; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ In function 'read32', inlined from 'xapic_read' at src/include/cpu/x86/lapic.h:13:9, inlined from 'lapic_update32' at src/include/cpu/x86/lapic.h:103:11, inlined from 'setup_lapic_interrupts' at src/cpu/x86/lapic/lapic.c:84:3: src/arch/x86/include/arch/mmio.h:20:16: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 20 | return *((volatile uint32_t *)(addr)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In function 'write32', inlined from 'xapic_write' at src/include/cpu/x86/lapic.h:18:2, inlined from 'lapic_update32' at src/include/cpu/x86/lapic.h:106:3, inlined from 'setup_lapic_interrupts' at src/cpu/x86/lapic/lapic.c:84:3: src/arch/x86/include/arch/mmio.h:40:40: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 40 | *((volatile uint32_t *)(addr)) = value; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ In function 'read32', inlined from 'xapic_read' at src/include/cpu/x86/lapic.h:13:9, inlined from 'lapic_update32' at src/include/cpu/x86/lapic.h:103:11, inlined from 'setup_lapic_interrupts' at src/cpu/x86/lapic/lapic.c:89:2: src/arch/x86/include/arch/mmio.h:20:16: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 20 | return *((volatile uint32_t *)(addr)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In function 'write32', inlined from 'xapic_write' at src/include/cpu/x86/lapic.h:18:2, inlined from 'lapic_update32' at src/include/cpu/x86/lapic.h:106:3, inlined from 'setup_lapic_interrupts' at src/cpu/x86/lapic/lapic.c:89:2: src/arch/x86/include/arch/mmio.h:40:40: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 40 | *((volatile uint32_t *)(addr)) = value; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ cc1: all warnings being treated as errors
```
I have no idea, if it’s valid or not, but judging from the discussion in the GCC bug report *[11/12 Regression] gcc-11 -Warray-bounds or -Wstringop-overread warning when accessing a pointer from integer literal* [1], the new behavior is controversial.
Could more knowledgeable people please take a look, and give feedback, so in case we consider it a regression for coreboot, we can give feedback to the GCC folks?
Kind regards,
Paul
Paul Menzel wrote:
x86_64-linux-gnu-gcc-12 .. -std=gnu11 ..
..
In file included from src/include/cpu/x86/lapic.h:4, from src/cpu/x86/lapic/lapic.c:5: In function 'read32', inlined from 'xapic_read' at src/include/cpu/x86/lapic.h:13:9, inlined from 'lapic_read' at src/include/cpu/x86/lapic.h:80:10, inlined from 'lapicid' at src/include/cpu/x86/lapic.h:138:21, inlined from 'enable_lapic' at src/cpu/x86/lapic/lapic.c:41:3: src/arch/x86/include/arch/mmio.h:20:16: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 20 | return *((volatile uint32_t *)(addr)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
..
I have no idea, if it’s valid or not
gcc-12 is technically correct that accessing element 0 (the first element) in an array with 0 elements is out of bounds.
But in practice it's not a problem for our code, because these are all uint32_t pointers to memory mapped registers (hint: volatile).
So our code is somehow incorrect since these are, in fact, not arrays with 0 elements. The question is why we use them.
One reason can be that older C standards didn't allow arrays to have an unknown size and nobody fixed that using coccinelle when bumping -std=.
Another is that an array with an unknown size must always be the last member in structs, and that there can only ever be one of those and using a 0-sized array can work around that, but IMHO isn't really very pretty.
If possible it would be nicer to use (const?) volatile uint32_t * instead of const volatile void[0], then the casts would also disappear.
Some analysis is required to understand the 0-sized arrays rationale.
Or maybe someone remembers? :)
Kind regards
//Peter
Hi Peter, Paul,
On 15.03.22 02:18, Peter Stuge wrote:
Paul Menzel wrote:
x86_64-linux-gnu-gcc-12 .. -std=gnu11 ..
..
In file included from src/include/cpu/x86/lapic.h:4, from src/cpu/x86/lapic/lapic.c:5: In function 'read32', inlined from 'xapic_read' at src/include/cpu/x86/lapic.h:13:9, inlined from 'lapic_read' at src/include/cpu/x86/lapic.h:80:10, inlined from 'lapicid' at src/include/cpu/x86/lapic.h:138:21, inlined from 'enable_lapic' at src/cpu/x86/lapic/lapic.c:41:3: src/arch/x86/include/arch/mmio.h:20:16: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 20 | return *((volatile uint32_t *)(addr)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
..
I have no idea, if it’s valid or not
gcc-12 is technically correct that accessing element 0 (the first element) in an array with 0 elements is out of bounds.
But in practice it's not a problem for our code, because these are all uint32_t pointers to memory mapped registers (hint: volatile).
So our code is somehow incorrect since these are, in fact, not arrays with 0 elements. The question is why we use them.
in this instance, there is no literal `[0]` involved. I'm not sure why GCC makes it up. Starting to manually inline at lapicid(), this is what our code does:
unsigned int lapic_read_param_reg = 0x020 /* LAPIC_ID */; unsigned int xapic_read_param_reg = lapic_read_reg; const volatile void *read32_param_addr = (volatile void *)(uintptr_t) (0xfee00000 /* LAPIC_DEFAULT_BASE */ + xapic_read_reg);
uint32_t lapicid_local_lapicid = *((volatile uint32_t *)(read32_param_addr));
(Where I named function parameters `x` `<func>_param_<x>` and local variables `y` <func>_local_<y>`.)
IIRC, I read in some issue tracker about this or something similar before, and there the solution was to use `volatile` which we do already. I don't see what our code could do better (beside adding some more `const` and removing odd parentheses). *shrug*
Nico
On 15.03.22 12:25, Nico Huber wrote:
Hi Peter, Paul,
On 15.03.22 02:18, Peter Stuge wrote:
Paul Menzel wrote:
x86_64-linux-gnu-gcc-12 .. -std=gnu11 ..
..
In file included from src/include/cpu/x86/lapic.h:4, from src/cpu/x86/lapic/lapic.c:5: In function 'read32', inlined from 'xapic_read' at src/include/cpu/x86/lapic.h:13:9, inlined from 'lapic_read' at src/include/cpu/x86/lapic.h:80:10, inlined from 'lapicid' at src/include/cpu/x86/lapic.h:138:21, inlined from 'enable_lapic' at src/cpu/x86/lapic/lapic.c:41:3: src/arch/x86/include/arch/mmio.h:20:16: error: array subscript 0 is outside array bounds of 'const volatile void[0]' [-Werror=array-bounds] 20 | return *((volatile uint32_t *)(addr)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
..
I have no idea, if it’s valid or not
gcc-12 is technically correct that accessing element 0 (the first element) in an array with 0 elements is out of bounds.
But in practice it's not a problem for our code, because these are all uint32_t pointers to memory mapped registers (hint: volatile).
So our code is somehow incorrect since these are, in fact, not arrays with 0 elements. The question is why we use them.
in this instance, there is no literal `[0]` involved. I'm not sure why GCC makes it up. Starting to manually inline at lapicid(), this is what our code does:
unsigned int lapic_read_param_reg = 0x020 /* LAPIC_ID */; unsigned int xapic_read_param_reg = lapic_read_reg; const volatile void *read32_param_addr = (volatile void *)(uintptr_t) (0xfee00000 /* LAPIC_DEFAULT_BASE */ + xapic_read_reg);
uint32_t lapicid_local_lapicid = *((volatile uint32_t *)(read32_param_addr));
(Where I named function parameters `x` `<func>_param_<x>` and local variables `y` <func>_local_<y>`.)
IIRC, I read in some issue tracker about this or something similar before, and there the solution was to use `volatile` which we do already. I don't see what our code could do better (beside adding some more `const` and removing odd parentheses). *shrug*
Turned out it was the same bug report Paul linked [1]. The suggestion is to make the pointer variables `volatile`, not the pointed to data. This should avoid that GCC tries to reason about the pointer value. The GCC behavior seems reasonable overall, just the warning text is very off.
Nico
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
If somebody wants to try (there are many more places but this should affect the reported lapic case):
diff --git a/src/arch/x86/include/arch/mmio.h b/src/arch/x86/include/arch/mmio.h index 7188eac22a..72302125f6 100644 --- a/src/arch/x86/include/arch/mmio.h +++ b/src/arch/x86/include/arch/mmio.h @@ -15,7 +15,7 @@ static __always_inline uint16_t read16(const volatile void *addr) return *((volatile uint16_t *)(addr)); }
-static __always_inline uint32_t read32(const volatile void *addr) +static __always_inline uint32_t read32(const volatile void *volatile addr) { return *((volatile uint32_t *)(addr)); }
-
Nico Huber -
Paul Menzel -
Peter Stuge