This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch v6 4/5] x86* unwinder: src/


On Tue, 29 Oct 2013 14:37:02 +0100, Mark Wielaard wrote:
> > +/* libdwfl/argp-std.c */
> > +#define OPT_COREFILE    0x101
> 
> No longer used in this file.

Removed.


> > +      error (0, 0, "%s", dwfl_errmsg (-1));
> > +      return 1;
> 
> nitpick. libdw.h defines DWARF_CB_ABORT == 1,
> which is slightly nicer to use to indicate the callback is done.

Used.


> I think you should loose the space between # and the frame number if
> possible (left align it, instead of right aligning with width 2, which
> is good, as is now). Make sure all addresses are printed with the same
> number of digits.

I printed it intentionally differently but no problem with changing it,
changed it.


> And just print the adjusted pc directly by default (or
> pc if you want to match what p/gstack does). That IMHO gives the output
> users would expect (and is easier to parse).

pc_adjusted really cannot be printed, just cannot, it points nowhere, it is
incompatible with everything etc.

So far I have kept the "- 1" output there.  elfutils do not completely conform
character-by-character to any existing utility and neither does eu-stack in
other parts of the output.

I find that " - 1" part very useful for users in the output, when looking at
objdump -d etc.  And it can be safely ignored if nobody understands that.
If you object to " - 1" one could print for example " (call)" there instead.


> Keep it simple by default.

elfutils should be useful, dropping useful information to make it more close
to existing utilities does not seem correct to me.


> You could add an extra flag to print more information about a frame,
> like whether or not it is an activation, but I don't think that should
> be the default.

If there is no other choice then at least some -v flag would be needed.


>   // Try to find the address wide if possible.
>   static int wide = 0;
>   if (wide == 0)
>     if (mod)
>       {
>         Dwarf_Addr bias;
>         Elf *elf = dwfl_module_getelf (mod, &bias);
>         if (elf)
>           {
>             GElf_Ehdr ehdr_mem;
>             GElf_Ehdr *ehdr = gelf_getehdr (elf, &ehdr_mem);
>             if (ehdr)
>               wide = ehdr->e_ident[EI_CLASS] == ELFCLASS32 ? 8 : 16;
>           }
>       }
>    if (wide == 0)
>       wide = 16;

Used.  Although s/wide/width/, that was IMO a typo, wasn't it?  Plus:
-   if (wide == 0)
-     if (mod)
+   if (wide == 0 && mod)


> +    {
> +      .doc = N_("\
> +Print a stack for each thread in a process or core file.\n\
> +Only real user processes are supported, no kernel or process maps."),
> +      .children = children
> +    };
> +

Used it.


Thanks,
Jan

diff --git a/src/stack.c b/src/stack.c
index 8eebadc..3b8a908 100644
--- a/src/stack.c
+++ b/src/stack.c
@@ -27,9 +27,6 @@
 #include <fcntl.h>
 #include ELFUTILS_HEADER(dwfl)
 
-/* libdwfl/argp-std.c */
-#define OPT_COREFILE    0x101
-
 static int
 frame_callback (Dwfl_Frame *state, void *arg)
 {
@@ -39,7 +36,7 @@ frame_callback (Dwfl_Frame *state, void *arg)
   if (! dwfl_frame_pc (state, &pc, &isactivation))
     {
       error (0, 0, "%s", dwfl_errmsg (-1));
-      return 1;
+      return DWARF_CB_ABORT;
     }
   Dwarf_Addr pc_adjusted = pc - (isactivation ? 0 : 1);
 
@@ -50,7 +47,24 @@ frame_callback (Dwfl_Frame *state, void *arg)
   if (mod)
     symname = dwfl_module_addrname (mod, pc_adjusted);
 
-  printf ("#%2u %#" PRIx64 "%4s\t%s\n", (*framenop)++, (uint64_t) pc,
+  // Try to find the address wide if possible.
+  static int width = 0;
+  if (width == 0 && mod)
+    {
+      Dwarf_Addr bias;
+      Elf *elf = dwfl_module_getelf (mod, &bias);
+      if (elf)
+	{
+	  GElf_Ehdr ehdr_mem;
+	  GElf_Ehdr *ehdr = gelf_getehdr (elf, &ehdr_mem);
+	  if (ehdr)
+	    width = ehdr->e_ident[EI_CLASS] == ELFCLASS32 ? 8 : 16;
+	}
+    }
+  if (width == 0)
+    width = 16;
+
+  printf ("#%-2u 0x%0*" PRIx64 "%4s\t%s\n", (*framenop)++, width, (uint64_t) pc,
 	  ! isactivation ? "- 1" : "", symname);
   return DWARF_CB_OK;
 }
@@ -62,8 +76,8 @@ thread_callback (Dwfl_Thread *thread, void *thread_arg __attribute__ ((unused)))
   unsigned frameno = 0;
   switch (dwfl_thread_getframes (thread, frame_callback, &frameno))
     {
-    case 0:
-    case 1:
+    case DWARF_CB_OK:
+    case DWARF_CB_ABORT:
       break;
     case -1:
       error (0, 0, "dwfl_thread_getframes: %s", dwfl_errmsg (-1));
@@ -85,7 +99,20 @@ main (int argc, char **argv)
   /* Set locale.  */
   (void) setlocale (LC_ALL, "");
 
-  struct argp argp = *dwfl_standard_argp ();
+  const struct argp_child children[] =
+    {
+      { .argp = dwfl_standard_argp () },
+      { .argp = NULL },
+    };
+
+  const struct argp argp =
+    {
+      .doc = N_("\
+Print a stack for each thread in a process or core file.\n\
+Only real user processes are supported, no kernel or process maps."),
+      .children = children
+    };
+
   int remaining;
   Dwfl *dwfl = NULL;
   argp_parse (&argp, argc, argv, 0, &remaining, &dwfl);
@@ -101,7 +128,7 @@ main (int argc, char **argv)
 
   switch (dwfl_getthreads (dwfl, thread_callback, NULL))
     {
-    case 0:
+    case DWARF_CB_OK:
       break;
     case -1:
       error (0, 0, "dwfl_getthreads: %s", dwfl_errmsg (-1));

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]