Re: [patch 4/4 v2] unwinder: The unwinder (x86* only)

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi Mark,

On Thu, 13 Jun 2013 00:03:21 +0200, Mark Wielaard wrote:
> - Need to include sys/types.h before sys/user.h on older systems.
>   On RHEL6 x86_64 at least.

OK.

> - GCC version check attributes, noclone was introduced with GCC 4.5

I would prefer "NORETURN", at least sourceware tree uses:
	src/include/ansidecl.h:#define ATTRIBUTE_NORETURN __attribute__ ((__noreturn__))


>   That is for the testcase. But I might have misunderstood. I never
>   could make the selfdump tests work since it tries to find the
>   "jmp" symbol (which seems to never be used?).

make check will not PASS for run-backtrace.sh?  It PASSes for me.

$ nm tests/backtrace-child|grep -w jmp
0000000000000acb t jmp

Tested on Fedora 18 x86_64 and wit mjw/unwind now also on CentOS 6.3 x86_64.


> - tests/backtrace.c: Drop out of dwfl_getmodules when done.
>   Just to show that you can return from a callback early and don't have
>   to iterate over all modules once you found your match.

I do not agree with this part, one of secondary goals was to verify exactly
one of the modules patches the filename.  Multiple modules of the same name
will not be caught by your change.


> - src/stack.c: Print usage also when number of arguments wrong.
>   The abort was a little too strong IMHO when someone (me) made a typo.

OK.


> Then I thought I could just write up the rest of the interface with
> extra callbacks for dwfl_begin when you want state (threads, registers
> and memory).

It should be enough to support registers + memory callback.  registers are
fetched only initially for a new thread so they in fact define a thread.

The current unwinder codebase does not implement 'lazy registers'.  Currently
every unwound frame contains everything what we could unwind.  For x86* there
are so few registers I guess the overhead of lazy-fetching may be higher than
the saved cycles.  Whether in reality it is worth it or not I have no idea.

I moved lazy registers feature into future incremental improvements although
they would affect API.  Still with the symbols versioning I do not think it is
such a problem, we could be improving initial commit indefinitely to have
every future extension in the initial API definition.


> So now I am thinking we should do "the opposite". Only while starting an
> iteration over the frames to unwind for one thread should we ptrace
> attach, and after we are done with unwinding for that thread we should
> detach it again. That would make things much lighter weight.
> 
> What do you think? I am struggling a bit with this because I can see the
> user might want both (full freeze on begin or minimal thread freeze at
> use only).

Besides dwfl_end (which automatically detaches all attached TIDs) there could
be also
	dwfl_detach_thread (Dwfl_Frame_State *state);
although it would be thread-specific, not frame-specific, so you would like
more:
	typedef struct Dwfl_Thread Dwfl_Thread;
	dwfl_detach_thread (Dwfl_Thread *thread);
So with some iterator across threads one can detach each thread after
unwinding it.


Thanks,
Jan