This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [patch v6 4/5] x86* unwinder: src/
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Tue, 29 Oct 2013 14:37:02 +0100
- Subject: 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