This is the mail archive of the
mailing list for the elfutils project.
Re: [patch 4/4 v2] unwinder: The unwinder (x86* only)
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Sat, 01 Jun 2013 20:30:26 +0200
- Subject: Re: [patch 4/4 v2] unwinder: The unwinder (x86* only)
On Fri, 31 May 2013 23:28:14 +0200, Mark Wielaard wrote:
> I have some nits about
> what I think are "reverse dependencies" between libebl and libdwfl.
I also did not like it much but I do not know about a better solution (without
making the code worse than it is).
> you have been very thorough.
The CFI operators set is not complete, there is no CFI code deadlock
prevention etc. but those parts were not on the critical path.
> > +/* This holds everything we know about the state of the frame at a particular
> > + PC location described by an FDE. */
> > +typedef struct Dwfl_Frame_State Dwfl_Frame_State;
> This is also for a particular thread. I found that at first a little
> confusing. That you can "jump" from thread to thread while unwinding. It
> might be nice/cleaner to make the thread concept and how to get the
> initial frame state for a thread a bit more explicit.
I made some API I am fine with. If you see it better a different way express
the API you want, please. I do not mind much, I just chose something I saw as
the most simple one.
> > +/* Get innermost frame of first thread of live process PID. Returns NULL on
> > + failure. */
> > +extern Dwfl_Frame_State *dwfl_frame_state_pid (Dwfl *dwfl, pid_t pid);
> This has the side effect, on success, of stopping the process. Which is
> good of course, or we wouldn't be able to get consistent unwinds. But it
> isn't clear how to "unstop" the process again.
> IMHO it would be nicer to have an explicit Process concept, so that we can
> start observing a process (which would freeze it) and end observing
> a process (which would set it free again).
During the current redesign of the API I will do not see a place for Process,
now Dwfl is used intead of Process. It does not make sense to have multiple
processes linked to single Dwfl as each process has different memory map
> > +/* Get innermost frame of first thread of core file COREFILE. Returns NULL on
> > + failure. */
> > +extern Dwfl_Frame_State *dwfl_frame_state_core (Dwfl *dwfl,
> > + const char *corefile);
> The user might have the *Elf core still around. Which brings up the
> point that the Dwfl pid or core file really should match with the given
> corefile or pid when the Dwfl was populated by dwfl_linux_proc_report or
> dwfl_core_file_report. We could record that (pid or core *Elf) and just
> do the right thing without the user possibly messing things up with the
> wrong pid or core file.
OK, application can open *Elf core on its own.
> > +/* Fetch inferior registers from a caller supplied storage. */
> > +typedef bool dwfl_frame_memory_read_t (Dwarf_Addr addr, Dwarf_Addr *result,
> > + void *user_data);
> > +extern Dwfl_Frame_State *dwfl_frame_state_data (Dwfl *dwfl, bool pc_set,
> > + Dwarf_Addr pc, unsigned nregs,
> > + const uint64_t *regs_set,
> > + const Dwarf_Addr *regs,
> > + dwfl_frame_memory_read_t
> > + *memory_read,
> > + void *memory_read_user_data);
> So this is the generic variant.
Not completely, you can emulate neither dwfl_frame_state_core () nor
dwfl_frame_state_pid () on top of it. This is because dwfl_frame_state_data's
produced Dwfl_Frame_State will always have only one thread.
Supporting multiple threads using dwfl_frame_state_data means just calling it
for every TID separately.
> pc_set might be redundant since the backend should know
> whether PC is part of the regs or not.
I find it as a sanity check to verify application vs. backend agree on whether
PC_SET is needed or not.
> Since we have a memory_read callback it makes sense to me to also have
> an (initial) frame register set callback
According to the design "one dwfl_frame_state_data call per each TID" I find
a registers callback superfluous. Such callback will be called exactly once
at the beginning and never more; that only brings additional code complexity
with no benefits.
> That callback would then be called whenever you switch to the next
> thread of a process to get the initial frame.
One cannot, as described above.
> Using the DWARF register
> numbering here seems a smart choice since that is the best documented.
Unfortunately it becames a hell due to PowerPC sparse DWARF numbering; but
that is waiting for us in the non-x86* extensions in futher patches.
> > +/* Return TRUE and update *STATEP for the unwound frame for successful unwind.
> > + Return TRUE and set *STATEP to NULL for the outermost frame. Return FALSE
> > + (and call __libdwfl_seterrno) otherwise. */
> > +extern bool dwfl_frame_unwind (Dwfl_Frame_State **statep);
> Another way to express this would be with a callback that walks over the
> frames as long as DWARF_CB_OK is returned. Like what dwfl_getmodules ()
dwfl_getmodules supports both iterative calls via OFFSET and also a callback,
TBH I do not find why, it seems overcomplicated to me. Callbacks make sense
when the iteration requires some additional context but neither
dwfl_getmodules nor dwfl_frame_unwind need any.
I can provide the callback if you still ask for it although I would prefer to
know the advantages; disadvantages are clear I hope, it requires another
function in the caller and passing context between caller and the callback.
> > +/* Get return address register value for frame. Return TRUE if *PC set and
> > + optionally *MINUSONE is also set, if MINUSONE is not NULL. Return FALSE
> > + (and call __libdw_seterrno) otherwise. *MINUSONE is TRUE for normal calls
> > + where *PC should be decremented by one to get the call instruction, it is
> > + FALSE if this frame was interrupted by a signal handler. */
> > +extern bool dwfl_frame_state_pc (Dwfl_Frame_State *state, Dwarf_Addr *pc,
> > + bool *minusone);
> I wish there was a better word for MINUSONE. Maybe RETURNADDRESS? To
> indicate that the PC is the return address (and to get to call address
> of this frame you need to substract at least one)?
As I find the rules where MINUSONE is set relatively complicated
( libdwfl/dwfl_frame_state_pc.c ) I find all the other namings confusing.
The name RETURNADDRESS is for example weird why to be set for the bottom frame.
> We should think about how we would like to expose the other registers of
> the frame. Though that doesn't have priority. Just walking the PCs is
> what is most interesting now.
Exactly, other registers were not on the critical path.
Is the API provided below OK?
/* This holds everything we know about state of a frame at a particular thread
at a particular PC location described by an FDE. */
typedef struct Dwfl_Frame_State Dwfl_Frame_State;
/* Attach (and stop) all threads of PID. PID will be detached by callind
dwfl_end on returned DWFL. Returns NULL on failure. */
extern Dwfl *dwfl_begin_unwind_pid (pid_t pid);
/* Initialize Dwfl * from an already opened core file CORE. Caller has to
close CORE after calling dwfl_end. Returns NULL on failure. */
extern Dwfl *dwfl_begin_unwind_core (Elf *core);
/* Initialize unwinder for DWFL from a caller supplied storage. Returns
innermost frame or NULL on failure. This function does not support
multiple threads, call it for each thread of the same process (DWFL). */
typedef bool dwfl_frame_memory_read_t (Dwarf_Addr addr, Dwarf_Addr *result,
extern Dwfl_Frame_State *dwfl_begin_unwind (Dwfl *dwfl, bool pc_set,
Dwarf_Addr pc, unsigned nregs,
const Dwarf_Word *regs_set,
const Dwarf_Addr *regs,
/* Return TRUE and update *STATEP for the unwound frame for successful unwind.
Return TRUE and set *STATEP to NULL for the outermost frame. Return FALSE
(and set dwfl_errno) otherwise. */
extern bool dwfl_frame_unwind (Dwfl_Frame_State **statep);
/* Get return address register value for frame. Return TRUE if *PC set and
optionally *MINUSONE is also set, if MINUSONE is not NULL. Return FALSE
(and set dwfl_errno) otherwise. *MINUSONE is TRUE for normal calls where
*PC should be decremented by one to get the call instruction, it is FALSE
if this frame was interrupted by a signal handler. */
extern bool dwfl_frame_state_pc (Dwfl_Frame_State *state, Dwarf_Addr *pc,
/* Get innermost frame of the next thread from DWFL. Return also Task ID of
the returned thread in *TID_RETURN. TID_RETURN can be NULL. Returns NULL
if there are no more threads in DWFL (in such case *TID_RETURN is left
extern Dwfl_Frame_State *dwfl_get_next_thread (Dwfl *dwfl, pid_t *tid_return);