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

[Mark Wielaard <mjw at redhat dot com>]

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