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

[Mark Wielaard <mjw at redhat dot com>]

On Thu, 2013-05-30 at 15:26 +0200, Jan Kratochvil wrote:
> made a new branch:
> 	jankratochvil/unwindx86
> 
> It is a single commit with all its pre-requisites already checked in.

It is fairly large :) I read through most of it. I have some nits about
what I think are "reverse dependencies" between libebl and libdwfl. But
I think the general framework for unwinding is all here. Nice job, you
have been very thorough. But before going into details of the code lets
first concentrate on the new public interfaces, since if we get those
wrong we are stuck with them, while we can fix bugs in the
implementation more easily. Also it will help me review things more
concretely by going "down" from the public entry points.

> diff --git a/libdw/libdw.map b/libdw/libdw.map
> index d38a8ef..54ee7f2 100644
> --- a/libdw/libdw.map
> +++ b/libdw/libdw.map
> @@ -259,4 +259,11 @@ ELFUTILS_0.156 {
>    global:
>      # Replaced ELFUTILS_0.122 version, which has a wrapper without add_p_vaddr.
>      dwfl_report_elf;
> +    dwfl_frame_state_pid;
> +    dwfl_frame_state_core;
> +    dwfl_frame_state_data;
> +    dwfl_frame_unwind;
> +    dwfl_frame_state_pc;
> +    dwfl_frame_tid_get;
> +    dwfl_frame_thread_next;
>  } ELFUTILS_0.149;
> [...]
> diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
> index 2b70e28..8998886 100644
> --- a/libdwfl/libdwfl.h
> +++ b/libdwfl/libdwfl.h
> @@ -41,6 +41,10 @@ typedef struct Dwfl_Module Dwfl_Module;
>  /* Handle describing a line record.  */
>  typedef struct Dwfl_Line Dwfl_Line;
>  
> +/* 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.

> @@ -564,6 +568,46 @@ extern int dwfl_module_register_names (Dwfl_Module *mod,
>  extern Dwarf_CFI *dwfl_module_dwarf_cfi (Dwfl_Module *mod, Dwarf_Addr *bias);
>  extern Dwarf_CFI *dwfl_module_eh_cfi (Dwfl_Module *mod, Dwarf_Addr *bias);
>  
> +/* 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).

And internally you already use the Process concept.

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

> +/* 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. It could use a bit more documentation.
But I get the idea. Expressing the regs as Dwarf_Words instead of
uint64_t might be more in line with existing usage in the libdwfl
interface. pc_set might be redundant since the backend should know
whether PC is part of the regs or not.

Since we have a memory_read callback it makes sense to me to also have
an (initial) frame register set callback (and maybe expose
dwfl_frame_state_reg_set to make the callback as flexible as possible).
That callback would then be called whenever you switch to the next
thread of a process to get the initial frame. Using the DWARF register
numbering here seems a smart choice since that is the best documented.

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

> +/* 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)?

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.

> +/* Get innermost frame of the next thread from STATE.  STATE can be any frame
> +   of (the previous) thread.  */
> +extern Dwfl_Frame_State *dwfl_frame_thread_next (Dwfl_Frame_State *state);

This I think should be an operation on the Process (calling the initial
register callback), to get the next threads initial frame (starting with
null for the first one). Returning at the same time the tid. So you
don't need the next function.

> +/* Get Task ID of the thread of STATE.  This is PID for the thread started by
> +   function main and gettid () for the other threads.  */
> +extern pid_t dwfl_frame_tid_get (Dwfl_Frame_State *state);

I like to try and rewrite this patch a little to have a public interface
that includes a begin/end_dwfl_process() with which you can explicitly
set the callbacks for memory and initial registers as outlined above. It
might show that is a silly idea, but at least it will force me to go
through the code for a full review.

Cheers,

Mark