[4/4] eu-stacktrace: add unwind origin diagnostics to eu-stack

Message ID 4313311c-5df8-4f78-a7ca-bccbede8a20a@app.fastmail.com
State Superseded
Headers
Series [1/4] eu-stacktrace: add eu-stacktrace tool |

Commit Message

Serhei Makarov Oct. 15, 2024, 3:26 p.m. UTC
  Since we obtain diagnostics about unwind method, another logical place
to show them is in eu-stack.

This requires an additional pseudo-unwind type DWFL_UNWOUND_INLINE to
clarify what happens with entries for inlined functions (which
eu-stacktrace does not display) based on the same Dwfl_Frame.

* libdwfl/libdwfl.h (Dwfl_Unwound_Source): Add DWFL_UNWOUND_INLINE.
* libdwfl/dwfl_frame.c (dwfl_unwound_source_str):
  Handle DWFL_UNWOUND_INLINE.
* src/stack.c (show_unwound_source): New global variable.
  (struct frame): Add unwound_source field.
  (frame_callback): Copy over unwound_source from Dwfl_Frame.
  (print_frame): Take unwound_source argument and print it.
  (print_inline_frames): Take unwound_source argument and pass it on,
  except for subsequent frames where DWFL_UNWOUND_INLINE is assumed.
  (print_frames): Take unwound_source field from struct frame and pass
  it on.
  (parse_opt): Add --cfi-type,-c option to set show_unwound_source.
  (main): Ditto.
* tests/run-stack-i-test.sh: Add testcase for --cfi-type.

Signed-off-by: Serhei Makarov <serhei@serhei.io>
---
 libdwfl/dwfl_frame.c      |  2 ++
 libdwfl/libdwfl.h         |  1 +
 src/stack.c               | 31 ++++++++++++++++++++++++-------
 tests/run-stack-i-test.sh | 15 ++++++++++++++-
 4 files changed, 41 insertions(+), 8 deletions(-)
  

Comments

Aaron Merey Oct. 17, 2024, 3:39 a.m. UTC | #1
Hi Serhei,

On Tue, Oct 15, 2024 at 11:29 AM Serhei Makarov <serhei@serhei.io> wrote:
> 
> Since we obtain diagnostics about unwind method, another logical place
> to show them is in eu-stack.
> 
> This requires an additional pseudo-unwind type DWFL_UNWOUND_INLINE to
> clarify what happens with entries for inlined functions (which
> eu-stacktrace does not display) based on the same Dwfl_Frame.
> 
> * libdwfl/libdwfl.h (Dwfl_Unwound_Source): Add DWFL_UNWOUND_INLINE.
> * libdwfl/dwfl_frame.c (dwfl_unwound_source_str):
>   Handle DWFL_UNWOUND_INLINE.
> * src/stack.c (show_unwound_source): New global variable.
>   (struct frame): Add unwound_source field.
>   (frame_callback): Copy over unwound_source from Dwfl_Frame.
>   (print_frame): Take unwound_source argument and print it.
>   (print_inline_frames): Take unwound_source argument and pass it on,
>   except for subsequent frames where DWFL_UNWOUND_INLINE is assumed.
>   (print_frames): Take unwound_source field from struct frame and pass
>   it on.
>   (parse_opt): Add --cfi-type,-c option to set show_unwound_source.
>   (main): Ditto.
> * tests/run-stack-i-test.sh: Add testcase for --cfi-type.
> 
> Signed-off-by: Serhei Makarov <serhei@serhei.io>

This patch LGTM.

Aaron
  
Mark Wielaard Oct. 17, 2024, 5 p.m. UTC | #2
Hi Serhei,

On Tue, 2024-10-15 at 11:26 -0400, Serhei Makarov wrote:
> Since we obtain diagnostics about unwind method, another logical place
> to show them is in eu-stack.
> 
> This requires an additional pseudo-unwind type DWFL_UNWOUND_INLINE to
> clarify what happens with entries for inlined functions (which
> eu-stacktrace does not display) based on the same Dwfl_Frame.
> 
> * libdwfl/libdwfl.h (Dwfl_Unwound_Source): Add DWFL_UNWOUND_INLINE.
> * libdwfl/dwfl_frame.c (dwfl_unwound_source_str):
>   Handle DWFL_UNWOUND_INLINE.

