Re: [patch v5] unwinder: The unwinder (x86* only)

[Mark Wielaard <mjw at redhat dot com>]

On Wed, 2013-10-09 at 18:48 +0200, Jan Kratochvil wrote: 
> On Tue, 08 Oct 2013 16:43:02 +0200, Mark Wielaard wrote:
> > I don't really like that ebl/backends now depends on dwfl.
> [...]
> > This is needed because the new ebl_set_initial_registers_tid function is
> > defined as:
> 
> It was not really needed as it was dealing with Dwfl stuff only in opaque
> style.

I think it also shouldn't depend on the types. That leads to things like
having to include libebl.h in libdwfl.h. That was one of the reasons I
suggested using something like Ebl_Register_Location. It doesn't have to
be that solution, it can also be done through the current setup with a
callback, but IMHO that callback should not be libdwfl type specific. We
really want to keep libebl completely internal, so defining types there
that get exposed through the user is IMHO a no-no.

(As a side note, dwfl_attach_state in libdwfl.h is defined to take an
Ebl * now, but the user doesn't have, and shouldn't have, a way to
create one.)

I can try to rewrite it a bit so that the types aren't intertwined
either. Just make a ebl_state_registers_t callback based on a given
generic void *arg instead of a Dwfl_Thread *. Only the libdwfl code
should deal with the specific Dwfl types.

> > The advantage would be that things are a little bit more flexible for
> > the caller and that more code could be shared between things that
> > process registers in core notes and those that grab them from a process.
> 
> I do not see more PID/core sharing, struct user has different layout than core
> files so that would be needed a new Ebl_Register_Location struct to describe
> struct user -> DWARF numbering translation.

Yes, it would need a new instance of to describe they layout, but since
you already parse the Register_Location contents in the core case you
could then reuse that part.

> But even then you cannot make PTRACE_GETREGS arch-independent as it is done
> (almost) on every arch differently.  Sometimes PTRACE_GETREGS, sometimes
> PTRACE_PEEKUSER (older ppc), sometimes PTRACE_PEEKUSR_AREA (s390), sometimes
> with swapped addr<->data parameters for PTRACE_GETREGS (sparc) etc.

Yes, but all eventually give you a memory area that can be described
through an Ebl_Register_Location list, doesn't it? The getting of the
register area and the description will not be arch-independent, but the
users of it can use it just like they would use a core note with a
similar interface/Register_Location description.

But now that I spelled it out I do realize it is a lot more work than
what is currently done by just using the user_regs_struct for sys/user.h
and setting the array values directly. It does mean that the backend
functions are native-arch only. Which might not be a problem in this
specific case. But in theory it would be nice to be able to support
"remote" fetching of registers (no idea how that would be implemented
though, so please ignore).

Just one concern. How stable is sys/user.h and user_regs_struct? Can we
build on an older system and expect the header and struct to be usable
on a newer system?

> So sending the patch without libebl/backends dependcy on libdwfl

When we untangle the libebl and libdwfl parts we should make our
reviewing lives a little easier and split that 120K patch into separate
parts. I would suggest something like: ebl part, x86 backend part,
libdwfl (and minimal libdw) part, src stack part, tests. I find it hard
to review 120K in one go. And if we have separate parts we can agree on
that part being ok in itself, even if they will get committed together.

Thanks,

Mark