"Gustavo A. R. Silva" <gust...@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