[libatomic] Fix testsuite regressions on ARM [raspberry pi].

Message ID 00e701da424c$c8bb0b60$5a312220$@nextmovesoftware.com
State Accepted
Headers
Series [libatomic] Fix testsuite regressions on ARM [raspberry pi]. |

Checks

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

Commit Message

Roger Sayle Jan. 8, 2024, 4:07 p.m. UTC
  Bootstrapping GCC on arm-linux-gnueabihf with --with-arch=armv6 currently
has a large number of FAILs in libatomic (regressions since last time I
attempted this).  The failure mode is related to IFUNC handling with the
file tas_8_2_.o containing an unresolved reference to the function
libat_test_and_set_1_i2.

Bearing in mind I've no idea what's going on, the following one line
change, to build tas_1_2_.o when building tas_8_2_.o, resolves the problem
for me and restores the libatomic testsuite to 44 expected passes and 5
unsupported tests [from 22 unexpected failures and 22 unresolved testcases].

If this looks like the correct fix, I'm not confident with rebuilding
Makefile.in with correct version of automake, so I'd very much appreciate
it if someone/the reviewer/mainainer could please check this in for me.
Thanks in advance.


2024-01-08  Roger Sayle  <roger@nextmovesoftware.com>

libatomic/ChangeLog
        * Makefile.am: Build tas_1_2_.o on ARCH_ARM_LINUX
        * Makefile.in: Regenerate.


Roger
--
  

Comments

Richard Earnshaw Jan. 10, 2024, 3:33 p.m. UTC | #1
On 08/01/2024 16:07, Roger Sayle wrote:
> 
> Bootstrapping GCC on arm-linux-gnueabihf with --with-arch=armv6 currently
> has a large number of FAILs in libatomic (regressions since last time I
> attempted this).  The failure mode is related to IFUNC handling with the
> file tas_8_2_.o containing an unresolved reference to the function
> libat_test_and_set_1_i2.
> 
> Bearing in mind I've no idea what's going on, the following one line
> change, to build tas_1_2_.o when building tas_8_2_.o, resolves the problem
> for me and restores the libatomic testsuite to 44 expected passes and 5
> unsupported tests [from 22 unexpected failures and 22 unresolved testcases].
> 
> If this looks like the correct fix, I'm not confident with rebuilding
> Makefile.in with correct version of automake, so I'd very much appreciate
> it if someone/the reviewer/mainainer could please check this in for me.
> Thanks in advance.
> 
> 
> 2024-01-08  Roger Sayle  <roger@nextmovesoftware.com>
> 
> libatomic/ChangeLog
>          * Makefile.am: Build tas_1_2_.o on ARCH_ARM_LINUX
>          * Makefile.in: Regenerate.
> 
> 
> Roger
> --
> 

Hi Roger,

I don't really understand all this make foo :( so I'm not sure if this 
is the right fix either.  If this is, as you say, a regression, have you 
been able to track down when it first started to occur?  That might also 
help me to understand what changed to cause this.

Perhaps we should have a PR for this, to make tracking the fixes easier.

R.
  
Roger Sayle Jan. 11, 2024, 3:55 p.m. UTC | #2
Hi Richard,
As you've recommended, this issue has now been filed in bugzilla
as PR other/113336.  As explained in the new PR, libatomic's testsuite
used to pass on armv6 (raspberry pi) in previous GCC releases, but
the code was incorrect/non-synchronous; this was reported as
PR target/107567 and PR target/109166.  Now that those issues
have been fixed, we now see that there's a missing dependency in
libatomic that's required to implement this functionality correctly.

I'm more convinced that my fix is correct, but it's perhaps a little
disappointing that libatomic doesn't have a (multi-threaded) run-time
test to search for race conditions, and confirm its implementations
are correctly serializing.

Please let me know what you think.
Best regards,
Roger
--

> -----Original Message-----
> From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com>
> Sent: 10 January 2024 15:34
> To: Roger Sayle <roger@nextmovesoftware.com>; gcc-patches@gcc.gnu.org
> Subject: Re: [libatomic PATCH] Fix testsuite regressions on ARM [raspberry pi].
> 
> 
> 
> On 08/01/2024 16:07, Roger Sayle wrote:
> >
> > Bootstrapping GCC on arm-linux-gnueabihf with --with-arch=armv6
> > currently has a large number of FAILs in libatomic (regressions since
> > last time I attempted this).  The failure mode is related to IFUNC
> > handling with the file tas_8_2_.o containing an unresolved reference
> > to the function libat_test_and_set_1_i2.
> >
> > Bearing in mind I've no idea what's going on, the following one line
> > change, to build tas_1_2_.o when building tas_8_2_.o, resolves the
> > problem for me and restores the libatomic testsuite to 44 expected
> > passes and 5 unsupported tests [from 22 unexpected failures and 22 unresolved
> testcases].
> >
> > If this looks like the correct fix, I'm not confident with rebuilding
> > Makefile.in with correct version of automake, so I'd very much
> > appreciate it if someone/the reviewer/mainainer could please check this in for
> me.
> > Thanks in advance.
> >
> >
> > 2024-01-08  Roger Sayle  <roger@nextmovesoftware.com>
> >
> > libatomic/ChangeLog
> >          * Makefile.am: Build tas_1_2_.o on ARCH_ARM_LINUX
> >          * Makefile.in: Regenerate.
> >
> >
> > Roger
> > --
> >
> 
> Hi Roger,
> 
> I don't really understand all this make foo :( so I'm not sure if this is the right fix
> either.  If this is, as you say, a regression, have you been able to track down when
> it first started to occur?  That might also help me to understand what changed to
> cause this.
> 
> Perhaps we should have a PR for this, to make tracking the fixes easier.
> 
> R.
  