This part looks a little iffy IMHO.
It feels like a layering violation.
The meaning of DWFL_UNWOUND_INLINE is also different from the others.
There is no worst/best ordering here.
They are not connected to frames.

Is there a way to move the printing of this into stack.c?
For example make print_frame take a decorator argument that is a const
char * instead of an unwound_source argument.

Thanks,

Mark
  
Serhei Makarov Oct. 17, 2024, 5:13 p.m. UTC | #3
On Thu, Oct 17, 2024, at 1:00 PM, Mark Wielaard wrote:
> This part looks a little iffy IMHO.
> It feels like a layering violation.
> The meaning of DWFL_UNWOUND_INLINE is also different from the others.
> There is no worst/best ordering here.
> They are not connected to frames.
>
> Is there a way to move the printing of this into stack.c?
> For example make print_frame take a decorator argument that is a const
> char * instead of an unwound_source argument.

Ok, just to be clear that I understand what you want.

The unwound_source arguments (in print_frame+print_inline_frames)
changed to const char *. The print_frames() function
invokes unwound_source_str() to supply that argument.

struct frame still stores Dwfl_Unwound_Source, not char *.

In print_inline_frames the print_frame invocation is changed to
> print_frame (..., i > 1 ? "inline" : unwound_source)

DWFL_UNWOUND_INLINE removed from the enum,
making the enum a straightforward sequence of unwind methods.

I can do that.
  
Mark Wielaard Oct. 17, 2024, 5:22 p.m. UTC | #4
Hi Serhei,

On Thu, 2024-10-17 at 13:13 -0400, Serhei Makarov wrote:
> On Thu, Oct 17, 2024, at 1:00 PM, Mark Wielaard wrote:
> > This part looks a little iffy IMHO.
> > It feels like a layering violation.
> > The meaning of DWFL_UNWOUND_INLINE is also different from the others.
> > There is no worst/best ordering here.
> > They are not connected to frames.
> > 
> > Is there a way to move the printing of this into stack.c?
> > For example make print_frame take a decorator argument that is a const
> > char * instead of an unwound_source argument.
> 
> Ok, just to be clear that I understand what you want.
> 
> The unwound_source arguments (in print_frame+print_inline_frames)
> changed to const char *. The print_frames() function
> invokes unwound_source_str() to supply that argument.

