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.
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 <anonymous>[1]' [-Werror=array-bounds]
286 | up[0] = byterev_8(up[3]);
| ^~~~~~~~~~~~~~~~
arch/powerpc/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 <anonymous>[1]' [-Werror=array-bounds]
287 | up[3] = tmp;
| ~~~~~~^~~~~
arch/powerpc/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:288:23: error: array subscript 2 is outside array bounds of 'union <anonymous>[1]' [-Werror=array-bounds]
288 | tmp = byterev_8(up[2]);
| ^~~~~~~~~~~~~~~~
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:708:11: note: at offset 16 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:289:23: error: array subscript 2 is outside array bounds of 'union <anonymous>[1]' [-Werror=array-bounds]
289 | up[2] = byterev_8(up[1]);
| ~~~~~~^~~~~~~~~~~~~~~~~~
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:708:11: note: at offset 16 into object 'u' of size 16
708 | } u;
| ^
In function 'do_byte_reverse',
inlined from 'do_vec_load' at arch/powerpc/lib/sstep.c:691:3,
inlined from 'emulate_loadstore' at arch/powerpc/lib/sstep.c:3438:9:
arch/powerpc/lib/sstep.c:286:25: error: array subscript [3, 4] is outside array bounds of 'u8[16]' {aka 'unsigned char[16]'} [-Werror=array-bounds]
286 | up[0] = byterev_8(up[3]);
| ^~~~~~~~~~~~~~~~
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into object 'u' of size 16
681 | } u = {};
| ^
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into object 'u' of size 16
In function 'do_byte_reverse',
inlined from 'do_vec_load' at arch/powerpc/lib/sstep.c:691:3,
inlined from 'emulate_loadstore' at arch/powerpc/lib/sstep.c:3438:9:
arch/powerpc/lib/sstep.c:287:23: error: array subscript [3, 4] is outside array bounds of 'u8[16]' {aka 'unsigned char[16]'} [-Werror=array-bounds]
287 | up[3] = tmp;
| ~~~~~~^~~~~
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into object 'u' of size 16
681 | } u = {};
| ^
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into object 'u' of size 16
In function 'do_byte_reverse',
inlined from 'do_vec_load' at arch/powerpc/lib/sstep.c:691:3,
inlined from 'emulate_loadstore' at arch/powerpc/lib/sstep.c:3438:9:
arch/powerpc/lib/sstep.c:288:23: error: array subscript 2 is outside array bounds of 'u8[16]' {aka 'unsigned char[16]'} [-Werror=array-bounds]
288 | tmp = byterev_8(up[2]);
| ^~~~~~~~~~~~~~~~
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:681:11: note: at offset 16 into object 'u' of size 16
681 | } u = {};
| ^
arch/powerpc/lib/sstep.c:681:11: note: at offset 16 into object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset 16 into object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset 16 into object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset 16 into object 'u' of size 16
In function 'do_byte_reverse',
inlined from 'do_vec_load' at arch/powerpc/lib/sstep.c:691:3,
inlined from 'emulate_loadstore' at arch/powerpc/lib/sstep.c:3438:9:
arch/powerpc/lib/sstep.c:289:23: error: array subscript 2 is outside array bounds of 'u8[16]' {aka 'unsigned char[16]'} [-Werror=array-bounds]
289 | up[2] = byterev_8(up[1]);
| ~~~~~~^~~~~~~~~~~~~~~~~~
arch/powerpc/lib/sstep.c: In function 'emulate_loadstore':
arch/powerpc/lib/sstep.c:681:11: note: at offset 16 into object 'u' of size 16
681 | } u = {};
| ^
arch/powerpc/lib/sstep.c:681:11: note: at offset 16 into object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset 16 into object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset 16 into object 'u' of size 16
arch/powerpc/lib/sstep.c:681:11: note: at offset 16 into object 'u' of size 16
CC arch/powerpc/kernel/jump_label.o
cc1: all warnings being treated as errors
make[2]: *** [scripts/Makefile.build:271: arch/powerpc/lib/sstep.o] Error 1
These errors above are unrelated to the patch.
[1]: https://lore.kernel.org/linuxppc-dev/[email protected]/
Thanks.
--
An old man doll... just what I always wanted! - Clara
On Tue, Jun 07, 2022 at 02:23:25PM +0000, 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
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 <anonymous>[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
Bagas Sanjaya <[email protected]> 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 <anonymous>[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 <anonymous>[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
From: Michael Ellerman
> Sent: 07 June 2022 03:05
>
> Bagas Sanjaya <[email protected]> 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
> <anonymous>[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
> <anonymous>[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)