Re: [patch v7 3/5] x86* unwinder: libdwfl/

[Mark Wielaard <mjw at redhat dot com>]

On Wed, 2013-10-23 at 15:57 +0200, Jan Kratochvil wrote:
> On Wed, 23 Oct 2013 14:18:10 +0200, Mark Wielaard wrote:
> > On Tue, 2013-10-22 at 22:32 +0200, Jan Kratochvil wrote:
> > > > Maybe you want threads to be iterated through callbacks?
> > > 
> > > Implemented it in the attached diff and checked into jankratochvil/unwindx86.
> > 
> > I like it. It seems to simplify the code.
> 
> That it makes the unwinder suited only for "tracers" but not for debuggers.
> The original goal was that you can stop the inferior and now you can traverse
> across threads and across frames.  With the current design you cannot.
> 
> OTOH for core files - ABRT - it does not matter (cores do not move).
> For perf it is questionable, it should minimally affect the inferior,
> OTOH currently the Dwfl virtual address map is generated when still the
> inferior is running so some of the backtraces may fail.  But still it is
> probably more suitable for perf the current non-stopping way.  And one can
> guess there will never be a real interactive debugger written a top of
> elfutils unwinder.

Yeah, I admit the current design is very tailored to doing just a "quick
none-intrusive backtrace" and nothing more. But since that is all we
need at the moment it feels better to just concentrate on that. If we
later add, maybe not an interactive debugger, but some simple automated
"debugging" support, we can look at adding extra interfaces to
explicitly freeze/attach and unfreeze/detach a whole process before
doing dwfl_reports. And new interfaces to make it possible to access any
thread at any time during the freeze. But none of that is IMHO needed
for the current use case. And it would only complicate matters right
now. I don't think we have something that prevents adding such new
interfaces in the future.

> > thread_free doesn't set dwfl_errno, so should be save to call without
> > saving/restoring it.
> 
> But nobody guarantees thread_free cannot modify errno in future.  Such bugs
> are difficult to catch, in fact bugs in the error paths are never caught.
> Do you still insist on removing saved_errno?

No, it is fine as is. Just like you said, doing this fully consistent in
all error paths would be hard.

> >   pid_t (*next_thread) (Dwfl *dwfl, void *dwfl_arg, void **thread_argp)
> >     __nonnull_attribute__ (1, 2);
> 
> Here should be only __nonnull_attribute__ (1) now.

Right, sorry missed that.

> diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
> @@ -256,34 +226,35 @@ dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
> [...]
> +  Dwfl_Thread thread;
> +  thread.process = process;
> +  thread.unwound = NULL;

And  thread.callbacks_args = NULL;

>    for (;;)
>      {
> -      nthread->tid = process->callbacks->next_thread (dwfl, nthread,
> -						      process->callbacks_arg,
> -						      &nthread->callbacks_arg);
> -      if (nthread->tid < 0)
> +      assert (thread.unwound == NULL);
> +      thread.tid = process->callbacks->next_thread (dwfl,
> +						    process->callbacks_arg,
> +						    &thread.callbacks_arg);

So also the first time the next_thread callback is called
thread.callbacks_arg has a defined (NULL) value.

Looks good with that change.

Thanks,

Mark