diff mbox series

libffi: fix handling of homogeneous float128 structs [PR109447]

Message ID f3381264-616a-6c76-3357-7dec1f60696d@linux.ibm.com
State Accepted
Headers
Series libffi: fix handling of homogeneous float128 structs [PR109447] |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Peter Bergner May 4, 2023, 7:29 p.m. UTC 
  I'd like to pull in Dan's upstream libffi commit into trunk to fix a
wrong code bug/testsuite failure on powerpc64le-linux with long double
defaulting to ieee128.  This passed bootstrap and regtesting with no
regressions.  Ok for trunk?

This bug is also on the GCC 12 and GCC 11 release branches. Ok there too
assuming testing is clean?  I can wait to push the gcc12 backport until
after the release.

Peter


If there is a homogeneous struct with float128 members, they should be
copied to vector register save area. The current code incorrectly copies
only the value of the first member, not increasing the pointer with each
iteration. Fix this.

Merged from upstream libffi commit: 464b4b66e3cf3b5489e730c1466ee1bf825560e0

2023-05-03  Dan Horák <dan@danny.cz>

libffi/
	PR libffi/109447
	* src/powerpc/ffi_linux64.c (ffi_prep_args64): Update arg.f128 pointer.

Comments

Peter Bergner May 5, 2023, 9:39 p.m. UTC | #1 
On 5/4/23 2:29 PM, Peter Bergner wrote:
> I'd like to pull in Dan's upstream libffi commit into trunk to fix a
> wrong code bug/testsuite failure on powerpc64le-linux with long double
> defaulting to ieee128.  This passed bootstrap and regtesting with no
> regressions.  Ok for trunk?
> 
> This bug is also on the GCC 12 and GCC 11 release branches. Ok there too
> assuming testing is clean?  I can wait to push the gcc12 backport until
> after the release.

Oops, and of course, this needs to be backported to GCC 13 as well.

Peter
Jakub Jelinek May 5, 2023, 9:42 p.m. UTC | #2 
On Thu, May 04, 2023 at 02:29:34PM -0500, Peter Bergner wrote:
> I'd like to pull in Dan's upstream libffi commit into trunk to fix a
> wrong code bug/testsuite failure on powerpc64le-linux with long double
> defaulting to ieee128.  This passed bootstrap and regtesting with no
> regressions.  Ok for trunk?
> 
> This bug is also on the GCC 12 and GCC 11 release branches. Ok there too
> assuming testing is clean?  I can wait to push the gcc12 backport until
> after the release.
> 
> Peter
> 
> 
> If there is a homogeneous struct with float128 members, they should be
> copied to vector register save area. The current code incorrectly copies
> only the value of the first member, not increasing the pointer with each
> iteration. Fix this.
> 
> Merged from upstream libffi commit: 464b4b66e3cf3b5489e730c1466ee1bf825560e0
> 
> 2023-05-03  Dan Horák <dan@danny.cz>
> 
> libffi/
> 	PR libffi/109447
> 	* src/powerpc/ffi_linux64.c (ffi_prep_args64): Update arg.f128 pointer.

Ok for 14/13.2/12.4 (i.e. after 12.3 is out)/11.4

