| Message ID | 20230817204506.34827-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rombios: Work around GCC issue 99578 | expand |
On 17.08.2023 22:45, Andrew Cooper wrote: > GCC 12 objects to pointers derived from a constant: > > util.c: In function 'find_rsdp': > util.c:429:16: error: array subscript 0 is outside array bounds of 'uint16_t[0]' {aka 'short unsigned int[]'} [-Werror=array-bounds] > 429 | ebda_seg = *(uint16_t *)ADDR_FROM_SEG_OFF(0x40, 0xe); > cc1: all warnings being treated as errors > Yet supposedly the bug was fixed in 12.1 (and the fix also backported to 11.3). Did you spot anything in ADDR_FROM_SEG_OFF() and this particular use of it that is different from the patterns mentioned in that bug? > This is a GCC bug, but work around it rather than turning array-bounds > checking off generally. I certainly agree here. I guess it's not worth trying to restrict the workaround for rombios (I will want to try doing so in the hypervisor). Jan
On 18/08/2023 7:50 am, Jan Beulich wrote: > On 17.08.2023 22:45, Andrew Cooper wrote: >> GCC 12 objects to pointers derived from a constant: >> >> util.c: In function 'find_rsdp': >> util.c:429:16: error: array subscript 0 is outside array bounds of 'uint16_t[0]' {aka 'short unsigned int[]'} [-Werror=array-bounds] >> 429 | ebda_seg = *(uint16_t *)ADDR_FROM_SEG_OFF(0x40, 0xe); >> cc1: all warnings being treated as errors >> > Yet supposedly the bug was fixed in 12.1 (and the fix also backported to > 11.3). Did you spot anything in ADDR_FROM_SEG_OFF() and this particular > use of it that is different from the patterns mentioned in that bug? $ gcc --version gcc (GCC) 12.2.1 20221121 At a guess, only a partial fix was backported into 12.1. AIUI, it was an area of logic which had already seen significant development in 13 before the behaviour was reverted. The only thing interesting about ADDR_FROM_SEG_OFF() is the constant folding required for the expression to become *(uint16_t *)0x40e, which (I suspect) is why it compiles fine at -Og but fails at -O2. >> This is a GCC bug, but work around it rather than turning array-bounds >> checking off generally. > I certainly agree here. I guess it's not worth trying to restrict the > workaround for rombios (I will want to try doing so in the hypervisor). Can I translate this to an ack? ~Andrew
On 18.08.2023 11:44, Andrew Cooper wrote: > On 18/08/2023 7:50 am, Jan Beulich wrote: >> On 17.08.2023 22:45, Andrew Cooper wrote: >>> GCC 12 objects to pointers derived from a constant: >>> >>> util.c: In function 'find_rsdp': >>> util.c:429:16: error: array subscript 0 is outside array bounds of 'uint16_t[0]' {aka 'short unsigned int[]'} [-Werror=array-bounds] >>> 429 | ebda_seg = *(uint16_t *)ADDR_FROM_SEG_OFF(0x40, 0xe); >>> cc1: all warnings being treated as errors >>> >> Yet supposedly the bug was fixed in 12.1 (and the fix also backported to >> 11.3). Did you spot anything in ADDR_FROM_SEG_OFF() and this particular >> use of it that is different from the patterns mentioned in that bug? > > $ gcc --version > gcc (GCC) 12.2.1 20221121 > > At a guess, only a partial fix was backported into 12.1. AIUI, it was > an area of logic which had already seen significant development in 13 > before the behaviour was reverted. Hmm, for 12 I didn't think there was any backporting involved. > The only thing interesting about ADDR_FROM_SEG_OFF() is the constant > folding required for the expression to become *(uint16_t *)0x40e, which > (I suspect) is why it compiles fine at -Og but fails at -O2. Oh, the relevant aspect may be that is is below 4k (which I think is their heuristic offset to guess "almost NULL" dereferencing). >>> This is a GCC bug, but work around it rather than turning array-bounds >>> checking off generally. >> I certainly agree here. I guess it's not worth trying to restrict the >> workaround for rombios (I will want to try doing so in the hypervisor). > > Can I translate this to an ack? You can now: Acked-by: Jan Beulich <jbeulich@suse.com> I wanted to at least have a vague idea of why this fails with gcc12, when I thought it shouldn't. In light of the 4k aspect maybe the bug reference wants adjusting / dropping. Jan
On 18/08/2023 11:09 am, Jan Beulich wrote: > On 18.08.2023 11:44, Andrew Cooper wrote: >> On 18/08/2023 7:50 am, Jan Beulich wrote: >>> On 17.08.2023 22:45, Andrew Cooper wrote: >>>> GCC 12 objects to pointers derived from a constant: >>>> >>>> util.c: In function 'find_rsdp': >>>> util.c:429:16: error: array subscript 0 is outside array bounds of 'uint16_t[0]' {aka 'short unsigned int[]'} [-Werror=array-bounds] >>>> 429 | ebda_seg = *(uint16_t *)ADDR_FROM_SEG_OFF(0x40, 0xe); >>>> cc1: all warnings being treated as errors >>>> >>> Yet supposedly the bug was fixed in 12.1 (and the fix also backported to >>> 11.3). Did you spot anything in ADDR_FROM_SEG_OFF() and this particular >>> use of it that is different from the patterns mentioned in that bug? >> $ gcc --version >> gcc (GCC) 12.2.1 20221121 >> >> At a guess, only a partial fix was backported into 12.1. AIUI, it was >> an area of logic which had already seen significant development in 13 >> before the behaviour was reverted. > Hmm, for 12 I didn't think there was any backporting involved. > >> The only thing interesting about ADDR_FROM_SEG_OFF() is the constant >> folding required for the expression to become *(uint16_t *)0x40e, which >> (I suspect) is why it compiles fine at -Og but fails at -O2. > Oh, the relevant aspect may be that is is below 4k (which I think is > their heuristic offset to guess "almost NULL" dereferencing). Ah yes, I remember that aspect now you've pointed it out. Yeah, that's probably it. >>>> This is a GCC bug, but work around it rather than turning array-bounds >>>> checking off generally. >>> I certainly agree here. I guess it's not worth trying to restrict the >>> workaround for rombios (I will want to try doing so in the hypervisor). >> Can I translate this to an ack? > You can now: > Acked-by: Jan Beulich <jbeulich@suse.com> Thanks.
diff --git a/tools/firmware/rombios/32bit/util.c b/tools/firmware/rombios/32bit/util.c index 6c1c4805144b..a47e000a2629 100644 --- a/tools/firmware/rombios/32bit/util.c +++ b/tools/firmware/rombios/32bit/util.c @@ -424,10 +424,10 @@ static struct acpi_20_rsdp *__find_rsdp(const void *start, unsigned int len) struct acpi_20_rsdp *find_rsdp(void) { struct acpi_20_rsdp *rsdp; - uint16_t ebda_seg; + uint16_t *volatile /* GCC issue 99578 */ ebda_seg = + ADDR_FROM_SEG_OFF(0x40, 0xe); - ebda_seg = *(uint16_t *)ADDR_FROM_SEG_OFF(0x40, 0xe); - rsdp = __find_rsdp((void *)(ebda_seg << 16), 1024); + rsdp = __find_rsdp((void *)(*ebda_seg << 16), 1024); if (!rsdp) rsdp = __find_rsdp((void *)0xE0000, 0x20000);
GCC 12 objects to pointers derived from a constant: util.c: In function 'find_rsdp': util.c:429:16: error: array subscript 0 is outside array bounds of 'uint16_t[0]' {aka 'short unsigned int[]'} [-Werror=array-bounds] 429 | ebda_seg = *(uint16_t *)ADDR_FROM_SEG_OFF(0x40, 0xe); cc1: all warnings being treated as errors This is a GCC bug, but work around it rather than turning array-bounds checking off generally. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> This only manifests in release builds, presumably a side effect of neediung some constant-folding to notice the ADDR_FROM_SEG_OFF() expression. We don't see this in CI because debug isn't configured correctly for the tools/ part of the build --- tools/firmware/rombios/32bit/util.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) base-commit: d0eabe3eaf0db5b78843095a2918d50961e99e96