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 Fri, 2013-10-11 at 22:47 +0200, Jan Kratochvil wrote:
> src/
> 2013-06-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	    Mark Wielaard  <mjw@redhat.com>
> 
> 	* Makefile.am (bin_PROGRAMS): Add stack.
> 	(stack_LDADD): New.
> 	* stack.c: New file.
> 
> Signed-off-by: Jan Kratochvil <jan.kratochvil@redhat.com>

I took the variant on your jankratochvil/unwindx86 branch for this
review.

> --- /dev/null
> +++ b/src/stack.c
> @@ -0,0 +1,128 @@
> [...]
> +
> +/* libdwfl/argp-std.c */
> +#define OPT_COREFILE    0x101

No longer used in this file.

> +static int
> +frame_callback (Dwfl_Frame *state, void *arg)
> +{
> +  unsigned *framenop = arg;
> +  Dwarf_Addr pc;
> +  bool isactivation;
> +  if (! dwfl_frame_pc (state, &pc, &isactivation))
> +    {
> +      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.

> +    }
> +  Dwarf_Addr pc_adjusted = pc - (isactivation ? 0 : 1);
> +
> +  /* Get PC->SYMNAME.  */
> +  Dwfl *dwfl = dwfl_thread_dwfl (dwfl_frame_thread (state));
> +  Dwfl_Module *mod = dwfl_addrmodule (dwfl, pc_adjusted);
> +  const char *symname = NULL;
> +  if (mod)
> +    symname = dwfl_module_addrname (mod, pc_adjusted);
> +
> +  printf ("#%2u %#" PRIx64 "%4s\t%s\n", (*framenop)++, (uint64_t) pc,
> +	  ! isactivation ? "- 1" : "", symname);
> +  return DWARF_CB_OK;
> +}

This gives out put like:

# 0 0x3597eac7ae    	__waitpid
# 1 0x43eb72 - 1	waitchld
[...]

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. 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). Keep it simple by default.
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. So just something like:

  printf ("#%-2u 0x%0*" PRIx64 "\t%s\n", (*framenop)++, 16
          (uint64_t) pc_adjusted, symname);

Which would give:

TID 18801:
#0  0x0000003597eac7ae	__waitpid
#1  0x000000000043eb71	waitchld

Or maybe check the first module to determine whether it is 32 or 64
bits, so you don't get too many zeros for 32-bits. e.g.

  // 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;

In a later version we could add some flags to output more info per frame
or module. But lets not in the initial version.

> +  struct argp argp = *dwfl_standard_argp ();
> +  int remaining;
> +  Dwfl *dwfl = NULL;
> +  argp_parse (&argp, argc, argv, 0, &remaining, &dwfl);

It makes sense to use the standard argp parser, but it would be good to
make sure the help does document what it does and why some options
aren't supported in this case. I would propose something like the
following to make the --help output nicer:

diff --git a/src/stack.c b/src/stack.c
index 8eebadc..146dd24 100644
--- a/src/stack.c
+++ b/src/stack.c
@@ -85,7 +85,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);

The -k, -K, and -M options already seem to give good error messages when
used. It would be nice to hide them fully, but I don't know if that can
be easily done.

I like this program, it is simple and still very useful.

Thanks,

Mark


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