| Message ID | 6a8bf78c-aedb-4d5a-b0aa-82a51a17b884@embeddedor.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [RFC,-,is,this,a,bug?] powerpc/lib/sstep: Asking for some light on this, please. :) | expand |
On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote: > Hi all, > > I'm trying to fix the following -Wstringop-overflow warnings, and I'd like > to get your feedback on this, please: > > In function 'do_byte_reverse', > inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3, > inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9: > arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] > 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 destination object 'u' of size 16 > 708 | } u; > | ^ > In function 'do_byte_reverse', > inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3, > inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9: > arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] > 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 destination object 'u' of size 16 > 708 | } u; > | ^ > In function 'do_byte_reverse', > inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3, > inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9: > arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] > 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 destination object 'u' of size 16 > 681 | } u = {}; > | ^ > arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16 > arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16 > > The origing of the issue seems to be the following calls to function `do_vec_store()`, when > `size > 16`, or more precisely when `size == 32` > > arch/powerpc/lib/sstep.c: > 3436 case LOAD_VMX: > 3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) > 3438 return 0; > 3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian); > 3440 break; > ... > 3507 case STORE_VMX: > 3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) > 3509 return 0; > 3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian); > 3511 break; LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not exceed 16. So, the warning looks incorrect to me. - Naveen
On 11/20/23 08:25, Naveen N Rao wrote: > On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote: >> Hi all, >> >> I'm trying to fix the following -Wstringop-overflow warnings, and I'd like >> to get your feedback on this, please: >> >> In function 'do_byte_reverse', >> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3, >> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9: >> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] >> 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 destination object 'u' of size 16 >> 708 | } u; >> | ^ >> In function 'do_byte_reverse', >> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3, >> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9: >> arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] >> 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 destination object 'u' of size 16 >> 708 | } u; >> | ^ >> In function 'do_byte_reverse', >> inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3, >> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9: >> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] >> 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 destination object 'u' of size 16 >> 681 | } u = {}; >> | ^ >> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16 >> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16 >> >> The origing of the issue seems to be the following calls to function `do_vec_store()`, when >> `size > 16`, or more precisely when `size == 32` >> >> arch/powerpc/lib/sstep.c: >> 3436 case LOAD_VMX: >> 3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) >> 3438 return 0; >> 3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian); >> 3440 break; >> ... >> 3507 case STORE_VMX: >> 3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) >> 3509 return 0; >> 3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian); >> 3511 break; > > LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not > exceed 16. So, the warning looks incorrect to me. Does that mean that this piece of code is never actually executed: 281 case 32: { 282 unsigned long *up = (unsigned long *)ptr; 283 unsigned long tmp; 284 285 tmp = byterev_8(up[0]); 286 up[0] = byterev_8(up[3]); 287 up[3] = tmp; 288 tmp = byterev_8(up[2]); 289 up[2] = byterev_8(up[1]); 290 up[1] = tmp; 291 break; 292 } or rather, under which conditions the above code is executed, if any? Thanks! -- Gustavo
On Mon, Nov 20, 2023 at 08:33:45AM -0600, Gustavo A. R. Silva wrote: > > > On 11/20/23 08:25, Naveen N Rao wrote: > > On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote: > > > Hi all, > > > > > > I'm trying to fix the following -Wstringop-overflow warnings, and I'd like > > > to get your feedback on this, please: > > > > > > In function 'do_byte_reverse', > > > inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3, > > > inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9: > > > arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] > > > 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 destination object 'u' of size 16 > > > 708 | } u; > > > | ^ > > > In function 'do_byte_reverse', > > > inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3, > > > inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9: > > > arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] > > > 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 destination object 'u' of size 16 > > > 708 | } u; > > > | ^ > > > In function 'do_byte_reverse', > > > inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3, > > > inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9: > > > arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] > > > 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 destination object 'u' of size 16 > > > 681 | } u = {}; > > > | ^ > > > arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16 > > > arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16 > > > > > > The origing of the issue seems to be the following calls to function `do_vec_store()`, when > > > `size > 16`, or more precisely when `size == 32` > > > > > > arch/powerpc/lib/sstep.c: > > > 3436 case LOAD_VMX: > > > 3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) > > > 3438 return 0; > > > 3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian); > > > 3440 break; > > > ... > > > 3507 case STORE_VMX: > > > 3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) > > > 3509 return 0; > > > 3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian); > > > 3511 break; > > > > LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not > > exceed 16. So, the warning looks incorrect to me. > > Does that mean that this piece of code is never actually executed: > > 281 case 32: { > 282 unsigned long *up = (unsigned long *)ptr; > 283 unsigned long tmp; > 284 > 285 tmp = byterev_8(up[0]); > 286 up[0] = byterev_8(up[3]); > 287 up[3] = tmp; > 288 tmp = byterev_8(up[2]); > 289 up[2] = byterev_8(up[1]); > 290 up[1] = tmp; > 291 break; > 292 } > > or rather, under which conditions the above code is executed, if any? Please see git history to understand the commit that introduced that code. You can do that by starting with a 'git blame' on the file. You'll find that the commit that introduced the above hunk was af99da74333b ("powerpc/sstep: Support VSX vector paired storage access instructions"). The above code path is hit via do_vsx_load()->emulate_vsx_load()->do_byte_reverse() - Naveen
On 11/20/23 09:21, Naveen N Rao wrote: > On Mon, Nov 20, 2023 at 08:33:45AM -0600, Gustavo A. R. Silva wrote: >> >> >> On 11/20/23 08:25, Naveen N Rao wrote: >>> On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote: >>>> Hi all, >>>> >>>> I'm trying to fix the following -Wstringop-overflow warnings, and I'd like >>>> to get your feedback on this, please: >>>> >>>> In function 'do_byte_reverse', >>>> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3, >>>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9: >>>> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] >>>> 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 destination object 'u' of size 16 >>>> 708 | } u; >>>> | ^ >>>> In function 'do_byte_reverse', >>>> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3, >>>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9: >>>> arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] >>>> 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 destination object 'u' of size 16 >>>> 708 | } u; >>>> | ^ >>>> In function 'do_byte_reverse', >>>> inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3, >>>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9: >>>> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] >>>> 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 destination object 'u' of size 16 >>>> 681 | } u = {}; >>>> | ^ >>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16 >>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16 >>>> >>>> The origing of the issue seems to be the following calls to function `do_vec_store()`, when >>>> `size > 16`, or more precisely when `size == 32` >>>> >>>> arch/powerpc/lib/sstep.c: >>>> 3436 case LOAD_VMX: >>>> 3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) >>>> 3438 return 0; >>>> 3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian); >>>> 3440 break; >>>> ... >>>> 3507 case STORE_VMX: >>>> 3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) >>>> 3509 return 0; >>>> 3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian); >>>> 3511 break; >>> >>> LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not >>> exceed 16. So, the warning looks incorrect to me. >> >> Does that mean that this piece of code is never actually executed: >> >> 281 case 32: { >> 282 unsigned long *up = (unsigned long *)ptr; >> 283 unsigned long tmp; >> 284 >> 285 tmp = byterev_8(up[0]); >> 286 up[0] = byterev_8(up[3]); >> 287 up[3] = tmp; >> 288 tmp = byterev_8(up[2]); >> 289 up[2] = byterev_8(up[1]); >> 290 up[1] = tmp; >> 291 break; >> 292 } >> >> or rather, under which conditions the above code is executed, if any? > > Please see git history to understand the commit that introduced that > code. You can do that by starting with a 'git blame' on the file. You'll > find that the commit that introduced the above hunk was af99da74333b > ("powerpc/sstep: Support VSX vector paired storage access > instructions"). > > The above code path is hit via > do_vsx_load()->emulate_vsx_load()->do_byte_reverse() It seems there is another path (at least the one that triggers the -Wstringop-overflow warnings): emulate_step()->emulate_loadstore()->do_vec_load()->do_byte_reverse() emulate_step() { ... if (OP_IS_LOAD_STORE(type)) { err = emulate_loadstore(regs, &op); if (err) return 0; goto instr_done; } ... } ----> emulate_loadstore() { ... #ifdef CONFIG_ALTIVEC case LOAD_VMX: if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) return 0; err = do_vec_load(op->reg, ea, size, regs, cross_endian); // with size == 32 break; #endif ... #ifdef CONFIG_ALTIVEC case STORE_VMX: if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) return 0; err = do_vec_store(op->reg, ea, size, regs, cross_endian); // with size == 32 break; #endif ... } ----> do_vec_load() // Here is where we have the union of size 16 bytes: { ... union { __vector128 v; u8 b[sizeof(__vector128)]; } u = {}; ... if (unlikely(cross_endian)) do_byte_reverse(&u.b[ea & 0xf], size); ... } ----> do_byte_reverse() { ... case 32: { unsigned long *up = (unsigned long *)ptr; // this is a pointer to u.b unsigned long tmp; tmp = byterev_8(up[0]); up[0] = byterev_8(up[3]); up[3] = tmp; // and here... tmp = byterev_8(up[2]); up[2] = byterev_8(up[1]); // and here we are accessing ptr out-of-bounds up[1] = tmp; break; } ... } This is where I'm trying to determine if there is a bug. If this path is actually executed, then it seems we have an issue. Is it possible that commit af99da74333b overlooked the path described above? -- Gustavo
"Gustavo A. R. Silva" <gustavo@embeddedor.com> writes: > On 11/20/23 09:21, Naveen N Rao wrote: >> On Mon, Nov 20, 2023 at 08:33:45AM -0600, Gustavo A. R. Silva wrote: >>> On 11/20/23 08:25, Naveen N Rao wrote: >>>> On Fri, Nov 17, 2023 at 12:36:01PM -0600, Gustavo A. R. Silva wrote: >>>>> Hi all, >>>>> >>>>> I'm trying to fix the following -Wstringop-overflow warnings, and I'd like >>>>> to get your feedback on this, please: >>>>> >>>>> In function 'do_byte_reverse', >>>>> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3, >>>>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9: >>>>> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] >>>>> 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 destination object 'u' of size 16 >>>>> 708 | } u; >>>>> | ^ >>>>> In function 'do_byte_reverse', >>>>> inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3, >>>>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9: >>>>> arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] >>>>> 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 destination object 'u' of size 16 >>>>> 708 | } u; >>>>> | ^ >>>>> In function 'do_byte_reverse', >>>>> inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3, >>>>> inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9: >>>>> arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] >>>>> 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 destination object 'u' of size 16 >>>>> 681 | } u = {}; >>>>> | ^ >>>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16 >>>>> arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16 >>>>> >>>>> The origing of the issue seems to be the following calls to function `do_vec_store()`, when >>>>> `size > 16`, or more precisely when `size == 32` >>>>> >>>>> arch/powerpc/lib/sstep.c: >>>>> 3436 case LOAD_VMX: >>>>> 3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) >>>>> 3438 return 0; >>>>> 3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian); >>>>> 3440 break; >>>>> ... >>>>> 3507 case STORE_VMX: >>>>> 3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) >>>>> 3509 return 0; >>>>> 3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian); >>>>> 3511 break; >>>> >>>> LOAD_VMX and STORE_VMX are set in analyse_instr() and size does not >>>> exceed 16. So, the warning looks incorrect to me. >>> >>> Does that mean that this piece of code is never actually executed: >>> >>> 281 case 32: { >>> 282 unsigned long *up = (unsigned long *)ptr; >>> 283 unsigned long tmp; >>> 284 >>> 285 tmp = byterev_8(up[0]); >>> 286 up[0] = byterev_8(up[3]); >>> 287 up[3] = tmp; >>> 288 tmp = byterev_8(up[2]); >>> 289 up[2] = byterev_8(up[1]); >>> 290 up[1] = tmp; >>> 291 break; >>> 292 } >>> >>> or rather, under which conditions the above code is executed, if any? >> >> Please see git history to understand the commit that introduced that >> code. You can do that by starting with a 'git blame' on the file. You'll >> find that the commit that introduced the above hunk was af99da74333b >> ("powerpc/sstep: Support VSX vector paired storage access >> instructions"). >> >> The above code path is hit via >> do_vsx_load()->emulate_vsx_load()->do_byte_reverse() > > It seems there is another path (at least the one that triggers the > -Wstringop-overflow warnings): > > emulate_step()->emulate_loadstore()->do_vec_load()->do_byte_reverse() > > emulate_step() { > ... > if (OP_IS_LOAD_STORE(type)) { The type comes from the op, which is determined in analyse_instr() > err = emulate_loadstore(regs, &op); > if (err) > return 0; > goto instr_done; > } > ... > } > > ----> emulate_loadstore() { > ... > #ifdef CONFIG_ALTIVEC > case LOAD_VMX: > if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) > return 0; > err = do_vec_load(op->reg, ea, size, regs, cross_endian); // with size == 32 > break; > #endif > ... > #ifdef CONFIG_ALTIVEC > case STORE_VMX: > if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) > return 0; > err = do_vec_store(op->reg, ea, size, regs, cross_endian); // with size == 32 > break; > #endif > ... > } If you look at analyse_instr() the cases that produce an op with type == LOAD_VMX/STORE_VMX never use a size of 32: $ git grep -E "MKOP\((LOAD|STORE)_VMX" arch/powerpc/ arch/powerpc/lib/sstep.c: op->type = MKOP(LOAD_VMX, 0, 1); arch/powerpc/lib/sstep.c: op->type = MKOP(LOAD_VMX, 0, 2); arch/powerpc/lib/sstep.c: op->type = MKOP(LOAD_VMX, 0, 4); arch/powerpc/lib/sstep.c: op->type = MKOP(LOAD_VMX, 0, 16); arch/powerpc/lib/sstep.c: op->type = MKOP(STORE_VMX, 0, 1); arch/powerpc/lib/sstep.c: op->type = MKOP(STORE_VMX, 0, 2); arch/powerpc/lib/sstep.c: op->type = MKOP(STORE_VMX, 0, 4); arch/powerpc/lib/sstep.c: op->type = MKOP(STORE_VMX, 0, 16); So I don't think there's an actual bug. But the code is very hard to follow and it would be very easy for a bug to be introduced. Déjà vu intensifies... ... and apparently I have a patch for this that I never sent out. Sorry! :} I'll post it and see if the build bots are happy. cheers
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c index a4ab8625061a..86786957b736 100644 --- a/arch/powerpc/lib/sstep.c +++ b/arch/powerpc/lib/sstep.c @@ -3436,7 +3436,7 @@ int emulate_loadstore(struct pt_regs *regs, struct instruction_op *op) case LOAD_VMX: if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) return 0; - err = do_vec_load(op->reg, ea, size, regs, cross_endian); + err = do_vec_load(op->reg, ea, (size > 16) ? 16 : size, regs, cross_endian); break; #endif #ifdef CONFIG_VSX @@ -3507,7 +3507,7 @@ int emulate_loadstore(struct pt_regs *regs, struct instruction_op *op) case STORE_VMX: if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) return 0; - err = do_vec_store(op->reg, ea, size, regs, cross_endian); + err = do_vec_store(op->reg, ea, (size > 16) ? 16 : size, regs, cross_endian); break; #endif #ifdef CONFIG_VSX
Hi all, I'm trying to fix the following -Wstringop-overflow warnings, and I'd like to get your feedback on this, please: In function 'do_byte_reverse', inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3, inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9: arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] 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 destination object 'u' of size 16 708 | } u; | ^ In function 'do_byte_reverse', inlined from 'do_vec_store' at /home/gus/linux/arch/powerpc/lib/sstep.c:722:3, inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3510:9: arch/powerpc/lib/sstep.c:289:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] 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 destination object 'u' of size 16 708 | } u; | ^ In function 'do_byte_reverse', inlined from 'do_vec_load' at /home/gus/linux/arch/powerpc/lib/sstep.c:691:3, inlined from 'emulate_loadstore' at /home/gus/linux/arch/powerpc/lib/sstep.c:3439:9: arch/powerpc/lib/sstep.c:287:23: error: writing 8 bytes into a region of size 0 [-Werror=stringop-overflow=] 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 destination object 'u' of size 16 681 | } u = {}; | ^ arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16 arch/powerpc/lib/sstep.c:681:11: note: at offset [24, 39] into destination object 'u' of size 16 The origing of the issue seems to be the following calls to function `do_vec_store()`, when `size > 16`, or more precisely when `size == 32` arch/powerpc/lib/sstep.c: 3436 case LOAD_VMX: 3437 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) 3438 return 0; 3439 err = do_vec_load(op->reg, ea, size, regs, cross_endian); 3440 break; ... 3507 case STORE_VMX: 3508 if (!(regs->msr & MSR_PR) && !(regs->msr & MSR_VEC)) 3509 return 0; 3510 err = do_vec_store(op->reg, ea, size, regs, cross_endian); 3511 break; Inside `do_vec_store()`, we have that the size of `union u` is 16 bytes: 702 int size, struct pt_regs *regs, 703 bool cross_endian) 704 { 705 union { 706 __vector128 v; 707 u8 b[sizeof(__vector128)]; 708 } u; and then function `do_byte_reverse()` is called 721 if (unlikely(cross_endian)) 722 do_byte_reverse(&u.b[ea & 0xf], size); and if `size == 32`, the following piece of code tries to access `ptr` outside of its boundaries: 260 static nokprobe_inline void do_byte_reverse(void *ptr, int nb) 261 { 262 switch (nb) { ... 281 case 32: { 282 unsigned long *up = (unsigned long *)ptr; 283 unsigned long tmp; 284 285 tmp = byterev_8(up[0]); 286 up[0] = byterev_8(up[3]); 287 up[3] = tmp; 288 tmp = byterev_8(up[2]); 289 up[2] = byterev_8(up[1]); 290 up[1] = tmp; 291 break; 292 } The following patch is merely a test to illustrate how the value in `size` initially appears to influence the warnings: Is there something that I may be overlooking here? :) Thanks! -- Gustavo