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

[Jan Kratochvil]

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.


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

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.

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


Thanks,
Jan

diff --git a/libdwfl/dwfl_frame.c b/libdwfl/dwfl_frame.c
index 0806678..ee20afd 100644
--- a/libdwfl/dwfl_frame.c
+++ b/libdwfl/dwfl_frame.c
@@ -231,12 +231,15 @@ 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;
   for (;;)
     {
-      assert (thread.unwound == NULL);
+      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);
@@ -259,6 +262,7 @@ dwfl_getthreads (Dwfl *dwfl, int (*callback) (Dwfl_Thread 
*thread, void *arg),
          thread_free_all_states (&thread);
          return err;
        }
+      assert (thread.unwound == NULL);
     }
   /* NOTREACHED */
 }