Message ID | alpine.DEB.2.21.2001240032350.7536@digraph.polyomino.org.uk |
---|---|
State | New |
Headers | show |
Series | Build raise with -fasynchronous-unwind-tables | expand |
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
* 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
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
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.
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; +} +
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