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: Wed, 30 Oct 2013 15:47:06 +0100
- Subject: Re: [patch v6 4/5] x86* unwinder: src/
Hi Jan,
On Wed, 2013-10-30 at 14:21 +0100, Jan Kratochvil wrote:
> > 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.
Well, it is the pc value we use to lookup the symbol. Which might or
might not be the exact "call address". But yes, I do see your point.
Using pc is better because it is a good known (return) address.
> 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.
Now that addresses are all of the same length I don't find it as
distracting, since it is always put in the same place. Lets keep it.
> > 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.
I do want as much information as we can give. But IMHO the default
should be as basic as possible. Lets do introduce a -v flag (or some
others for symbol name demangling, module names, source/line, function
arguments, etc.). But not right now. Lets first get the basic
functionality in. I am very eager to finally see this committed!
> Used. Although s/wide/width/, that was IMO a typo, wasn't it?
Yeah, my English is not always as good as it should be...
One tiny formatting nitpick (sorry):
> + printf ("#%-2u 0x%0*" PRIx64 "%4s\t%s\n", (*framenop)++, width, (uint64_t) pc,
> ! isactivation ? "- 1" : "", symname);
Could we replace the \t with a simple space?
Now that the output lines are all of equal width a space is enough IMHO.
With or without that change I think it is good to go in.
Thanks,
Mark