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