> diff --git a/libffi/src/powerpc/ffi_linux64.c b/libffi/src/powerpc/ffi_linux64.c
> index 4d50878e402..3454dacd3d6 100644
> --- a/libffi/src/powerpc/ffi_linux64.c
> +++ b/libffi/src/powerpc/ffi_linux64.c
> @@ -680,7 +680,7 @@ ffi_prep_args64 (extended_cif *ecif, unsigned long *const stack)
>                      {
>                        if (vecarg_count < NUM_VEC_ARG_REGISTERS64
>                            && i < nfixedargs)
> -		        memcpy (vec_base.f128++, arg.f128, sizeof (float128));
> +		        memcpy (vec_base.f128++, arg.f128++, sizeof (float128));
>                        else
>  		        memcpy (next_arg.f128, arg.f128++, sizeof (float128));
>                        if (++next_arg.f128 == gpr_end.f128)

	Jakub
Peter Bergner May 9, 2023, 8:24 p.m. UTC | #3 
On 5/5/23 4:42 PM, Jakub Jelinek wrote:
> On Thu, May 04, 2023 at 02:29:34PM -0500, Peter Bergner wrote:
>> Merged from upstream libffi commit: 464b4b66e3cf3b5489e730c1466ee1bf825560e0
>>
>> 2023-05-03  Dan Horák <dan@danny.cz>
>>
>> libffi/
>> 	PR libffi/109447
>> 	* src/powerpc/ffi_linux64.c (ffi_prep_args64): Update arg.f128 pointer.
> 
> Ok for 14/13.2/12.4 (i.e. after 12.3 is out)/11.4

Thanks, I've pushed the GCC trunk and GCC 13 commits and now that GCC 12.3
is released, I have pushed the GCC 12 backport too.

I have yet to push to GCC 11 yet, due to bootstrap is broken when building
GCC 11 on our Fedora 36 system, so I cannot test there yet.  It also seems
GCC 11 is missing some IEEE128 changes from upstream libffi that GCC 12 and
later have, so it might not even be appropriate, but I'll wait for bootstrap
to be restored before making any decisions.  The problem seem to be that the
system ld.gold which is used to link libgo.so is dying with an undefined
runtime symbol:

/usr/bin/ld.gold: /home/bergner/gcc/build/gcc-fsf-11-baselib-regtest/powerpc64le-linux/libstdc++-v3/src/.libs/libstdc++.so.6: version `GLIBCXX_3.4.30' not found (required by /usr/bin/ld.gold)
collect2: error: ld returned 1 exit status

Running the link command by hand or via a make in powerpc64le-linux/libgo,
the link succeeds.  It's only when I type make in the top level build dir
where I see this error.  It's almost as if the top level build machinery
adds a LD_LIBRARY_PATH=... forcing the system ld.gold (which was built
with a gcc12 based compiler) to pick up the build's gcc11 libstdc++ which
doesn't have that symbol, rather than the gcc12 system libstdc++.  Has anyone
seen anything like that before?

Peter
Andreas Schwab May 9, 2023, 8:50 p.m. UTC | #4 
On Mai 09 2023, Peter Bergner via Gcc-patches wrote:

> It's almost as if the top level build machinery
> adds a LD_LIBRARY_PATH=...

See how the toplevel Makefile sets LD_LIBRARY_PATH (via RPATH_ENVVAR) if
gcc-bootstrap is set.
Peter Bergner May 9, 2023, 9:20 p.m. UTC | #5 
On 5/9/23 3:50 PM, Andreas Schwab wrote:
> On Mai 09 2023, Peter Bergner via Gcc-patches wrote:
> 
>> It's almost as if the top level build machinery
>> adds a LD_LIBRARY_PATH=...
> 
> See how the toplevel Makefile sets LD_LIBRARY_PATH (via RPATH_ENVVAR) if
> gcc-bootstrap is set.

I'm sorry to be dense, but can you point to the specific line?  In my
$GCC_BUILD/Makefile, the only mention of LD_LIBRARY_PATH is:

  RPATH_ENVVAR = LD_LIBRARY_PATH

...so that isn't setting LD_LIBRARY_PATH, but using it.

Peter
Andreas Schwab May 10, 2023, 7:34 a.m. UTC | #6 
On Mai 09 2023, Peter Bergner via Gcc-patches wrote:

> On 5/9/23 3:50 PM, Andreas Schwab wrote:
>> On Mai 09 2023, Peter Bergner via Gcc-patches wrote:
>> 
>>> It's almost as if the top level build machinery
>>> adds a LD_LIBRARY_PATH=...
>> 
>> See how the toplevel Makefile sets LD_LIBRARY_PATH (via RPATH_ENVVAR) if
>> gcc-bootstrap is set.
>
> I'm sorry to be dense, but can you point to the specific line?  In my
> $GCC_BUILD/Makefile, the only mention of LD_LIBRARY_PATH is:
>
>   RPATH_ENVVAR = LD_LIBRARY_PATH
>
> ...so that isn't setting LD_LIBRARY_PATH, but using it.

Have you considered searching for uses of RPATH_ENVVAR?

$ grep RPATH_ENVVAR Makefile.in 
RPATH_ENVVAR = @RPATH_ENVVAR@
# On targets where RPATH_ENVVAR is PATH, a subdirectory of the GCC build path
        $(RPATH_ENVVAR)=`echo "$(TARGET_LIB_PATH)$$$(RPATH_ENVVAR)" | sed 's,::*,:,g;s,^:*,,;s,:*$$,,'`; export $(RPATH_ENVVAR); \
        $(RPATH_ENVVAR)=`echo "$(HOST_LIB_PATH)$$$(RPATH_ENVVAR)" | sed 's,::*,:,g;s,^:*,,;s,:*$$,,'`; export $(RPATH_ENVVAR);
        $(RPATH_ENVVAR)=`echo "$(TARGET_LIB_PATH)$$$(RPATH_ENVVAR)" | sed 's,::*,:,g;s,^:*,,;s,:*$$,,'`; export $(RPATH_ENVVAR); \
        $(RPATH_ENVVAR)=`echo "$(HOST_LIB_PATH)$$$(RPATH_ENVVAR)" | sed 's,::*,:,g;s,^:*,,;s,:*$$,,'`; export $(RPATH_ENVVAR); \
# This is the list of directories that may be needed in RPATH_ENVVAR
# This is the list of directories that may be needed in RPATH_ENVVAR
        "RPATH_ENVVAR=$(RPATH_ENVVAR)" \
Peter Bergner May 13, 2023, 5:16 p.m. UTC | #7 
On 5/10/23 2:34 AM, Andreas Schwab wrote:
> On Mai 09 2023, Peter Bergner via Gcc-patches wrote:
>> I'm sorry to be dense, but can you point to the specific line?  In my
>> $GCC_BUILD/Makefile, the only mention of LD_LIBRARY_PATH is:
>>
>>   RPATH_ENVVAR = LD_LIBRARY_PATH
>>
>> ...so that isn't setting LD_LIBRARY_PATH, but using it.
> 
> Have you considered searching for uses of RPATH_ENVVAR?

Ah, I misread that as RPATH_ENVVAR = $LD_LIBRARY_PATH and was
expecting to see "export LD_LIBRARY_PATH=..." in the code.
Thanks for the pointer!

Peter
diff mbox series

Patch

diff --git a/libffi/src/powerpc/ffi_linux64.c b/libffi/src/powerpc/ffi_linux64.c
index 4d50878e402..3454dacd3d6 100644
--- a/libffi/src/powerpc/ffi_linux64.c
+++ b/libffi/src/powerpc/ffi_linux64.c
@@ -680,7 +680,7 @@  ffi_prep_args64 (extended_cif *ecif, unsigned long *const stack)
                     {
                       if (vecarg_count < NUM_VEC_ARG_REGISTERS64
                           && i < nfixedargs)
-		        memcpy (vec_base.f128++, arg.f128, sizeof (float128));
+		        memcpy (vec_base.f128++, arg.f128++, sizeof (float128));
                       else
 		        memcpy (next_arg.f128, arg.f128++, sizeof (float128));
                       if (++next_arg.f128 == gpr_end.f128)