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)
>(0xfee0 /* LAPIC_DEFAULT_BASE */ + xapic_read_reg);
>
>uint32_t lapicid_local_lapicid =
>*((volatile uint32_t *)(read32_param_addr));
>
> (Where I named function parameters `x` `_param_` and local
> variables `y` _local_`.)
>
> 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));
}
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org
