NetBSD Problem Report #56839

From www@netbsd.org  Mon May 16 15:13:18 2022
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id CA1881A921F
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 16 May 2022 15:13:18 +0000 (UTC)
Message-Id: <20220516151317.5F00B1A923A@mollari.NetBSD.org>
Date: Mon, 16 May 2022 15:13:17 +0000 (UTC)
From: rokuyama.rk@gmail.com
Reply-To: rokuyama.rk@gmail.com
To: gnats-bugs@NetBSD.org
Subject: GCC emits wrong codes for compare_and_swap_1 bultins on armv5 (el & eb)
X-Send-Pr-Version: www-1.0

>Number:         56839
>Category:       port-arm
>Synopsis:       GCC emits wrong codes for compare_and_swap_1 bultins on armv5 (el & eb)
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    port-arm-maintainer
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon May 16 15:15:00 +0000 2022
>Last-Modified:  Mon Apr 28 06:00:02 +0000 2025
>Originator:     Rin Okuyama
>Release:        9.99.96
>Organization:
Department of Physics, Meiji University
>Environment:
NetBSD hdlg 9.99.96 NetBSD 9.99.96 (HDL_G) #5: Sun May 15 19:24:35 JST 2022  rin@latipes:/build/src/sys/arch/evbarm/compile/HDL_G evbarm
>Description:
Tests for __sync_{bool,val}_compare_and_swap_1() fail on armv5:

https://www.netbsd.org/~martin/evbarm-atf/402_atf.html#lib_libc_atomic_t___sync_compare_and_swap___sync_bool_compare_and_swap_1

These failures are observed both for little- and big-endian.

This is because GCC emits wrong codes for these built-in functions.
For some cases, GCC uses "mvn" insn to generate 2nd ("expected") and
3rd ("new") arguments for these builtins.

For example, it emits "mvn reg,#0x77" for 0x88, which results in
0xffffff88. This is wrong as these arguments are passed via registers;
inappropriate 0xff's in upper 3 bytes for "expected" argument make
comparison with uint8_t value of "*ptr" unconditionally fail.

This kind of wrong immediate generations are not observed for other
functions that take uint8_t arguments. Therefore, something is wrong
for treatments peculiar to these builtins.

Note that Linux version of __sync_bool_compare_and_swap_N() takes
signed integers as the 2nd and 3rd arguments on arm, whereas these are
unsigned for us. This may be related to this wrong "sign-extension".

The above-mentioned scenario is confirmed by a minimal stripped-down
version of the test for __sync_bool_compare_and_swap_1() (and
_atomic_cas_8(), which is used internally for this builtin):

https://gist.github.com/rokuyama/f469cbf96774db1f4658f0bb932c7a23

It fails for __sync_bool_compare_and_swap_1(), and succeeds for
_atomic_cas_8():

----
$ cc -g -O2 -c arm_builtin_compare_and_swap_1.c
$ cc arm_builtin_compare_and_swap_1.o
$ ./a.out
__sync_bool_compare_and_swap_1: val expects 0xf0 but 0x88
----

(Here, "-O2" is only for readability of objdump (see below).
The same failure occurs even with "-O0".)

objdump for this test case reads:

https://gist.github.com/rokuyama/eaf499d1e57767dbbe3e7bf30b2e0fce

Here, GCC generates 0x88 by "mvn" and "mov" for arguments for
__sync_bool_compare_and_swap_1() and _atomic_cas_8(), respectively.
>How-To-Repeat:
cd /usr/tests/lib/libc/atomic && atf-run t___sync_compare_and_swap

The stripped-down version of this test is provided above.
>Fix:
N/A

>Audit-Trail:
From: Nick Hudson <nick.hudson@gmx.co.uk>
To: gnats-bugs@netbsd.org, port-arm-maintainer@netbsd.org,
 gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Cc: 
Subject: Re: port-arm/56839: GCC emits wrong codes for compare_and_swap_1
 bultins on armv5 (el & eb)
Date: Wed, 18 May 2022 12:48:54 +0100

 I think this fixes things. It's a bit ugly...

 https://www.netbsd.org/~skrll/pr56839.diff

 Nick

From: Rin Okuyama <rokuyama.rk@gmail.com>
To: Nick Hudson <nick.hudson@gmx.co.uk>, gnats-bugs@netbsd.org,
 port-arm-maintainer@netbsd.org, gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org
Cc: 
Subject: Re: port-arm/56839: GCC emits wrong codes for compare_and_swap_1
 bultins on armv5 (el & eb)
