Re: [PATCH 3/5] libdwfl: add interface for attaching to/detaching from threads

[Omar Sandoval <osandov@osandov.com>]

On Thu, Oct 31, 2019 at 05:21:17PM +0100, Mark Wielaard wrote:
> On Wed, 2019-10-30 at 16:49 -0700, Omar Sandoval wrote:
> > On Wed, Oct 30, 2019 at 01:47:52PM +0100, Mark Wielaard wrote:
> > > Also, if we would adopt dwfl_attach_thread then I think it should take
> > > a Dwfl_Thread object not a pid/tid as argument.
> > 
> > In that case, we'd probably want to expose the internal getthread
> > function with something like:
> > 
> > /* Find the thread with the given thread ID.  Returns zero if the
> >    thread was processed, -1 on error (including when no thread with the
> >    given thread ID exists), or the return value of the callback when not
> >    DWARF_CB_OK.  */
> > int dwfl_getthread (Dwfl *dwfl, pid_t tid,
> > 		    int (*callback) (Dwfl_Thread *thread, void *arg),
> > 		    void *arg)
> >   __nonnull_attribute__ (1, 3);
> > 
> > Then dwfl_attach_thread could be used with either dwfl_getthread or
> > dwfl_getthreads, which is nice. However, a crucial part of this
> > feature
> > is being able to access the Dwfl_Thread outside of the callback. 
> >
> > Since
> > the Dwfl_Thread is currently on the stack, I see a couple of options:
> > 
> > 1. We keep Dwfl_Thread on the stack and make dwfl_attach_thread()
> > return
> >    a copy (like it does in this patch).
> > 2. We always allocate the Dwfl_Thread on the heap and free it before
> >    returning from dwfl_getthread(s) _unless_ dwfl_attach_thread() was
> >    called. If it was, it will be freed by dwfl_detach_thread()
> > instead.
> > 
> > Option 2 sounds better to me. What do you think?
> 
> To be honest I am not sure I like either.
> 
> I think it is mainly because this isn't really about
> attaching/detaching from a thread but stopping/resuming it. That is
> part of what set_initial_registers does. So what
> dwfl_attach_thread/dwfl_detach_thread really do is setting up the
> Dwfl_Thread so it can be queried/used.
> 
> You are attached/detached on the process as a whole. Which is done with
> the dwfl_linux_proc_attach call.
> 
> So basically I think what we want is an interface which you call
> pauzing the thread.
> 
> Sorry for not having a clear vision of the perfect interface yet.

No problem, I'm sure we'll be able to come up with something better :)

I was hesitant to call this dwfl_stop_thread/dwfl_resume_thread because
you aren't necessarily stopping/resuming anything, for example when
you're working with a core dump. Plus, the detach terminology is
consistent with Dwfl_Thread_Callbacks::thread_detach.

But, stop/resume is more descriptive. Does that sound better to you? The
semantics could be that a Dwfl_Thread is valid after dwfl_getthread(s)
returns if and only if it is currently paused. The Dwfl_Thread is freed
on dwfl_resume_thread() or when dwfl_getthread(s) returns, whichever
comes last.

Thanks,
Omar