Re: [PATCH] [sim] Fix build failure in d10v sim

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

On 5/10/21 7:30 PM, Mike Frysinger wrote:
> On 10 May 2021 16:14, Luis Machado via Gdb-patches wrote:
>> While building all targets on Ubuntu 20.04/aarch64, I ran into the following
>> build error:
>>
>> In file included from /usr/include/string.h:495,
>>                   from ../../bfd/bfd.h:48,
>>                   from ../../../../repos/binutils-gdb/sim/d10v/interp.c:4:
>> In function 'memset',
>>      inlined from 'sim_create_inferior' at ../../../../repos/binutils-gdb/sim/d10v/interp.c:1146:3:
>> /usr/include/aarch64-linux-gnu/bits/string_fortified.h:71:10: error: ‘__builtin_memset’ offset [33, 616] from the object at ‘State’ is out of the bounds of referenced subobject ‘regs’ with type ‘reg_t[16]’ {aka ‘short unsigned int[16]’} at offset 0 [-Werror=array-bounds]
>>     71 |   return __builtin___memset_chk (__dest, __ch, __len, __bos0 (__dest));
>>        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> cc1: all warnings being treated as errors
>> make[3]: *** [Makefile:558: interp.o] Error 1
>>
>> I looked at a different sim (cr16), and it zeroes out the entire State, not
>> just the registers.
> 
> it's clearing more than just the regs.  it's clearing all the members in the
> state struct from regs up to mem.  so regs, cregs, sp, a, slot, etc...  if
> you want to change it, the comment in the header probably needs adjusting.

Heh... the comment in the header says something and the one before 
memset says another that is completely different.

> 
>> It is unclear why we have the casts to uintptr.
> 
> because pointer math on diff types isn't allowed, and the 3rd arg to memset
> needs to be a scalar integer.  so casting to uintptr_t is the correct way to
> calculate this value.

Right. What I mean is I don't understand why the complexity to clear 
some data with memset while the comment is stating we should clear 
everything. Then again, it looks like it was copy/pasted from somewhere 
else and not documented properly in the .c file.

The same seems to be happening with cr16. The header says we reset 
things until a certain member. But the .c code says we reset everything.

> 
>> The following patch fixes this for me.
>>
>> -  memset (&State.regs, 0, (uintptr_t)&State.mem - (uintptr_t)&State.regs);
>> +  memset (&State, 0, sizeof (State));
> 
> i think changing the first arg to &State is sufficient and keeping the mem-regs
> calculation.  and maybe add a static assert that &State == &State.regs ?

Ok. I'll send a v2. Thanks for the input. The comments were throwing me off.

> 
> this code needs to be rewritten fundamentally at some point :/.

Yeah. It needs some TLC.

> -mike
>