This is the mail archive of the elfutils-devel@sourceware.org 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 v7 3/5] x86* unwinder: libdwfl/


On Fri, 2013-10-25 at 14:50 +0200, Jan Kratochvil wrote:
> On Fri, 25 Oct 2013 11:44:57 +0200, Mark Wielaard wrote:
> > On Wed, 2013-10-23 at 15:57 +0200, Jan Kratochvil wrote:
> > > 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 (;;)
> > >      {
> 
> It should be rather here.  thread.callbacks_args is specific for each thread
> and it would be a mistake to rely on the same value across different threads.

No, it is specific for the callback, which might or might not set it
differently for each thread.

> > So also the first time the next_thread callback is called
> > thread.callbacks_arg has a defined (NULL) value.
> 
> I was looking that it was uninitialized already in the first post (different
> API) and it was intentional.  Documentation does not guarantee NULL and if the
> application does not use thread.callbacks_arg at all it does not have to be
> touched.

If the documentation doesn't guarantee it now, then it should IMHO.

> But I see now it should be initialized due to false warnings by Valgrind.

No, that is not the reason. If we wouldn't guarantee it is set then if
some code access it anyway before setting itself then valgrind would be
right to warn about that.

What I want is that the thread_argp can be used as iterator state for
the callback. That way the state doesn't (all) have to be held in the
dwfl_arg (like is currently done for both the pid and core backends).
NULL would be the initial value passed in, after that the callback gets
to set it (possibly different for each thread). So what I would like is
something like the following:

diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index ee20afd..af584fb 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -231,15 +231,13 @@ dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread *thread, void *arg),
       __libdwfl_seterrno (DWFL_E_NO_ATTACH_STATE);
       return -1;
     }
+
+  Dwfl_Thread thread;
+  thread.process = process;
+  thread.unwound = NULL;
+  thread.callbacks_arg = NULL;
   for (;;)
     {
-      Dwfl_Thread thread;
-      thread.process = process;
-      thread.unwound = NULL;
-      /* Nobody guarantees NULL but tools like valgrind could warn on accessing
-	 uninitialized data if the application does not set it (as the
-	 application does not use it).  */
-      thread.callbacks_arg = NULL;
       thread.tid = process->callbacks->next_thread (dwfl,
 						    process->callbacks_arg,
 						    &thread.callbacks_arg);
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index 83cabb8..46a3906 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -582,10 +582,12 @@ typedef struct
   /* Called to iterate through threads.  Returns next TID (thread ID) on
      success, a negative number on failure and zero if there are no more
      threads.  dwfl_errno () should be set if negative number has been
-     returned.  *THREAD_ARGP may be optionally set by the implementation,
-     THREAD_ARGP is never NULL.  *THREAD_ARGP will be passed to
-     set_initial_registers or thread_detach callbacks together with Dwfl_Thread
-     *thread.  This method must not be NULL.  */ 
+     returned.  *THREAD_ARGP is NULL on first call, and may be optionally
+     set by the implementation. The value set by the implementation will
+     be passed in on the next call to NEXT_THREAD.  THREAD_ARGP is never
+     NULL.  *THREAD_ARGP will be passed to set_initial_registers or
+     thread_detach callbacks together with Dwfl_Thread *thread.  This
+     method must not be NULL.  */
   pid_t (*next_thread) (Dwfl *dwfl, void *dwfl_arg, void **thread_argp)
     __nonnull_attribute__ (1);
 



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