Re: outside array bounds error on ppc64_defconfig, GCC 12.1.0
Bagas Sanjaya writes: > Hi, > > I'm trying to verify Drop ppc_inst_as_str() patch on [1] by performing > ppc64_defconfig build with powerpc64-unknown-linux-gnu-gcc (GCC 12.1.0). > The patch is applied on top of powerpc tree, next branch. Yeah I see it too. > I got outside array bounds error: > > CC arch/powerpc/kernel/dbell.o > In function 'do_byte_reverse', > inlined from 'do_vec_store' at arch/powerpc/lib/sstep.c:722:3, > inlined from 'emulate_loadstore' at arch/powerpc/lib/sstep.c:3509:9: > arch/powerpc/lib/sstep.c:286:25: error: array subscript [3, 4] is outside > array bounds of 'union [1]' [-Werror=array-bounds] > 286 | up[0] = byterev_8(up[3]); > | ^~~~ > > arch/owerpc/lib/sstep.c: In function 'emulate_loadstore': > arch/powerpc/lib/sstep.c:708:11: note: at offset [24, 39] into object 'u' of > size 16 > 708 | } u; > | ^ > In function 'do_byte_reverse', > inlined from 'do_vec_store' at arch/powerpc/lib/sstep.c:722:3, > inlined from 'emulate_loadstore' at arch/powerpc/lib/sstep.c:3509:9: > arch/powerpc/lib/sstep.c:287:23: error: array subscript [3, 4] is outside > array bounds of 'union [1]' [-Werror=array-bounds] > 287 | up[3] = tmp; > | ~~^ This happens because we have a generic byte reverse function (do_byte_reverse()), that takes a size as a parameter. So it will reverse 8, 16, 32 bytes etc. In some cases the compiler can see that we're passing a pointer to storage that is smaller than 32 bytes, but it isn't convinced that the size parameter is also smaller than 32 bytes. Which I think is reasonable, the code that sets the size is separate from this code, so the compiler can't really deduce that it's safe. I don't see a really simple fix. I tried clamping the size parameter to do_byte_reverse() with max(), but that didn't work :/ cheers
RE: outside array bounds error on ppc64_defconfig, GCC 12.1.0
From: Michael Ellerman > Sent: 07 June 2022 03:05 > > Bagas Sanjaya writes: > > Hi, > > > > I'm trying to verify Drop ppc_inst_as_str() patch on [1] by performing > > ppc64_defconfig build with powerpc64-unknown-linux-gnu-gcc (GCC 12.1.0). > > The patch is applied on top of powerpc tree, next branch. > > Yeah I see it too. > > > I got outside array bounds error: > > > > CC arch/powerpc/kernel/dbell.o > > In function 'do_byte_reverse', > > inlined from 'do_vec_store' at arch/powerpc/lib/sstep.c:722:3, > > inlined from 'emulate_loadstore' at arch/powerpc/lib/sstep.c:3509:9: > > arch/powerpc/lib/sstep.c:286:25: error: array subscript [3, 4] is outside > > array bounds of 'union > [1]' [-Werror=array-bounds] > > 286 | up[0] = byterev_8(up[3]); > > | ^~~~ > > > > arch/owerpc/lib/sstep.c: In function 'emulate_loadstore': > > arch/powerpc/lib/sstep.c:708:11: note: at offset [24, 39] into object 'u' > > of size 16 > > 708 | } u; > > | ^ > > In function 'do_byte_reverse', > > inlined from 'do_vec_store' at arch/powerpc/lib/sstep.c:722:3, > > inlined from 'emulate_loadstore' at arch/powerpc/lib/sstep.c:3509:9: > > arch/powerpc/lib/sstep.c:287:23: error: array subscript [3, 4] is outside > > array bounds of 'union > [1]' [-Werror=array-bounds] > > 287 | up[3] = tmp; > > | ~~^ > > This happens because we have a generic byte reverse function > (do_byte_reverse()), that takes a size as a parameter. So it will > reverse 8, 16, 32 bytes etc. > > In some cases the compiler can see that we're passing a pointer to > storage that is smaller than 32 bytes, but it isn't convinced that the > size parameter is also smaller than 32 bytes. > > Which I think is reasonable, the code that sets the size is separate > from this code, so the compiler can't really deduce that it's safe. > > I don't see a really simple fix. I tried clamping the size parameter to > do_byte_reverse() with max(), but that didn't work :/ I had a quick look at the code - it is somewhat horrid! Not really surprising the compiler is confused. Although it shouldn't be outputting that error message unless it is certain. Could it be re-written to read the data into an __u128 (or whatever the compiler type is). Optionally byteswap the entire thing (swap the words and then byteswap each word). The do a put_user_8/16/32/64() to write out the value. I think that would remove all the memory accesses and make it a lot faster as well. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: outside array bounds error on ppc64_defconfig, GCC 12.1.0
On Tue, Jun 07, 2022 at 12:05:18PM +1000, Michael Ellerman wrote: > > arch/powerpc/lib/sstep.c:287:23: error: array subscript [3, 4] is outside > > array bounds of 'union [1]' [-Werror=array-bounds] > > 287 | up[3] = tmp; > > | ~~^ > > This happens because we have a generic byte reverse function > (do_byte_reverse()), that takes a size as a parameter. So it will > reverse 8, 16, 32 bytes etc. > > In some cases the compiler can see that we're passing a pointer to > storage that is smaller than 32 bytes, but it isn't convinced that the > size parameter is also smaller than 32 bytes. > > Which I think is reasonable, the code that sets the size is separate > from this code, so the compiler can't really deduce that it's safe. > > I don't see a really simple fix. I tried clamping the size parameter to > do_byte_reverse() with max(), but that didn't work :/ -Wno-error or at least -Wno-error=array-bounds is a good, simple fix. Segher
Re: outside array bounds error on ppc64_defconfig, GCC 12.1.0
On Tue, Jun 07, 2022 at 02:23:25PM +, David Laight wrote: > > I don't see a really simple fix. I tried clamping the size parameter to > > do_byte_reverse() with max(), but that didn't work :/ > > I had a quick look at the code - it is somewhat horrid! > Not really surprising the compiler is confused. > Although it shouldn't be outputting that error message > unless it is certain. No. It is a warning message, and the compiler should output it for all code it finds suspicious. The conditions for this could be improved for sure, but it is and will remain a heuristic affair, so using -Werror with is is akin to self-flagellation. It is not an error, it is a warning. The correct response to it when you determine it is not an error, or even you do not want to deal with it now, is to ignore it. Which -Werror prevents, one of the ways that that warning flag is counterproductive to use. > Could it be re-written to read the data into an __u128 > (or whatever the compiler type is). > Optionally byteswap the entire thing (swap the words and > then byteswap each word). > The do a put_user_8/16/32/64() to write out the value. > > I think that would remove all the memory accesses and make > it a lot faster as well. Yes, the warning sometimes fires for correct code that is "merely" next to impossible to read. It may well improve even the performance of the code if the code is rewritten. But it also may introduce new bugs, or anything else detrimental. It is yakshaving extraordinaire to do this every time a compiler warning points out something doesn't smell quite right :-) Segher
