Re: [RFC - is this a bug?] powerpc/lib/sstep: Asking for some light on this, please. :)

[Michael Ellerman <mpe@ellerman.id.au>]

"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