I think (but haven't tried) only print_frame has to have a char *
argument, print_inline_frames and print_frames can still pass around an
Dwfl_Unwound_Source. Both print_frames and print_inline_frames can then
call print_frame with the string argument from unwound_source_str (or
the static string "inline" in the print_inline_frames case for the
"bottom" frame).

> struct frame still stores Dwfl_Unwound_Source, not char *.

Yep.

> In print_inline_frames the print_frame invocation is changed to
> > print_frame (..., i > 1 ? "inline" : unwound_source)

The second call, yes. But with "inline" :
unwound_source_str(unwound_source). The first call to print_frame would
just use unwound_source_str(unwound_source) directly.

> DWFL_UNWOUND_INLINE removed from the enum,
> making the enum a straightforward sequence of unwind methods.
> 
> I can do that.

Great.

Thanks,

Mark
  
Serhei Makarov Oct. 17, 2024, 5:27 p.m. UTC | #5
On Thu, Oct 17, 2024, at 1:22 PM, Mark Wielaard wrote:
> I think (but haven't tried) only print_frame has to have a char *
> argument, print_inline_frames and print_frames can still pass around an
> Dwfl_Unwound_Source. Both print_frames and print_inline_frames can then
> call print_frame with the string argument from unwound_source_str (or
> the static string "inline" in the print_inline_frames case for the
> "bottom" frame).
I can also do that. The asymmetry between print_frames/print_inline_frames is a bit weird.

One more needed simplification I noticed:

i > 1 ? "inline" : ...
can just be replaced with "inline"

The prior lines have a guard that skips the scope if dwarf_tag(scope) != DW_TAG_inlined_subroutine, hence anything printed through that invocation is already known to be an inlined invocation.
  

Patch

diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 2e6c6de8..3659420a 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -265,6 +265,8 @@  dwfl_unwound_source_str (Dwfl_Unwound_Source unwound_source)
       return "none";
     case DWFL_UNWOUND_INITIAL_FRAME:
       return "initial";
+    case DWFL_UNWOUND_INLINE:
+      return "inline";
     case DWFL_UNWOUND_EH_CFI:
       return "eh_frame";
     case DWFL_UNWOUND_DWARF_CFI:
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index ffd951db..21cb7d8d 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -53,6 +53,7 @@  typedef struct Dwfl_Frame Dwfl_Frame;
 typedef enum {
     DWFL_UNWOUND_NONE = 0,
     DWFL_UNWOUND_INITIAL_FRAME,
+    DWFL_UNWOUND_INLINE, /* XXX used for src/stack.c print_inline_frames() */
     DWFL_UNWOUND_EH_CFI,
     DWFL_UNWOUND_DWARF_CFI,
     DWFL_UNWOUND_EBL,
diff --git a/src/stack.c b/src/stack.c
index eb87f5f1..baee9cae 100644
--- a/src/stack.c
+++ b/src/stack.c
@@ -1,5 +1,5 @@ 
 /* Unwinding of frames like gstack/pstack.
-   Copyright (C) 2013-2014 Red Hat, Inc.
+   Copyright (C) 2013-2014, 2024 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -44,6 +44,7 @@  ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
 static bool show_activation = false;
 static bool show_module = false;
 static bool show_build_id = false;
+static bool show_unwound_source = false;
 static bool show_source = false;
 static bool show_one_tid = false;
 static bool show_quiet = false;
@@ -58,6 +59,7 @@  struct frame
 {
   Dwarf_Addr pc;
   bool isactivation;
+  Dwfl_Unwound_Source unwound_source;
 };
 
 struct frames
@@ -180,6 +182,7 @@  frame_callback (Dwfl_Frame *state, void *arg)
 {
   struct frames *frames = (struct frames *) arg;
   int nr = frames->frames;
+  frames->frame[nr].unwound_source = dwfl_frame_unwound_source (state);
   if (! dwfl_frame_pc (state, &frames->frame[nr].pc,
 		       &frames->frame[nr].isactivation))
     return -1;
@@ -221,7 +224,7 @@  static void
 print_frame (int nr, Dwarf_Addr pc, bool isactivation,
 	     Dwarf_Addr pc_adjusted, Dwfl_Module *mod,
 	     const char *symname, Dwarf_Die *cudie,
-	     Dwarf_Die *die)
+	     Dwarf_Die *die, Dwfl_Unwound_Source unwound_source)
 {
   int width = get_addr_width (mod);
   printf ("#%-2u 0x%0*" PRIx64, nr, width, (uint64_t) pc);
@@ -271,6 +274,11 @@  print_frame (int nr, Dwarf_Addr pc, bool isactivation,
 	}
     }
 
+  if (show_unwound_source)
+    {
+      printf (" [%s]", dwfl_unwound_source_str (unwound_source));
+    }
+
   if (show_source)
     {
       int line, col;
@@ -323,7 +331,8 @@  print_frame (int nr, Dwarf_Addr pc, bool isactivation,
 static void
 print_inline_frames (int *nr, Dwarf_Addr pc, bool isactivation,
 		     Dwarf_Addr pc_adjusted, Dwfl_Module *mod,
-		     const char *symname, Dwarf_Die *cudie, Dwarf_Die *die)
+		     const char *symname, Dwarf_Die *cudie, Dwarf_Die *die,
+		     Dwfl_Unwound_Source unwound_source)
 {
   Dwarf_Die *scopes = NULL;
   int nscopes = dwarf_getscopes_die (die, &scopes);
@@ -333,7 +342,7 @@  print_inline_frames (int *nr, Dwarf_Addr pc, bool isactivation,
 	 the name.  This is the actual source location where it
 	 happened.  */
       print_frame ((*nr)++, pc, isactivation, pc_adjusted, mod, symname,
-		   NULL, NULL);
+		   NULL, NULL, unwound_source);
 
       /* last_scope is the source location where the next frame/function
 	 call was done. */
@@ -349,7 +358,8 @@  print_inline_frames (int *nr, Dwarf_Addr pc, bool isactivation,
 
 	  symname = die_name (scope);
 	  print_frame ((*nr)++, pc, isactivation, pc_adjusted, mod, symname,
-		       cudie, last_scope);
+		       cudie, last_scope,
+		       i > 1 ? DWFL_UNWOUND_INLINE : unwound_source);
 
 	  /* Found the "top-level" in which everything was inlined?  */
 	  if (tag == DW_TAG_subprogram)
@@ -375,6 +385,7 @@  print_frames (struct frames *frames, pid_t tid, int dwflerr, const char *what)
       Dwarf_Addr pc = frames->frame[nr].pc;
       bool isactivation = frames->frame[nr].isactivation;
       Dwarf_Addr pc_adjusted = pc - (isactivation ? 0 : 1);
+      Dwfl_Unwound_Source unwound_source = frames->frame[nr].unwound_source;
 
       /* Get PC->SYMNAME.  */
       Dwfl_Module *mod = dwfl_addrmodule (dwfl, pc_adjusted);
@@ -417,10 +428,10 @@  print_frames (struct frames *frames, pid_t tid, int dwflerr, const char *what)
 
       if (show_inlines && die != NULL)
 	print_inline_frames (&frame_nr, pc, isactivation, pc_adjusted, mod,
-			     symname, cudie, die);
+			     symname, cudie, die, unwound_source);
       else
 	print_frame (frame_nr++, pc, isactivation, pc_adjusted, mod, symname,
-		     NULL, NULL);
+		     NULL, NULL, unwound_source);
     }
 
   if (frames->frames > 0 && frame_nr == maxframes)