Victor Do Nascimento Jan. 25, 2024, 5:27 p.m. UTC | #3
On 1/11/24 15:55, Roger Sayle wrote:
> 
> Hi Richard,
> As you've recommended, this issue has now been filed in bugzilla
> as PR other/113336.  As explained in the new PR, libatomic's testsuite
> used to pass on armv6 (raspberry pi) in previous GCC releases, but
> the code was incorrect/non-synchronous; this was reported as
> PR target/107567 and PR target/109166.  Now that those issues
> have been fixed, we now see that there's a missing dependency in
> libatomic that's required to implement this functionality correctly.
> 
> I'm more convinced that my fix is correct, but it's perhaps a little
> disappointing that libatomic doesn't have a (multi-threaded) run-time
> test to search for race conditions, and confirm its implementations
> are correctly serializing.
> 
> Please let me know what you think.
> Best regards,
> Roger
> --

I do think that if the regression is caused by HAVE_ATOMIC_TAS now being 
detected as false due to a bugfix elsewhere as you kindly pointed out, 
then the fix perhaps ought to change the compile-time behavior for TAS 
alone.

As I point out in Bugzilla, we can get away with replacing the proposed

   libatomic_la_LIBADD += $(addsuffix _1_2_.lo,$(SIZEOBJS))

with

   libatomic_la_LIBADD += tas_1_2_.lo

so that we generate the missing `libat_test_and_set_1_i2' specifically.
I've not manage to detect the need for any other *_1_i2 thus far and 
this alone appears sufficient to fix all observed regressions.

Happy to investigate further, but my initial findings seem to be that 
this may be a better fix.

Let me know if you disagree ;).

Regards,
Victor

>> -----Original Message-----
>> From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com>
>> Sent: 10 January 2024 15:34
>> To: Roger Sayle <roger@nextmovesoftware.com>; gcc-patches@gcc.gnu.org
>> Subject: Re: [libatomic PATCH] Fix testsuite regressions on ARM [raspberry pi].
>>
>>
>>
>> On 08/01/2024 16:07, Roger Sayle wrote:
>>>
>>> Bootstrapping GCC on arm-linux-gnueabihf with --with-arch=armv6
>>> currently has a large number of FAILs in libatomic (regressions since
>>> last time I attempted this).  The failure mode is related to IFUNC
>>> handling with the file tas_8_2_.o containing an unresolved reference
>>> to the function libat_test_and_set_1_i2.
>>>
>>> Bearing in mind I've no idea what's going on, the following one line
>>> change, to build tas_1_2_.o when building tas_8_2_.o, resolves the
>>> problem for me and restores the libatomic testsuite to 44 expected
>>> passes and 5 unsupported tests [from 22 unexpected failures and 22 unresolved
>> testcases].
>>>
>>> If this looks like the correct fix, I'm not confident with rebuilding
>>> Makefile.in with correct version of automake, so I'd very much
>>> appreciate it if someone/the reviewer/mainainer could please check this in for
>> me.
>>> Thanks in advance.
>>>
>>>
>>> 2024-01-08  Roger Sayle  <roger@nextmovesoftware.com>
>>>
>>> libatomic/ChangeLog
>>>           * Makefile.am: Build tas_1_2_.o on ARCH_ARM_LINUX
>>>           * Makefile.in: Regenerate.
>>>
>>>
>>> Roger
>>> --
>>>
>>
>> Hi Roger,
>>
>> I don't really understand all this make foo :( so I'm not sure if this is the right fix
>> either.  If this is, as you say, a regression, have you been able to track down when
>> it first started to occur?  That might also help me to understand what changed to
>> cause this.
>>
>> Perhaps we should have a PR for this, to make tracking the fixes easier.
>>
>> R.
>
  

Patch

diff --git a/libatomic/Makefile.am b/libatomic/Makefile.am
index cfad90124f9..e0988a18c9a 100644
--- a/libatomic/Makefile.am
+++ b/libatomic/Makefile.am
@@ -139,6 +139,7 @@  if ARCH_ARM_LINUX
 IFUNC_OPTIONS	     = -march=armv7-a+fp -DHAVE_KERNEL64
 libatomic_la_LIBADD += $(foreach s,$(SIZES),$(addsuffix _$(s)_1_.lo,$(SIZEOBJS)))
 libatomic_la_LIBADD += $(addsuffix _8_2_.lo,$(SIZEOBJS))
+libatomic_la_LIBADD += $(addsuffix _1_2_.lo,$(SIZEOBJS))
 endif
 if ARCH_I386
 IFUNC_OPTIONS	     = -march=i586