Re: [PATCH] 0001-Fix-multi-thread-debug-bug-in-AIX.patch

[Ulrich Weigand <Ulrich.Weigand@de.ibm.com>]

Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:

>Yes, it resolves the issue.

Excellent.  A few final comment on the patch, including one
change I hadn't noticed before:

>@@ -508,14 +552,17 @@ pdc_read_data (pthdb_user_t user_current_pid, void *buf,
>   /* This is needed to eliminate the dependency of current thread
>      which is null so that thread reads the correct target memory.  */
>   {
>-    scoped_restore_current_thread restore_current_thread;
>+    scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
>     /* Before the first inferior is added, we pass inferior_ptid.pid ()
>        from pd_enable () which is 0.  There is no need to switch threads
>        during first initialisation.  In the rest of the callbacks the
>        current thread needs to be correct.  */
This comment is no longer relevant as the code relating to it was
deleted.  The comment should be deleted as well.
>-    if (user_current_pid != 0)
>-      switch_to_thread (current_inferior ()->process_target (),
>-			ptid_t (user_current_pid));
>+    inferior_ptid = ptid_t (user_current_pid);
>+    scoped_restore_current_inferior restore_inferior;
>+    set_current_inferior (inf);
>+
>+    scoped_restore_current_program_space restore_current_progspace;
>+    set_current_program_space (inf->pspace);
>     status = target_read_memory (addr, (gdb_byte *) buf, len);
>   }
>   ret = status == 0 ? PDC_SUCCESS : PDC_FAILURE;

 
>+  tp = find_thread_ptid (proc_target, ptid_t (pid));
>+
>+  /* If the pthreadlibrary is not ready to debug 
>+     then this is just a main process which needs 
>+     a priv to be set.  The if condition below does 
>+     the same.  Otherwise we go to the for loop to 
>+     sync the pthread and GDB thread lists.  */
>+
>   /* Apply differences between the two arrays to GDB's thread list.  */
>+
>   for (pi = gi = 0; pi < pcount || gi < gcount;)

These changes all seem to be leftovers from previous attempts,
I guess they should be removed again.

>+  inferior *inf = current_inferior (); 
>+  /* When attaching / handling fork child, don't try loading libthread_db
>+     until we know about all shared libraries.  */
>+  if (inf->in_initial_library_scan)
>+    return;

"libthread_db" is Linux specific.  Please update the comment so
it makes sense in the AIX context.
 
>@@ -1362,12 +1439,16 @@ aix_thread_target::fetch_registers (struct regcache *regcache, int regno)
> {
>   struct thread_info *thread;
>   pthdb_tid_t tid;
>+  thread = find_thread_ptid (current_inferior ()->process_target (), ptid_t (regcache->ptid ().pid (), 0, regcache->ptid ().tid ()));
> 
>-  if (!PD_TID (regcache->ptid ()))
>+  /* If a new inferior is born, then its pthread debug library is yet to
>+     initialised and hence has no private data. So the below if condition
>+     exists.  */
>+
>+  if (regcache->ptid ().tid () == 0)
>     beneath ()->fetch_registers (regcache, regno);
>   else
>     {
>-      thread = find_thread_ptid (current_inferior (), regcache->ptid ());
>       aix_thread_info *priv = get_aix_thread_info (thread);
>       tid = priv->tid;

I hadn't seen this change below, it doesn't really make sense to me.
You really need to use regcache->ptid here, this should be correct.
When did you see a case where this was not correct?   Does this still
happen now that we have the in_initial_library_scan check?

Bye,
Ulrich