@@ -535,6 +546,10 @@  parse_opt (int key, char *arg __attribute__ ((unused)),
       show_build_id = true;
       break;
 
+    case 'c':
+      show_unwound_source = true;
+      break;
+
     case 'q':
       show_quiet = true;
       break;
@@ -676,6 +691,8 @@  main (int argc, char **argv)
 	N_("Show raw function symbol names, do not try to demangle names"), 0 },
       { "build-id",  'b', NULL, 0,
 	N_("Show module build-id, load address and pc offset"), 0 },
+      { "cfi-type", 'c', NULL, 0,
+	N_("Show the backtrace method for each frame (eh_frame, dwarf, inline, or ebl)"), 0 },
       { NULL, '1', NULL, 0,
 	N_("Show the backtrace of only one thread"), 0 },
       { NULL, 'n', "MAXFRAMES", 0,
diff --git a/tests/run-stack-i-test.sh b/tests/run-stack-i-test.sh
index 3722ab09..bc46d9d5 100755
--- a/tests/run-stack-i-test.sh
+++ b/tests/run-stack-i-test.sh
@@ -1,5 +1,5 @@ 
 #! /bin/sh
-# Copyright (C) 2014, 2015 Red Hat, Inc.
+# Copyright (C) 2014, 2015, 2024 Red Hat, Inc.
 # This file is part of elfutils.
 #
 # This file is free software; you can redistribute it and/or modify
@@ -73,6 +73,19 @@  TID 13654:
 $STACKCMD: tid 13654: shown max number of frames (6, use -n 0 for unlimited)
 EOF
 
+# With --cfi-type we also see what unwind method was used for each frame:
+testrun_compare ${abs_top_builddir}/src/stack -r -n 6 -c -i -e testfiledwarfinlines --core testfiledwarfinlines.core<<EOF
+PID 13654 - core
+TID 13654:
+#0  0x00000000004006c8 fubar [initial]
+#1  0x00000000004006c8 foobar [inline]
+#2  0x00000000004006c8 bar [inline]
+#3  0x00000000004006c8 foo [inline]
+#4  0x00000000004006c8 _Z2fui [inline]
+#5  0x00000000004004c5 main [eh_frame]
+$STACKCMD: tid 13654: shown max number of frames (6, use -n 0 for unlimited)
+EOF
+
 if [ "x$SAVED_VALGRIND_CMD" != "x" ]; then
   VALGRIND_CMD="$SAVED_VALGRIND_CMD"
   export VALGRIND_CMD