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

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

[Luis Machado]

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 is unclear why we have the casts to uintptr.

The following patch fixes this for me.

OK?

sim/ChangeLog:

YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

	d10v/interp.c (sim_create_inferior): Fix memset call.
---
 sim/d10v/interp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sim/d10v/interp.c b/sim/d10v/interp.c
index 86e566a79fb..a59779810eb 100644
--- a/sim/d10v/interp.c
+++ b/sim/d10v/interp.c
@@ -1143,7 +1143,7 @@ sim_create_inferior (SIM_DESC sd, struct bfd *abfd,
   bfd_vma start_address;
 
   /* reset all state information */
-  memset (&State.regs, 0, (uintptr_t)&State.mem - (uintptr_t)&State.regs);
+  memset (&State, 0, sizeof (State));
 
   /* There was a hack here to copy the values of argc and argv into r0
      and r1.  The values were also saved into some high memory that
-- 
2.25.1

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

[Mike Frysinger]

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.

> 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.

> 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 ?

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

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

[Luis Machado]

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
> 

[PATCH,v2] [sim] Fix build failure in d10v sim

[Luis Machado]

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

The following patch fixes this for me.

OK?

sim/ChangeLog:

YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

	d10v/interp.c (sim_create_inferior): Fix memset call.
---
 sim/d10v/interp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sim/d10v/interp.c b/sim/d10v/interp.c
index 86e566a79fb..64dd392381c 100644
--- a/sim/d10v/interp.c
+++ b/sim/d10v/interp.c
@@ -1142,8 +1142,12 @@ sim_create_inferior (SIM_DESC sd, struct bfd *abfd,
 {
   bfd_vma start_address;
 
-  /* reset all state information */
-  memset (&State.regs, 0, (uintptr_t)&State.mem - (uintptr_t)&State.regs);
+  /* Make sure we have the right structure for the following memset.  */
+  _Static_assert ((uintptr_t) &State == (uintptr_t) &State.regs,
+		  "&State != &State.regs");
+
+  /* Reset state from the regs field until the mem field.  */
+  memset (&State, 0, (uintptr_t)&State.mem - (uintptr_t)&State.regs);
 
   /* There was a hack here to copy the values of argc and argv into r0
      and r1.  The values were also saved into some high memory that
-- 
2.25.1

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

[Mike Frysinger]

On 10 May 2021 23:01, Luis Machado via Gdb-patches wrote:
> 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.

i don't think the comment/code is at odds.  it's just not as precise as it
should be.  it's clearing all the cpu state, but leaving the system (memory)
state alone.

if you look through the git history on these files, it was a bit more explicit:
it would save the fields it cared about, memset the whole thing, then restore
the fields.  at some point that was all deleted and replaced with this (clever?)
memset call.

copy pasta between some of these older sim ports is quite bad/heavy.  it's slow
going converting it over, especially for ports that don't seem to have anyone
who cares about them anymore.  some of these arches i know exist only because
there's a sim port for them ... never heard of them otherwise.
-mike

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

[Mike Frysinger]

On 11 May 2021 10:07, Luis Machado wrote:
> +  /* Make sure we have the right structure for the following memset.  */
> +  _Static_assert ((uintptr_t) &State == (uintptr_t) &State.regs,
> +		  "&State != &State.regs");

we require C11, so you can include assert.h & use static_assert().
-mike

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

[Luis Machado]

On 5/11/21 6:54 PM, Mike Frysinger wrote:
> On 11 May 2021 10:07, Luis Machado wrote:
>> +  /* Make sure we have the right structure for the following memset.  */
>> +  _Static_assert ((uintptr_t) &State == (uintptr_t) &State.regs,
>> +		  "&State != &State.regs");
> 
> we require C11, so you can include assert.h & use static_assert().
> -mike
> 

Pushed with that change. Thanks!