Date: Fri, 20 May 2022 10:32:32 +0900

 Hi,

 On 2022/05/18 20:48, Nick Hudson wrote:
 > I think this fixes things. It's a bit ugly...
 > 
 > https://www.netbsd.org/~skrll/pr56839.diff

 This does not work:

 (1) GCC expands __sync_bool_compare_and_swap_N() into
      __sync_val_compare_and_swap_N() as optimization.

 (2) The patch does not cover __sync_val_compare_and_swap_N()
      ( == _atomic_cas_8N() ).

 On the other hand, this patch for _atomic_cas_8N() fixes the problem:

 https://gist.github.com/rokuyama/3d791f2ceb2757dc389a95910162c9cb

 However, IMO, this is not the real fix:

 (a) I've found that MI part, not arm-dependent part, of GCC carries
      out sign-extension; something is wrong in
      expand_builtin_compare_and_swap() @ dist/gcc/gcc/builtins.c,
      but I've never fully understand what goes wrong yet...
      (Strangely enough, GCC itself recognizes that these builtins
      takes unsigned integers...)

 (b) Therefore, archs other than arm that do not have 1- and 2-byte
      atomic cas builtins (== missing equivalent instructions) should
      be affected.

 (c) It should be better to fix GCC rather than adding workaround to
      libc routines.

 Thanks,
 rin

From: Rin Okuyama <rokuyama.rk@gmail.com>
To: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>, netbsd-bugs@netbsd.org
Cc: 
Subject: Re: port-arm/56839 (GCC emits wrong codes for compare_and_swap_1
 bultins on armv5 (el & eb))
Date: Tue, 15 Aug 2023 13:02:24 +0900

 This still fails in the same manner even for GCC 12.3.0 and 10.5.0.

From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: port-arm/56839: GCC emits wrong codes for compare_and_swap_1
 builtins
Date: Wed, 5 Feb 2025 11:50:18 +0100

 I am not convinced GCC is wrong here. The ABI says the caller needs to
 0-extend or sign-extend the sub-word value and pass it as a 32bit register
 value. So it all boils down to what the signature of the called function
 is - and unfortunately that seems to be poorly or not documented anywhere.

 But: gcc/dist/libgcc/config/arm/ implements the functions as taking
 signed values - so that would match the gcc generated call sites.

 #define SUBWORD_VAL_CAS(TYPE, WIDTH)                                    \
   TYPE HIDDEN                                                           \
   __sync_val_compare_and_swap_##WIDTH (TYPE *ptr, TYPE oldval,          \
                                        TYPE newval)                     \

 SUBWORD_VAL_CAS (short,       2)
 SUBWORD_VAL_CAS (signed char, 1)


 I think we need to make our functions match that (basically Nick's patch,
 plus maybe extensions to a few other functions).

 Martin

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56839 CVS commit: src/tests/lib/libc/atomic
Date: Thu, 13 Mar 2025 14:00:51 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Thu Mar 13 14:00:51 UTC 2025

 Modified Files:
 	src/tests/lib/libc/atomic: t___sync_compare_and_swap.c

 Log Message:
 tests/lib/libc/atomic: Test subword compare-and-swap explicitly.

 Make sure the sign doesn't bleed into an adjacent word.

 We already had failing tests, but this should make the failures a
 little more obvious and perhaps check for more adjacent problems.

 PR port-arm/56839: GCC emits wrong codes for compare_and_swap_1
 bultins on armv5 (el & eb)


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r1.2 \
     src/tests/lib/libc/atomic/t___sync_compare_and_swap.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Taylor R Campbell <riastradh@NetBSD.org>
To: Rin Okuyama <rokuyama.rk@gmail.com>,
	Nick Hudson <nick.hudson@gmx.co.uk>,
	Martin Husemann <martin@duskware.de>
