Re: [PATCH] libgcc: Add a backchain fallback to _Unwind_Backtrace() on PowerPC
Hi Segher, thank you for the review, I'll work on the issues you pointed.
On 13/08/2021 13:04, Segher Boessenkool wrote:
Hi!
On Fri, Aug 13, 2021 at 10:46:37AM -0300, Raphael Moreira Zinsly wrote:
* config/rs6000/linux-unwind.h (struct rt_sigframe): Move it to
outside of get_regs() in order to use it in another function.
Say you do this twice, once for __powerpc64__, once for !__powerpc64__?
(struct trace_arg): New struct.
(struct layout): New struct.
(ppc_backchain_fallback): New function.
* unwind.inc (_Unwind_Backtrace): Look for _URC_NORMAL_STOP
code state and call MD_BACKCHAIN_FALLBACK.
+struct trace_arg
+{
+ void **array;
+ struct unwind_link *unwind_link;
+ _Unwind_Word cfa;
+ int cnt;
+ int size;
+};
Did you get this definition from elsewhere? If not, please spell out
"count". Also, count of what? This needs a comment, just like "array"
and "size".
I got this from glibc, I agree that spelling out "count" should be
better, I'll do that and add comments.
+/* This is the stack layout we see with every stack frame.
+ Note that every routine is required by the ABI to lay out the stack
+ like this.
+
+ +++-+
+%r1 -> | %r1 last frame> | %r1 last frame--->... --> NULL
+ ||| |
+ | cr save|| cr save |
+ ||| |
+ | (unused) || return address |
+ +++-+
+*/
"last frame" is an unfortunate choice of words... "previous frame"?
And "r1 previous frame" is nonsensical, r1 by definition points at the
_current_ frame. Just "previous frame" maybe?
Mention near the figure that the CR save is for the 64-bit ABIs only.
but the backchain and the LR save are there always?
>> +struct layout
+{
+ struct layout *next;
+#ifdef __powerpc64__
+ long int condition_register;
+#endif
+ void *return_address;
+};
Call it "cr_save" perhaps, like ABIs do? And "lr_save" maybe? And
"backchain"? The name "layout" could be better, too... "frame_layout"
maybe? "layout" is just too generic >
+void ppc_backchain_fallback (struct _Unwind_Context *context, void *a)
(Why "ppc"? Why not "power" or "powerpc" or "rs6000"?)
I am following the style of "ppc_fallback_frame_state".
+{
+ struct layout *current;
+ struct trace_arg *arg = a;
+ int count;
+
+ /* Get the last address computed and start with the next. */
+ current = context->cfa;
+ current = current->next;
+
+ for (count = arg->cnt; current != NULL && count < arg->size;
+ current = current->next, count++)
Either write all three expressions in a "for" statement on one line, or
each on its own line? You may want to have only one variable in here
("current" probably), update count in the loop body, for cleaner, easier
to read code.
+{
+ arg->array[count] = current->return_address;
+
+ /* Check if the symbol is the signal trampoline and get the interrupted
+ * symbol address from the trampoline saved area. */
No leading * in comments.
+ context->ra = current->return_address;
+ if (current->return_address != NULL && get_regs(context) != NULL)
You can just write
if (current->return_address && get_regs (context))
fwiw (note the space before the "(").
+ {
+ struct rt_sigframe *sigframe = (struct rt_sigframe*) current;
Space before *.
+ if (count + 1 == arg->size)
+ break;
+ arg->array[++count] = (void *) sigframe->uc.rsave.nip;
+ current = (void *) sigframe->uc.rsave.gpr[1];
No two spaces.
This needs a testcase (that can run on all subtargets).
Segher
Thanks,
--
Raphael Moreira Zinsly
Re: [PATCH v2] libgcc: Add a backchain fallback to _Unwind_Backtrace() on PowerPC
Ping
On 26/08/2021 11:53, Raphael Moreira Zinsly wrote:
Without dwarf2 unwind tables available _Unwind_Backtrace() is not
able to return the full backtrace.
This patch adds a fallback function on powerpc to get the backtrace
by doing a backchain, this code was originally at glibc.
libgcc/ChangeLog:
* config/rs6000/linux-unwind.h (struct rt_sigframe): Move it to
outside of get_regs() in order to use it in another function,
this is done twice: for __powerpc64__ and for !__powerpc64__.
(struct trace_arg): New struct.
(struct layout): New struct.
(ppc_backchain_fallback): New function.
* unwind.inc (_Unwind_Backtrace): Look for _URC_NORMAL_STOP
code state and call MD_BACKCHAIN_FALLBACK.
gcc/testsuite/ChangeLog:
* gcc.target/powerpc/unwind-backchain.c: New test.
---
.../gcc.target/powerpc/unwind-backchain.c | 22
libgcc/config/rs6000/linux-unwind.h | 102 +++---
libgcc/unwind.inc | 14 ++-
3 files changed, 122 insertions(+), 16 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/powerpc/unwind-backchain.c
diff --git a/gcc/testsuite/gcc.target/powerpc/unwind-backchain.c
b/gcc/testsuite/gcc.target/powerpc/unwind-backchain.c
new file mode 100644
index 000..fdce78a1f63
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/unwind-backchain.c
@@ -0,0 +1,22 @@
+/* { dg-do run { target { powerpc*-*-linux* } } } */
+/* { dg-options "-fno-asynchronous-unwind-tables" } */
+
+#include
+
+void
+test_backtrace()
+{
+ int addresses;
+ void *buffer[10];
+
+ addresses = backtrace(buffer, 10);
+ if(addresses != 4)
+__builtin_abort();
+}
+
+int
+main()
+{
+ test_backtrace();
+ return 0;
+}
diff --git a/libgcc/config/rs6000/linux-unwind.h
b/libgcc/config/rs6000/linux-unwind.h
index acdc948f85d..8deccc1d650 100644
--- a/libgcc/config/rs6000/linux-unwind.h
+++ b/libgcc/config/rs6000/linux-unwind.h
@@ -94,6 +94,15 @@ struct gcc_ucontext
enum { SIGNAL_FRAMESIZE = 128 };
+struct rt_sigframe {
+ char gap[SIGNAL_FRAMESIZE];
+ struct gcc_ucontext uc;
+ unsigned long pad[2];
+ int tramp[6];
+ void *pinfo;
+ struct gcc_ucontext *puc;
+};
+
/* If PC is at a sigreturn trampoline, return a pointer to the
regs. Otherwise return NULL. */
@@ -136,14 +145,7 @@ get_regs (struct _Unwind_Context *context)
#endif
{
/* This works for 2.4.21 and later kernels. */
- struct rt_sigframe {
- char gap[SIGNAL_FRAMESIZE];
- struct gcc_ucontext uc;
- unsigned long pad[2];
- int tramp[6];
- void *pinfo;
- struct gcc_ucontext *puc;
- } *frame = (struct rt_sigframe *) context->cfa;
+ struct rt_sigframe *frame = (struct rt_sigframe *) context->cfa;
return frame->uc.regs;
}
}
@@ -154,6 +156,12 @@ get_regs (struct _Unwind_Context *context)
enum { SIGNAL_FRAMESIZE = 64 };
+struct rt_sigframe {
+ char gap[SIGNAL_FRAMESIZE + 16];
+ char siginfo[128];
+ struct gcc_ucontext uc;
+};
+
static struct gcc_regs *
get_regs (struct _Unwind_Context *context)
{
@@ -176,11 +184,7 @@ get_regs (struct _Unwind_Context *context)
}
else if (pc[0] == 0x3800 || pc[0] == 0x38AC)
{
- struct rt_sigframe {
- char gap[SIGNAL_FRAMESIZE + 16];
- char siginfo[128];
- struct gcc_ucontext uc;
- } *frame = (struct rt_sigframe *) context->cfa;
+ struct rt_sigframe *frame = (struct rt_sigframe *) context->cfa;
return frame->uc.regs;
}
return NULL;
@@ -203,7 +207,7 @@ ppc_fallback_frame_state (struct _Unwind_Context *context,
int i;
if (regs == NULL)
-return _URC_END_OF_STACK;
+return _URC_NORMAL_STOP;
new_cfa = regs->gpr[__LIBGCC_STACK_POINTER_REGNUM__];
fs->regs.cfa_how = CFA_REG_OFFSET;
@@ -352,3 +356,73 @@ frob_update_context (struct _Unwind_Context *context,
_Unwind_FrameState *fs ATT
}
#endif
}
+
+#define MD_BACKCHAIN_FALLBACK ppc_backchain_fallback
+
+struct trace_arg
+{
+ /* Stores the list of addresses. */
+ void **array;
+ struct unwind_link *unwind_link;
+ _Unwind_Word cfa;
+ /* Number of addresses currently stored. */
+ int count;
+ /* Maximum number of addresses. */
+ int size;
+};
+
+/* This is the stack layout we see with every stack frame.
+ Note that every routine is required by the ABI to lay out the stack
+ like this.
+
+ +++-+
+%r1 -> | previous frame> | previous frame--->... --> NULL
+ ||| |
+ | cr save|| cr save |
+ ||| |
+ | (unused) || lr save |
+ +++-+
+
+ The CR save is only present on 64-bit ABIs.
+*/
+struct frame_layout
+{
+
Re: [PATCH v2] libgcc: Add a backchain fallback to _Unwind_Backtrace() on PowerPC
On 28/09/2021 16:50, Segher Boessenkool wrote:
Hi!
On Thu, Aug 26, 2021 at 11:53:24AM -0300, Raphael Moreira Zinsly wrote:
Without dwarf2 unwind tables available _Unwind_Backtrace() is not
able to return the full backtrace.
This patch adds a fallback function on powerpc to get the backtrace
by doing a backchain, this code was originally at glibc.
Okay, the backchain as fallback if other (better!) methods cannot work.
* config/rs6000/linux-unwind.h (struct rt_sigframe): Move it to
outside of get_regs() in order to use it in another function,
this is done twice: for __powerpc64__ and for !__powerpc64__.
(struct trace_arg): New struct.
(struct layout): New struct.
(ppc_backchain_fallback): New function.
* unwind.inc (_Unwind_Backtrace): Look for _URC_NORMAL_STOP
code state and call MD_BACKCHAIN_FALLBACK.
Changelog lines wrap at 80 chars, not 70 or so. The emails from commits
(to bugzilla) are a bit malformed (it counts the number of columns for
leading tabs wrong it seems), but the actual commits are just fine.
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/unwind-backchain.c
@@ -0,0 +1,22 @@
+/* { dg-do run { target { powerpc*-*-linux* } } } */
Don't say such targets in gcc.target/powerpc/ tests please. Everything
in gcc.target is for powerpc*-*-* already, so if you really want to
limit to powerpc*-*-linux* just write *-*-linux*. But there are better
ways to get what you want, like, testing for the actual feature you want
(which is if backtrace() works?) But such an improvement can be done
later (and needs more testing etc).
Ok, I'll change to *-*-linux*.
I used powerpc*-*-linux* because most of the tests in this directory do
that way.
But please write some simple comment saying why you need -linux* in the
test.
+void
+test_backtrace()
+{
+ int addresses;
+ void *buffer[10];
+
+ addresses = backtrace(buffer, 10);
+ if(addresses != 4)
+__builtin_abort();
+}
Does that work?! Has this been tested on all powerpc*-linux configs?
Importantly also BE and 32-bit.
I haven't tested on BE, will do.
Okay for trunk with the testcase fix, if all testing works out. Thanks!
Segher
Thanks for the review,
--
Raphael Moreira Zinsly
Re: [PATCH] libgcc: fix backtrace fallback on PowerPC Big-endian. [PR103004]
Hi Segher,
On 11/11/2021 10:43, Segher Boessenkool wrote:
Hi!
On Wed, Nov 10, 2021 at 06:59:23PM -0300, Raphael Moreira Zinsly wrote:
At the end of the backtrace stream _Unwind_Find_FDE() may not be able
to find the frame unwind info and will later call the backtrace fallback
instead of finishing. This occurs when using an old libc on ppc64 due to
dl_iterate_phdr() not being able to set the fde in the last trace.
When this occurs the cfa of the trace will be behind of context's cfa.
Also, libgo’s probestackmaps() calls the backtrace with a null pointer
and can get to the backchain fallback with the same problem, in this case
we are only interested in find a stack map, we don't need nor can do a
backchain.
_Unwind_ForcedUnwind_Phase2() can hit the same issue as it uses
uw_frame_state_for(), so we need to treat _URC_NORMAL_STOP.
libgcc/ChangeLog:
* config/rs6000/linux-unwind.h (ppc_backchain_fallback): turn into
static to fix -Wmissing-prototypes. Check if it's called with a null
argument or at the end of the backtrace and return.
* unwind.inc (_Unwind_ForcedUnwind_Phase2): treat _URC_NORMAL_STOP.
Formatting is messed up. Lines start with a capital. Two spaces after
full stop, while you're at it.
Ok.
-void ppc_backchain_fallback (struct _Unwind_Context *context, void *a)
+static void
+ppc_backchain_fallback (struct _Unwind_Context *context, void *a)
This was already fixed in 75ef0353a2d3.
Ops, missed that.
{
struct frame_layout *current;
struct trace_arg *arg = a;
int count;
- /* Get the last address computed and start with the next. */
+ /* Get the last address computed. */
current = context->cfa;
Empty line after here please. Most of the time if you have a full-line
comment it means a new paragraph is starting.
Ok.
+ /* If the trace CFA is not the context CFA the backtrace is done. */
+ if (arg == NULL || arg->cfa != current)
+ return;
+
+ /* Start with next address. */
current = current->backchain;
Like you did here :-)
Do you have a testcase (that failed without this, but now doesn't)?
I don't have a simple testcase for that, but many of the asan and go
tests catch that.
Looks okay, but please update and resend.
Segher
Thanks,
--
Raphael Moreira Zinsly
Re: [PATCH v3] libgcc: Add a backchain fallback to _Unwind_Backtrace() on PowerPC
On 05/10/2021 19:03, Segher Boessenkool wrote: On Tue, Oct 05, 2021 at 03:32:52PM -0300, Raphael Moreira Zinsly wrote: Without dwarf2 unwind tables available _Unwind_Backtrace() is not able to return the full backtrace. This patch adds a fallback function on powerpc to get the backtrace by doing a backchain, this code was originally at glibc. libgcc/ChangeLog: * config/rs6000/linux-unwind.h (struct rt_sigframe): Move it to outside of get_regs() in order to use it in another function, this is done twice: for __powerpc64__ and for !__powerpc64__. (struct trace_arg): New struct. (struct layout): New struct. (ppc_backchain_fallback): New function. * unwind.inc (_Unwind_Backtrace): Look for _URC_NORMAL_STOP code state and call MD_BACKCHAIN_FALLBACK. gcc/testsuite/ChangeLog: * gcc.target/powerpc/unwind-backchain.c: New test. --- .../gcc.target/powerpc/unwind-backchain.c | 24 + libgcc/config/rs6000/linux-unwind.h | 102 +++--- libgcc/unwind.inc | 14 ++- 3 files changed, 124 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/gcc.target/powerpc/unwind-backchain.c So what is new or different in v3 compared to v2? gcc/testsuite/gcc.target/powerpc/unwind-backchain.c: - Added a comment explaining why test only on *-linux targets. - Changed the dejagnu target from powerpc*-*-linux* to *-*-linux*. -- Raphael Moreira Zinsly
