Re: Sim bfin build failure with gcc 11

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-05-23 10:16 p.m., Mike Frysinger wrote:
> On 23 May 2021 21:41, Simon Marchi via Gdb-patches wrote:
>> On 2021-05-23 9:37 p.m., Mike Frysinger wrote:
>>> On 23 May 2021 20:51, Simon Marchi via Gdb-patches wrote:
>>>> I see this with gcc 11:
>>>>
>>>> $ ccache gcc  -DHAVE_CONFIG_H -DWITH_DEFAULT_MODEL='"bf537"' -DWITH_DEFAULT_ALIGNMENT=STRICT_ALIGNMENT  -DWITH_TARGET_BYTE_ORDER=BFD_ENDIAN_LITTLE  -DWITH_HW=1 -DDEFAULT_INLINE=0   -Wall -Wdeclaration-after-statement -Wpointer-arith -Wpointer-sign -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wmissing-prototypes -Wdeclaration-after-statement -Wempty-body -Wmissing-parameter-type -Wold-style-declaration -Werror  -I. -I/home/simark/src/binutils-gdb/sim/bfin -I../common -I/home/simark/src/binutils-gdb/sim/bfin/../common -I../../include -I/home/simark/src/binutils-gdb/sim/bfin/../../include -I../../bfd -I/home/simark/src/binutils-gdb/sim/bfin/../../bfd -I../../opcodes -I/home/simark/src/binutils-gdb/sim/bfin/../../opcodes  -I/usr/include/SDL -D_GNU_SOURCE=1 -D_REENTRANT  -DHAVE_SDL -g3 -O0 -fsanitize=address -fmax-errors=1     -c -o dv-bfin_otp.o -MT dv-bfin_otp.o -MMD -MP -MF .deps/dv-bfin_otp.Tpo /home/simark/src/binutils-gdb/sim/bfin/dv-bfin_otp.c
>>>> /home/simark/src/binutils-gdb/sim/bfin/dv-bfin_otp.c: In function ‘bfin_otp_write_page’:
>>>> /home/simark/src/binutils-gdb/sim/bfin/dv-bfin_otp.c:94:3: error: ‘bfin_otp_write_page_val’ accessing 16 bytes in a region of size 4 [-Werror=stringop-overflow=]
>>>>    94 |   bfin_otp_write_page_val (otp, page, (void *)&otp->data0);
>>>>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> /home/simark/src/binutils-gdb/sim/bfin/dv-bfin_otp.c:94:3: note: referencing argument 3 of type ‘bu64 *’ {aka ‘long unsigned int *’}
>>>> /home/simark/src/binutils-gdb/sim/bfin/dv-bfin_otp.c:81:1: note: in a call to function ‘bfin_otp_write_page_val’
>>>>    81 | bfin_otp_write_page_val (struct bfin_otp *otp, bu16 page, bu64 val[2])
>>>>       | ^~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> It's not immediately obvious to me why it says that.
>>>
>>> gcc really wants its pointer types to be in harmony.  i cheated here.
>>>
>>> write_page_val wants a pointer to an array of 2 64-bit values.  i know the
>>> otp struct has "bu64 data0, data1, data2, data3;", and taking the address
>>> of data0 has the same memory layout as if it were "bu64 data[3];".  but with
>>> the fortify work gcc has long been doing, they've stopped accepting these
>>> kinds of hacks.
>>>
>>> this code is not perf sensitive, so i can be less "clever" while keeping the
>>> compiler happy.
>>
>> Ah, I see.  Well if what you want is make sure the fields are
>> contiguous, why not make that an array of four bu32 instead?
> 
> i'm not worried about the ordering ... it's a struct, so that's guaranteed.
> the code uses a style to make it easy to compare against the datasheets and
> to make it easy to access individual fields.  the datasheets have "DATA0"
> an "DATA1" and such as explicit MMRs, not DATA[3].
> 
> but i misread the code and thought it said bu64 data0... when it's really
> bu32 as you point out.  which is why i was being clever in the first place,
> and the patch i just pushed is incorrect.
> 
> i guess the existing code was assuming the host cpu was little endian, so
> doing a manual construction of the 64bit fields is unavoidable.  so let's
> try this again.
> -mike
> 
> commit d699be882b42f36677836c320edbf7db24021a30
> Author: Mike Frysinger <vapier@gentoo.org>
> Date:   Sun May 23 22:15:01 2021 -0400
> 
>     sim: bfin: fix the otp fix
>     
>     I misread the code and thought data0/... were bu64 when they were
>     actually bu32.  Fix the call to assemble the 2 64-bit values instead
>     of passing the 2 halves of the first 64-bit value.
> 
> diff --git a/sim/bfin/ChangeLog b/sim/bfin/ChangeLog
> index 9c517d20bbeb..29dfde8fe6e3 100644
> --- a/sim/bfin/ChangeLog
> +++ b/sim/bfin/ChangeLog
> @@ -1,3 +1,8 @@
> +2021-05-23  Mike Frysinger  <vapier@gentoo.org>
> +
> +	* dv-bfin_otp.c (bfin_otp_write_page): Fix args to
> +	bfin_otp_write_page_val2.
> +
>  2021-05-23  Mike Frysinger  <vapier@gentoo.org>
>  
>  	* dv-bfin_otp.c (bfin_otp_write_page): Call bfin_otp_write_page_val2.
> diff --git a/sim/bfin/dv-bfin_otp.c b/sim/bfin/dv-bfin_otp.c
> index 65afdf58b27f..cdc010ae551b 100644
> --- a/sim/bfin/dv-bfin_otp.c
> +++ b/sim/bfin/dv-bfin_otp.c
> @@ -91,7 +91,8 @@ bfin_otp_write_page_val2 (struct bfin_otp *otp, bu16 page, bu64 lo, bu64 hi)
>  static void
>  bfin_otp_write_page (struct bfin_otp *otp, bu16 page)
>  {
> -  bfin_otp_write_page_val2 (otp, page, otp->data0, otp->data1);
> +  bfin_otp_write_page_val2 (otp, page, (bu64)otp->data1 | otp->data0,
> +			    (bu64)otp->data3 | otp->data2);

I really don't know what this is doing but... did you mean to have some
shifts in there?

Simon