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

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

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.

dwfl_end


> 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
layout.


> > +/* 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 ()
> does.

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?


Thanks,
Jan


/* 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,
				       void *user_data);
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,
					    dwfl_frame_memory_read_t *
								    memory_read,
					    void *memory_read_user_data);

/* 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,
				 bool *minusone);

/* 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
   uninitialized.  */
extern Dwfl_Frame_State *dwfl_get_next_thread (Dwfl *dwfl, pid_t *tid_return);