Build raise with -fasynchronous-unwind-tables

Build raise with -fasynchronous-unwind-tables

[Joseph Myers]

In testing glibc for Arm and MIPS, I see:

FAIL: misc/tst-sigcontext-get_pc

If this test - backtracing through a call to raise - is valid, then
raise needs to be built with -fasynchronous-unwind-tables (as the test
itself is) to have the required unwind information for that
backtracing to work.  Adding that option, which this patch does,
causes the test for pass for Arm.  For MIPS, the test still does not
pass (the backtrace has an address that is 2 bytes after the "address
in signal handler", for unknown reasons), although the patch allows a
longer backtrace to be produced.

diff --git a/signal/Makefile b/signal/Makefile
index 7da67def84..37de438bba 100644
--- a/signal/Makefile
+++ b/signal/Makefile
@@ -52,6 +52,7 @@ tests		:= tst-signal tst-sigset tst-sigsimple tst-raise tst-sigset2 \
 
 include ../Rules
 
+CFLAGS-raise.c += -fasynchronous-unwind-tables
 CFLAGS-sigpause.c += -fexceptions
 CFLAGS-sigsuspend.c += -fexceptions -fasynchronous-unwind-tables
 CFLAGS-sigtimedwait.c += -fexceptions -fasynchronous-unwind-tables

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Build raise with -fasynchronous-unwind-tables

[Siddhesh Poyarekar]

On 24/01/20 6:03 am, Joseph Myers wrote:
> In testing glibc for Arm and MIPS, I see:
> 
> FAIL: misc/tst-sigcontext-get_pc
> 
> If this test - backtracing through a call to raise - is valid, then
> raise needs to be built with -fasynchronous-unwind-tables (as the test
> itself is) to have the required unwind information for that
> backtracing to work.  Adding that option, which this patch does,
> causes the test for pass for Arm.  For MIPS, the test still does not
> pass (the backtrace has an address that is 2 bytes after the "address
> in signal handler", for unknown reasons), although the patch allows a
> longer backtrace to be produced.
> 

The fix seems fine, but I wonder how the test passes on other
architectures.  Also the commit that introduced this test seems to
indicate that the test was passing earlier:

commit a43565ac447b1608ae2626f5012673560bb623ab
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date:   Mon Dec 17 16:44:14 2018 -0200

    Refactor sigcontextinfo.h

Siddhesh

Re: Build raise with -fasynchronous-unwind-tables

[Florian Weimer]

* Siddhesh Poyarekar:

> On 24/01/20 6:03 am, Joseph Myers wrote:
>> In testing glibc for Arm and MIPS, I see:
>> 
>> FAIL: misc/tst-sigcontext-get_pc
>> 
>> If this test - backtracing through a call to raise - is valid, then
>> raise needs to be built with -fasynchronous-unwind-tables (as the test
>> itself is) to have the required unwind information for that
>> backtracing to work.  Adding that option, which this patch does,
>> causes the test for pass for Arm.  For MIPS, the test still does not
>> pass (the backtrace has an address that is 2 bytes after the "address
>> in signal handler", for unknown reasons), although the patch allows a
>> longer backtrace to be produced.
>> 
>
> The fix seems fine, but I wonder how the test passes on other
> architectures.

Many architectures default to -fasynchronous-unwind-tables in GCC.

The patch seems reasonable to me.

Thanks,
Florian

Re: Build raise with -fasynchronous-unwind-tables

[Siddhesh Poyarekar]

On 24/01/20 12:05 pm, Florian Weimer wrote:
>> The fix seems fine, but I wonder how the test passes on other
>> architectures.
> 
> Many architectures default to -fasynchronous-unwind-tables in GCC.
> 
> The patch seems reasonable to me.

Thanks, looks good to me for master then.

Siddhesh

Re: Build raise with -fasynchronous-unwind-tables

[Joseph Myers]

On Fri, 24 Jan 2020, Joseph Myers wrote:

> causes the test for pass for Arm.  For MIPS, the test still does not
> pass (the backtrace has an address that is 2 bytes after the "address
> in signal handler", for unknown reasons), although the patch allows a
> longer backtrace to be produced.

I now understand the MIPS issue: libgcc/config/mips/linux-unwind.h adds 2 
to the return address for a signal frame.

The question then is how best to fix that MIPS issue.  Adding 2 in the 
MIPS sigcontext_get_pc would fix things for the testcase, and be safe for 
the use in debug/segfault.c.  I'm less sure, however, what's safe for the 
use of sigcontext_get_pc in sysdeps/unix/sysv/linux/profil-counter.h - 
what exactly that use requires regarding the return value of 
sigcontext_get_pc.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Build raise with -fasynchronous-unwind-tables

[Adhemerval Zanella]

On 24/01/2020 23:18, Joseph Myers wrote:
> On Fri, 24 Jan 2020, Joseph Myers wrote:
> 
>> causes the test for pass for Arm.  For MIPS, the test still does not
>> pass (the backtrace has an address that is 2 bytes after the "address
>> in signal handler", for unknown reasons), although the patch allows a
>> longer backtrace to be produced.
> 
> I now understand the MIPS issue: libgcc/config/mips/linux-unwind.h adds 2 
> to the return address for a signal frame.
> 
> The question then is how best to fix that MIPS issue.  Adding 2 in the 
> MIPS sigcontext_get_pc would fix things for the testcase, and be safe for 
> the use in debug/segfault.c.  I'm less sure, however, what's safe for the 
> use of sigcontext_get_pc in sysdeps/unix/sysv/linux/profil-counter.h - 
> what exactly that use requires regarding the return value of 
> sigcontext_get_pc.
> 

My plan to refactor this code and add sigcontext_get_pc is that the 
current proposal to fix BZ#12683 requires the SIGCANCEL handler to obtain
where exactly the syscall was interrupted to decide whether or not to act
on cancellation request. The tst-sigcontext-get_pc idea is to check if
kernel SA_SIGINFO returns the expected interrupted instruction using a 
different mechanism (libgcc).

And my understanding is what really need to be fixed is the backtrace 
result on MIPS, since the expected correct value of the interrupted 
instruction by a signal frame is the one set by the kernel in the 
create signal frame. 

Changing the value of sigcontext_get_pc to take in consideration the
libgcc added value might be ok for profil-counter.h but it definitely
wrong for BZ#12683 fix (it will mostly likely create cancellation request
with false-positive side-effects).

Below a proposed fix:

--

[PATCH] mips: Fix bracktrace result for signal frames

The MIPS fallback code to handle a frame where its FDE can not be obtained
(for instance a signal frame) is to read the kernel allocated signal frame
and add 2 to the value of 'sc_pc' [1].  The added value is used to
recognize an end of an EH region on mips16 [2].

For default MIPS encoding we can mask off the 2 least significant bit to
get the faulting instruction.  However, on mips16/micromips there is no
easy way to recognize when libgcc adds this extra offset.

Checked with backtrace and tst-sigcontext-get_pc tests on mips-linux-gnu
and mips64-linux-gnu.

[1] libgcc/config/mips/linux-unwind.h from gcc code.
[2] gcc/config/mips/mips.h from gcc code.  */
---
 debug/backtrace.c                          |  3 +-
 sysdeps/generic/unwind-arch.h              | 30 +++++++++++++++
 sysdeps/unix/sysv/linux/mips/unwind-arch.h | 45 ++++++++++++++++++++++
 3 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/generic/unwind-arch.h
 create mode 100644 sysdeps/unix/sysv/linux/mips/unwind-arch.h

diff --git a/debug/backtrace.c b/debug/backtrace.c
index cc4b9a5c90..957558fd88 100644
--- a/debug/backtrace.c
+++ b/debug/backtrace.c
@@ -23,6 +23,7 @@
 #include <gnu/lib-names.h>
 #include <stdlib.h>
 #include <unwind.h>
+#include <unwind-arch.h>
 
 struct trace_arg
 {
@@ -77,7 +78,7 @@ backtrace_helper (struct _Unwind_Context *ctx, void *a)
      Skip it.  */
   if (arg->cnt != -1)
     {
-      arg->array[arg->cnt] = (void *) unwind_getip (ctx);
+      arg->array[arg->cnt] = unwind_getip_masked (unwind_getip (ctx));
 
       /* Check whether we make any progress.  */
       _Unwind_Word cfa = unwind_getcfa (ctx);
diff --git a/sysdeps/generic/unwind-arch.h b/sysdeps/generic/unwind-arch.h
new file mode 100644
index 0000000000..d1030c368a
--- /dev/null
+++ b/sysdeps/generic/unwind-arch.h
@@ -0,0 +1,30 @@
+/* Return backtrace of current program state.  Arch-specific bits.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef _UNWIND_ARCH_H
+#define _UNWIND_ARCH_H
+
+#include <unwind.h>
+
+static inline void *
+unwind_getip_masked (_Unwind_Ptr ip)
+{
+  return (void*) ip;
+}
+
+#endif
diff --git a/sysdeps/unix/sysv/linux/mips/unwind-arch.h b/sysdeps/unix/sysv/linux/mips/unwind-arch.h
new file mode 100644
index 0000000000..0f5a1a83a4
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/mips/unwind-arch.h
@@ -0,0 +1,45 @@
+/* Return backtrace of current program state.  Arch-specific bits.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef _UNWIND_ARCH_H
+#define _UNWIND_ARCH_H
+
+#include <unwind.h>
+
+/* The MIPS fallback code to handle a frame where its FDE can not be obtained
+   (for instance a signal frame) is to read the kernel allocated signal frame
+   and add 2 to the value of 'sc_pc' [1].  The added value is used to
+   recognize an end of an EH region on mips16 [2].
+
+   For default MIPS encoding we can mask off the 2 least significant bit to
+   get the faulting instruction.  However, on mips16/micromips there is no
+   easy way to recognize when libgcc adds this extra offset.
+
+   [1] libgcc/config/mips/linux-unwind.h from gcc code.
+   [2] gcc/config/mips/mips.h from gcc code.  */
+
+static inline void *
+unwind_getip_masked (_Unwind_Ptr ip)
+{
+#ifndef __mips16
+  ip &= -4;
+#endif
+  return (void*) ip;
+}
+