sh4: ensure FPSCR.PR==0 when executing FRCHG [BZ #27543]
Checks
Commit Message
If the bit is not 0, the operations FRCHG and FSCHG are
undefined and cause a trap; qemu now checks for this as
well, so we set it to 0 temporarily and restore the old
value in getcontext afterwards (setcontext/swapcontext
already do so).
From the discussion in the bugreport, this can probably
be optimised in one place but none of the people involved
are SH4 assembly experts, this patch is field-tested, and
it’s not a code path run often. The other question, what
happens if a signal occurs while the bit is temporarily 0,
is also still unsolved, but to fix that a kernel change is
most likely needed; this patch changes a certain trap on
many CPUs for a hard-to-get trap in a signal handler if a
signal is delivered during the few instructions the PR bit
is temporarily set to 0, so it’s not a regression for most
users.
See BZ and https://bugs.launchpad.net/qemu/+bug/1796520 for
related discussion, references and review comments.
Signed-off-by: mirabilos <tg@debian.org>
Reviewed-by: Oleg Endo <olegendo@gcc.gnu.org>
Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
---
sysdeps/unix/sysv/linux/sh/sh4/getcontext.S | 6 ++++++
sysdeps/unix/sysv/linux/sh/sh4/setcontext.S | 2 ++
sysdeps/unix/sysv/linux/sh/sh4/swapcontext.S | 2 ++
3 files changed, 10 insertions(+)
From b9015c5a51bfbf3b7dc59f52620a8d5ca307c54b Mon Sep 17 00:00:00 2001
From: mirabilos <tg@debian.org>
Date: Fri, 3 Jan 2025 18:53:21 +0000
Subject: [PATCH] sh4: ensure FPSCR.PR==0 when executing FRCHG [BZ #27543]
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
To: libc-alpha@sourceware.org
Cc: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>,
John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
If the bit is not 0, the operations FRCHG and FSCHG are
undefined and cause a trap; qemu now checks for this as
well, so we set it to 0 temporarily and restore the old
value in getcontext afterwards (setcontext/swapcontext
already do so).
From the discussion in the bugreport, this can probably
be optimised in one place but none of the people involved
are SH4 assembly experts, this patch is field-tested, and
it’s not a code path run often. The other question, what
happens if a signal occurs while the bit is temporarily 0,
is also still unsolved, but to fix that a kernel change is
most likely needed; this patch changes a certain trap on
many CPUs for a hard-to-get trap in a signal handler if a
signal is delivered during the few instructions the PR bit
is temporarily set to 0, so it’s not a regression for most
users.
See BZ and https://bugs.launchpad.net/qemu/+bug/1796520 for
related discussion, references and review comments.
Signed-off-by: mirabilos <tg@debian.org>
Reviewed-by: Oleg Endo <olegendo@gcc.gnu.org>
Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
---
sysdeps/unix/sysv/linux/sh/sh4/getcontext.S | 6 ++++++
sysdeps/unix/sysv/linux/sh/sh4/setcontext.S | 2 ++
sysdeps/unix/sysv/linux/sh/sh4/swapcontext.S | 2 ++
3 files changed, 10 insertions(+)
--
2.30.2
Comments
On 03/01/25 16:23, mirabilos wrote:
> If the bit is not 0, the operations FRCHG and FSCHG are
> undefined and cause a trap; qemu now checks for this as
> well, so we set it to 0 temporarily and restore the old
> value in getcontext afterwards (setcontext/swapcontext
> already do so).
>
> From the discussion in the bugreport, this can probably
> be optimised in one place but none of the people involved
> are SH4 assembly experts, this patch is field-tested, and
> it’s not a code path run often. The other question, what
> happens if a signal occurs while the bit is temporarily 0,
> is also still unsolved, but to fix that a kernel change is
> most likely needed; this patch changes a certain trap on
> many CPUs for a hard-to-get trap in a signal handler if a
> signal is delivered during the few instructions the PR bit
> is temporarily set to 0, so it’s not a regression for most
> users.
>
> See BZ and https://bugs.launchpad.net/qemu/+bug/1796520 for
> related discussion, references and review comments.
>
> Signed-off-by: mirabilos <tg@debian.org>
> Reviewed-by: Oleg Endo <olegendo@gcc.gnu.org>
> Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> ---
> sysdeps/unix/sysv/linux/sh/sh4/getcontext.S | 6 ++++++
> sysdeps/unix/sysv/linux/sh/sh4/setcontext.S | 2 ++
> sysdeps/unix/sysv/linux/sh/sh4/swapcontext.S | 2 ++
> 3 files changed, 10 insertions(+)
The patch does not apply from patchwork, but I could apply it manually.
Besides it, LGTM although I don't have a working hardware to actually
test it (and qemu-sh4-static shows a failure, setjmp/tst-setjmp-fp).
> From b9015c5a51bfbf3b7dc59f52620a8d5ca307c54b Mon Sep 17 00:00:00 2001
> From: mirabilos <tg@debian.org>
> Date: Fri, 3 Jan 2025 18:53:21 +0000
> Subject: [PATCH] sh4: ensure FPSCR.PR==0 when executing FRCHG [BZ #27543]
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> To: libc-alpha@sourceware.org
> Cc: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>,
> John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
>
> If the bit is not 0, the operations FRCHG and FSCHG are
> undefined and cause a trap; qemu now checks for this as
> well, so we set it to 0 temporarily and restore the old
> value in getcontext afterwards (setcontext/swapcontext
> already do so).
>
> From the discussion in the bugreport, this can probably
> be optimised in one place but none of the people involved
> are SH4 assembly experts, this patch is field-tested, and
> it’s not a code path run often. The other question, what
> happens if a signal occurs while the bit is temporarily 0,
> is also still unsolved, but to fix that a kernel change is
> most likely needed; this patch changes a certain trap on
> many CPUs for a hard-to-get trap in a signal handler if a
> signal is delivered during the few instructions the PR bit
> is temporarily set to 0, so it’s not a regression for most
> users.
>
> See BZ and https://bugs.launchpad.net/qemu/+bug/1796520 for
> related discussion, references and review comments.
>
> Signed-off-by: mirabilos <tg@debian.org>
> Reviewed-by: Oleg Endo <olegendo@gcc.gnu.org>
> Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> ---
> sysdeps/unix/sysv/linux/sh/sh4/getcontext.S | 6 ++++++
> sysdeps/unix/sysv/linux/sh/sh4/setcontext.S | 2 ++
> sysdeps/unix/sysv/linux/sh/sh4/swapcontext.S | 2 ++
> 3 files changed, 10 insertions(+)
>
> diff --git a/sysdeps/unix/sysv/linux/sh/sh4/getcontext.S b/sysdeps/unix/sysv/linux/sh/sh4/getcontext.S
> index 4470e5730b..329a790cd6 100644
> --- a/sysdeps/unix/sysv/linux/sh/sh4/getcontext.S
> +++ b/sysdeps/unix/sysv/linux/sh/sh4/getcontext.S
> @@ -67,6 +67,8 @@ ENTRY(__getcontext)
> add #(oFPUL+4-124),r0
> sts.l fpul, @-r0
> sts.l fpscr, @-r0
> + mov #0, r6
> + lds r6, fpscr
> frchg
> fmov.s fr15, @-r0
> fmov.s fr14, @-r0
> @@ -101,6 +103,10 @@ ENTRY(__getcontext)
> fmov.s fr2, @-r0
> fmov.s fr1, @-r0
> fmov.s fr0, @-r0
> + mov r4, r0
> + add #124, r0
> + add #(oFPSCR-124), r0
> + lds.l @r0+, fpscr
> #endif /* __SH_FPU_ANY__ */
>
> /* sigprocmask (SIG_BLOCK, NULL, &uc->uc_sigmask). */
> diff --git a/sysdeps/unix/sysv/linux/sh/sh4/setcontext.S b/sysdeps/unix/sysv/linux/sh/sh4/setcontext.S
> index a6d1de960c..60aff78256 100644
> --- a/sysdeps/unix/sysv/linux/sh/sh4/setcontext.S
> +++ b/sysdeps/unix/sysv/linux/sh/sh4/setcontext.S
> @@ -50,6 +50,8 @@ ENTRY(__setcontext)
>
> .Lsetcontext_restore:
> #ifdef __SH_FPU_ANY__
> + mov #0, r9
> + lds r9, fpscr
> mov r8, r0
> add #(oFR0),r0
> fmov.s @r0+, fr0
> diff --git a/sysdeps/unix/sysv/linux/sh/sh4/swapcontext.S b/sysdeps/unix/sysv/linux/sh/sh4/swapcontext.S
> index a299e05b41..6cf88f2b68 100644
> --- a/sysdeps/unix/sysv/linux/sh/sh4/swapcontext.S
> +++ b/sysdeps/unix/sysv/linux/sh/sh4/swapcontext.S
> @@ -67,6 +67,8 @@ ENTRY(__swapcontext)
> add #(oFPUL+4-124),r0
> sts.l fpul, @-r0
> sts.l fpscr, @-r0
> + mov #0, r9
> + lds r9, fpscr
> frchg
> fmov.s fr15, @-r0
> fmov.s fr14, @-r0
> --
> 2.30.2
Hi Adhemerval,
On Tue, 2025-01-07 at 12:10 -0300, Adhemerval Zanella Netto wrote:
>
> On 03/01/25 16:23, mirabilos wrote:
> > If the bit is not 0, the operations FRCHG and FSCHG are
> > undefined and cause a trap; qemu now checks for this as
> > well, so we set it to 0 temporarily and restore the old
> > value in getcontext afterwards (setcontext/swapcontext
> > already do so).
> >
> > From the discussion in the bugreport, this can probably
> > be optimised in one place but none of the people involved
> > are SH4 assembly experts, this patch is field-tested, and
> > it’s not a code path run often. The other question, what
> > happens if a signal occurs while the bit is temporarily 0,
> > is also still unsolved, but to fix that a kernel change is
> > most likely needed; this patch changes a certain trap on
> > many CPUs for a hard-to-get trap in a signal handler if a
> > signal is delivered during the few instructions the PR bit
> > is temporarily set to 0, so it’s not a regression for most
> > users.
> >
> > See BZ and https://bugs.launchpad.net/qemu/+bug/1796520 for
> > related discussion, references and review comments.
> >
> > Signed-off-by: mirabilos <tg@debian.org>
> > Reviewed-by: Oleg Endo <olegendo@gcc.gnu.org>
> > Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
> > ---
> > sysdeps/unix/sysv/linux/sh/sh4/getcontext.S | 6 ++++++
> > sysdeps/unix/sysv/linux/sh/sh4/setcontext.S | 2 ++
> > sysdeps/unix/sysv/linux/sh/sh4/swapcontext.S | 2 ++
> > 3 files changed, 10 insertions(+)
>
> The patch does not apply from patchwork, but I could apply it manually.
>
> Besides it, LGTM although I don't have a working hardware to actually
> test it (and qemu-sh4-static shows a failure, setjmp/tst-setjmp-fp).
Does the failure show before or after applying the patch?
For me, the patch fixes the problem that programs like debfoster which
will trigger a trap when run in an Debian unstable sh4 chroot without a
patched glibc:
(unstable-sh4-sbuild)root@adams:/# debfoster -f
Unhandled trap: 0x180
pc=0x2b3a807e sr=0x00000100 pr=0x2b3095da fpscr=0x00080000
spc=0x00000000 ssr=0x00000000 gbr=0x2b2fe200 vbr=0x00000000
sgr=0x00000000 dbr=0x00000000 delayed_pc=0x2b3a8040 fpul=0x00000000
r0=0x2b2ab8cc r1=0x00000000 r2=0xffffb8c6 r3=0x2b2efe78
r4=0x2b2ab7dc r5=0x2b2ab9b8 r6=0x2b34056c r7=0x00000000
r8=0x2b35f15c r9=0x00000021 r10=0x00000000 r11=0x00001000
r12=0x2b340250 r13=0x00000100 r14=0x2b35f158 r15=0x2b2ab70c
r16=0x00000000 r17=0x00000000 r18=0x00000000 r19=0x00000000
r20=0x00000000 r21=0x00000000 r22=0x00000000 r23=0x00000000
(unstable-sh4-sbuild)root@adams:/#
Since debfoster [1] relies on libgc, it might be an issue with libgc on
qemu-sh4-static that triggers the issue.
Adrian
> [1] https://salsa.debian.org/debian/debfoster/-/tree/debian/latest/debian?ref_type=heads
On 07/01/25 12:41, John Paul Adrian Glaubitz wrote:
> Hi Adhemerval,
>
> On Tue, 2025-01-07 at 12:10 -0300, Adhemerval Zanella Netto wrote:
>>
>> On 03/01/25 16:23, mirabilos wrote:
>>> If the bit is not 0, the operations FRCHG and FSCHG are
>>> undefined and cause a trap; qemu now checks for this as
>>> well, so we set it to 0 temporarily and restore the old
>>> value in getcontext afterwards (setcontext/swapcontext
>>> already do so).
>>>
>>> From the discussion in the bugreport, this can probably
>>> be optimised in one place but none of the people involved
>>> are SH4 assembly experts, this patch is field-tested, and
>>> it’s not a code path run often. The other question, what
>>> happens if a signal occurs while the bit is temporarily 0,
>>> is also still unsolved, but to fix that a kernel change is
>>> most likely needed; this patch changes a certain trap on
>>> many CPUs for a hard-to-get trap in a signal handler if a
>>> signal is delivered during the few instructions the PR bit
>>> is temporarily set to 0, so it’s not a regression for most
>>> users.
>>>
>>> See BZ and https://bugs.launchpad.net/qemu/+bug/1796520 for
>>> related discussion, references and review comments.
>>>
>>> Signed-off-by: mirabilos <tg@debian.org>
>>> Reviewed-by: Oleg Endo <olegendo@gcc.gnu.org>
>>> Tested-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
>>> ---
>>> sysdeps/unix/sysv/linux/sh/sh4/getcontext.S | 6 ++++++
>>> sysdeps/unix/sysv/linux/sh/sh4/setcontext.S | 2 ++
>>> sysdeps/unix/sysv/linux/sh/sh4/swapcontext.S | 2 ++
>>> 3 files changed, 10 insertions(+)
>>
>> The patch does not apply from patchwork, but I could apply it manually.
>>
>> Besides it, LGTM although I don't have a working hardware to actually
>> test it (and qemu-sh4-static shows a failure, setjmp/tst-setjmp-fp).
>
> Does the failure show before or after applying the patch?
I forgot to add that it was present before the patch, sorry about that.
>
> For me, the patch fixes the problem that programs like debfoster which
> will trigger a trap when run in an Debian unstable sh4 chroot without a
> patched glibc:
>
> (unstable-sh4-sbuild)root@adams:/# debfoster -f
> Unhandled trap: 0x180
> pc=0x2b3a807e sr=0x00000100 pr=0x2b3095da fpscr=0x00080000
> spc=0x00000000 ssr=0x00000000 gbr=0x2b2fe200 vbr=0x00000000
> sgr=0x00000000 dbr=0x00000000 delayed_pc=0x2b3a8040 fpul=0x00000000
> r0=0x2b2ab8cc r1=0x00000000 r2=0xffffb8c6 r3=0x2b2efe78
> r4=0x2b2ab7dc r5=0x2b2ab9b8 r6=0x2b34056c r7=0x00000000
> r8=0x2b35f15c r9=0x00000021 r10=0x00000000 r11=0x00001000
> r12=0x2b340250 r13=0x00000100 r14=0x2b35f158 r15=0x2b2ab70c
> r16=0x00000000 r17=0x00000000 r18=0x00000000 r19=0x00000000
> r20=0x00000000 r21=0x00000000 r22=0x00000000 r23=0x00000000
> (unstable-sh4-sbuild)root@adams:/#
>
> Since debfoster [1] relies on libgc, it might be an issue with libgc on
> qemu-sh4-static that triggers the issue.
Fair enough, I will install this.
>
> Adrian
>
>> [1] https://salsa.debian.org/debian/debfoster/-/tree/debian/latest/debian?ref_type=heads
>
@@ -67,6 +67,8 @@ ENTRY(__getcontext)
add #(oFPUL+4-124),r0
sts.l fpul, @-r0
sts.l fpscr, @-r0
+ mov #0, r6
+ lds r6, fpscr
frchg
fmov.s fr15, @-r0
fmov.s fr14, @-r0
@@ -101,6 +103,10 @@ ENTRY(__getcontext)
fmov.s fr2, @-r0
fmov.s fr1, @-r0
fmov.s fr0, @-r0
+ mov r4, r0
+ add #124, r0
+ add #(oFPSCR-124), r0
+ lds.l @r0+, fpscr
#endif /* __SH_FPU_ANY__ */
/* sigprocmask (SIG_BLOCK, NULL, &uc->uc_sigmask). */
@@ -50,6 +50,8 @@ ENTRY(__setcontext)
.Lsetcontext_restore:
#ifdef __SH_FPU_ANY__
+ mov #0, r9
+ lds r9, fpscr
mov r8, r0
add #(oFR0),r0
fmov.s @r0+, fr0
@@ -67,6 +67,8 @@ ENTRY(__swapcontext)
add #(oFPUL+4-124),r0
sts.l fpul, @-r0
sts.l fpscr, @-r0
+ mov #0, r9
+ lds r9, fpscr
frchg
fmov.s fr15, @-r0
fmov.s fr14, @-r0