This is the mail archive of the mailing list for the elfutils project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

Hi Jan,

Sorry, I thought I sent feedback last week, but apparently I never
finished that email.

On Sat, 2013-06-01 at 20:30 +0200, Jan Kratochvil wrote: 
> 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.

I don't know if what I am suggesting is better, but it is slightly
different, just so it is a little more generic and matches the existing
interface a bit more. I just want to make sure we understand it
completely before committing to it, so we don't have to break something
in the future.

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

Good points. Thanks. Identifying Dwfl with Process is much simpler than
trying to introduce a new Process concept.

The only issue I have with dwfl_end to mark the process "unfrozen" again
is that it means you have to reconstruct the whole Dwfl again for the
next backtrace. Something which is a bit expensive if you like to use
the unwinder for something like a profiler that is periodically taking
backtraces of a process it is monitoring. I think we must convince
ourselves that we can introduce something a bit more subtle that
preserves the Dwfl in the future for that use case. But we can go with
the "freeze at begin", "unfreeze at end" policy for now.

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

Yeah, this was really the clue that Dwfl == Process.
But I missed it till you pointed it out. Thanks.

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

I think that is a bit of a bug in the design/interface though. If we
introduce a register callback that is called when inspecting a different
thread the user can (in theory) emulate either a core, pid or data
backed Dwfl. And the other frame/state function would just work
similarly without the user having to adjust code depending on how things
were initialized.

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

If there is no data/registers for other threads then the callback can
just say so, and the users will just see one thread.

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

Sigh, I forgot that PowerPC uses "sparse" DWARF numbering (and even a
little ambiguous). Still, it might be the best we have. It might just
not be the most efficient way to pass things through for the ppc
backend. Having a register callback might help a little with that
though. Then the callback can report the registers in known ranges.

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

The main advantage is that it is consistent with other such iterators in
libdw/libdwfl. And consistency in the interface is somewhat nice to
have. The context would be the Dwfl_Frame_State, so you don't have to
hold on that yourself and the lifetime of the frame state is clear, just
during that callback (though that is admittedly a somewhat minor
advantage, maybe not offset by the more complicated use case).

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

No worries, I'll try and hack something up. If it is rubbish we can just
revert it again. But it will give me some experience with the new code
for review. And it will make things more concrete than my current "hand

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

Yes, you are right that it is relatively complicated. And RETURNADDRESS
is also not really the correct way to express the concept. When
unwinding we always get the address that is being returned to. But it
might not be the address that the call was made from or the frame was
interrupted in (what the DWARF spec call the activation address). When
the previous frame was an asynchronous interrupt (signal) then the
return address is in the actual context of the current frame. Otherwise
the return address might be outside the actual context of the current
frame (and in that case we have to substract at least one to get there,
but not otherwise). I don't like the name MINUSONE since it is somewhat
architecture specific (it isn't exactly 1, and on some architectures you
might know the instruction length and get an exact answer) and doesn't
express why you might want to substract 1. Maybe ISACTIVATION might be
more descriptive (which would be the inverse of MINUSONE, it shows the
return pc is equal to the frame "call" address)?

Sorry for discussing the naming at length. We do know technically what
it represents. Just hope to find something a bit intuitive. Maybe that
is just impossible with such a technical subject.

Let me just hack up an example of the interface as proposed above to
make it more concrete (or to show me why it shouldn't/cannot be done).



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]