Cc: gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org
Subject: Re: port-arm/56839: GCC emits wrong codes for compare_and_swap_1 bultins on armv5 (el & eb)
Date: Thu, 13 Mar 2025 14:29:40 +0000

 It looks like gcc is _inconsistent_ about the signedness of the
 __sync_* builtins.  Here, for example, in gcc-14.2.0, it appears to
 use unsigned arguments in libgcc:

  134 typedef unsigned int UQItype __attribute__((mode (QI)));
  135 DEFINE (FN, 1, UQItype)
 ...
  139 typedef unsigned int UHItype __attribute__((mode (HI)));
  140 DEFINE (FN, 2, UHItype)
 ...
  144 typedef unsigned int USItype __attribute__((mode (SI)));
  145 DEFINE (FN, 4, USItype)
 ...

 https://gcc.gnu.org/git/?p=3Dgcc.git;a=3Dblob;f=3Dlibgcc/sync.c;h=3D6ffe88a=
 ac0f08d1adbc0b02f7f10d50d95d4c6eb;hb=3D04696df09633baf97cdbbdd6e9929b9d4721=
 61d3#l125

 And, for _FreeBSD_ on arm32, it also uses unsigned arguments:

  220 EMIT_ALL_OPS_N (1, unsigned char, "ldrb", "strb", "streqb")
  221 EMIT_ALL_OPS_N (2, unsigned short, "ldrh", "strh", "streqh")
  222 EMIT_ALL_OPS_N (4, unsigned int, "ldr", "str", "streq")

 https://gcc.gnu.org/git/?p=3Dgcc.git;a=3Dblob;f=3Dlibgcc/config/arm/freebsd=
 -atomic.c;h=3D7cf00b1e75d4759b983401797f0224764a2ce82d;hb=3D04696df09633baf=
 97cdbbdd6e9929b9d472161d3#l220

 But elsewhere, e.g. for _Linux_ on arm32, it uses signed arguments:

  249 SUBWORD_VAL_CAS (short,       2)
  250 SUBWORD_VAL_CAS (signed char, 1)

 https://gcc.gnu.org/git/?p=3Dgcc.git;a=3Dblob;f=3Dlibgcc/config/arm/linux-a=
 tomic.c;h=3D6d6683194aff0404967b28030bc7677b0f2e5949;hb=3D04696df09633baf97=
 cdbbdd6e9929b9d472161d3#l249

 And I have no idea about the code generation parts of things.

 We should file a bug upstream, perhaps.

 But until we do, I think it should be sufficient to write all of our
 __sync_* routines -- including both __sync_bool_* and __sync_val_*, of
 course -- with signed arguments and pass them to the unsigned
 _atomic_* functions, like Nick suggested.

 This should paper over the gcc issue, by forcing gcc to generate code
 that always zero-extends the subword when passing arguments in
 registers so the comparison will succeed (at least, for ABIs where
 that is relevant), without requiring changes to our _atomic_*
 definitions which are perfectly good for their signatures.

 I guess it is conceivable that there is an ABI where this will do the
 wrong thing but I doubt it.  Even on riscv64, where 8/16-bit units are
 type-extended to 32 bit and then sign-extended to 64 bits (no matter
 the signedness of the type), I think this will produce the correct
 result.  (Also I think these stubs won't be used on riscv anyway.)=20

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56839 CVS commit: src/tests/lib/libc/atomic
Date: Fri, 25 Apr 2025 12:11:14 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Apr 25 12:11:14 UTC 2025

 Modified Files:
 	src/tests/lib/libc/atomic: t___sync_compare_and_swap.c

 Log Message:
 t___sync_compare_and_swap: Mark tests xfail on armv5.

 PR port-arm/56839: GCC emits wrong codes for compare_and_swap_1
 bultins on armv5 (el & eb)


 To generate a diff of this commit:
 cvs rdiff -u -r1.2 -r1.3 \
     src/tests/lib/libc/atomic/t___sync_compare_and_swap.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@netbsd.org
Cc: rokuyama.rk@gmail.com
Subject: Re: PR/56839 CVS commit: src/tests/lib/libc/atomic
Date: Fri, 25 Apr 2025 14:21:38 +0200

 Rin, I would like to resolve this along the lines of the fix suggested
 by skrll (but extended to whatever other primitives that change did
 not cover). Would you be OK with that?

 If so, I'll prepare a patch with the other functions covered.

 Martin

From: Rin Okuyama <rokuyama.rk@gmail.com>
To: Martin Husemann <martin@duskware.de>, gnats-bugs@netbsd.org
Cc: 
Subject: Re: PR/56839 CVS commit: src/tests/lib/libc/atomic
Date: Mon, 28 Apr 2025 14:57:21 +0900

 On 2025/04/25 21:21, Martin Husemann wrote:
 > Rin, I would like to resolve this along the lines of the fix suggested
 > by skrll (but extended to whatever other primitives that change did
 > not cover). Would you be OK with that?
 > 
 > If so, I'll prepare a patch with the other functions covered.

 Thanks, please!!

 // Belatedly, I catched up with this PR. The current GCC behavior is
 // not optimal, but there should be ~ no practical benefits to ``fix''
 // GCC in this case, IMO...

 rin

 > 
 > Martin

Home	PR Database Search