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

[Ulrich Weigand via Gdb-patches <gdb-patches@sourceware.org>]

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

>>I think the question here is simply whether, if you run the
>>test suite both without and with your patch, are any of the
>>FAILs fixed with the patch?   If not, it would be good to
>>create a new test that fails without the patch and succeeds
>>with it, and add that to the test suite.
>
>So, this is something new to me. We will add it as a continuation in the same thread
>after this patch. I will need one information. Which test suite will we add it in?
>gdb.threads or gdb.base? Also, kindly suggest a simple test case that is written that
>I can see and learn. Any simple hello_world program will do. I want to understand how
>that exp file is written and how it compares to tell if a test case is pass or fail.  

I think this would fit better into gdb.threads, given that this is about the
interaction of multiple inferiors with the threading library on AIX.

I'd just look at existing test cases in that directory.  For simple tests, we
usually have a .c file and a .exp file with the same name.  The .exp file
starts out with instructions to build the test case, and start it up under
GDB.  Then follow a series of test statements which are verified against
the output of the GDB under test.  As a simple example in a related area,
you can look e.g. at fork-child-threads.{c,exp}.


>Kindly give me feedback for this patch, incase we can do anything better
>or is incorrect.

Some comments:

>@@ -508,14 +550,13 @@ 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.  */
>     if (user_current_pid != 0)
>-      switch_to_thread (current_inferior ()->process_target (),
>-			ptid_t (user_current_pid));
>+      inferior_ptid = ptid_t (user_current_pid);
>     status = target_read_memory (addr, (gdb_byte *) buf, len);
>   }

This seems unrelated to the rest of the changes at first glance.
Why is this necessary?

Also, is the "user_current_pid != 0" check even still needed given
the change to pd_enable() below?

By comparison, the Linux version of this in proc-service.c also
switches the current inferior and address space:

  scoped_restore_current_inferior restore_inferior;
  set_current_inferior (ph->thread->inf);

  scoped_restore_current_program_space restore_current_progspace;
  set_current_program_space (ph->thread->inf->pspace);

  scoped_restore save_inferior_ptid = make_scoped_restore (&inferior_ptid);
  inferior_ptid = ph->thread->ptid;

so we should probably do the same for consistency.

Also, the same logic will be required in pdc_write_data, where it
is currently missing completely.

>+  for (thread_info *tp : all_threads (proc_target, ptid_t (pid)))
>+  {
>+    **(struct thread_info ***) &g = tp;
>+    (*(struct thread_info ***) &g)++;
>+  }

This looks unnecessarily complicated.  Isn't this just
   *g++ = tp;
?

>+	  /* If there is only one thread then we need not make the main 
>+	     thread look like a thread.  It can stay as a process. This
>+	     is useful when we have multiple inferiors, but only one is
>+	     threaded.  So we need not make the other inferiors with only
>+	     main thread, look like a threaded one.  For example, Thread
>+	     1.1, 1.2, 2.1, 3.1 exists then it is useful to skip this for
>+	     loop for 2.1 and 3.1 leaving them as main process thread with
>+	     a dummy priv set.  */
>+
>+	  if (pcount == 1 && gcount == 1)
>+	  {
>+	    aix_thread_info *priv = new aix_thread_info;
>+	    tp = find_thread_ptid (proc_target, gptid);
>+	    tp->priv.reset (priv);
>+	    break;
>+	  }

Is this a change in behavior to current GDB?  I thought if the
application (whether a single inferior or one of multiple inferiors)
is threaded in the sense that it uses the libpthread library we
wanted to show it as threaded, so that the user can e.g. see the
thread ID in info threads.

>+	      /* This is to make the main process thread now look
>+		 like a thread.  */
>+
>+	      if (gptid.is_pid () && gptid.pid () == pptid.pid ())
>+	      {
>+		thread_change_ptid (proc_target, gptid, pptid);
>+		aix_thread_info *priv = new aix_thread_info;
>+		priv->pdtid = pbuf[pi].pdtid;
>+		priv->tid = pbuf[pi].tid;
>+		tp = find_thread_ptid (proc_target, pptid);
>+		tp->priv.reset (priv);
>+		pi++;
>+		gi++;
>+	      }
>+	      else
>+	      {
>+		delete_thread (gbuf[gi]);
>+		gi++;
>+	      }

This logic is still confusing me.  Why is the
   gptid.pid () == pptid.pid ()
check still needed?  I thought we now collected only threads
of a single process to begin with, so they all ought to have
the same PID?

Also, if the point is the gptid.is_pid () check, this can
really only happen once per inferior, as it is switched
from non-threaded to threaded mode, right?  Maybe it
would simplify the logic to have all that (including
the code under 
  if (pcount == 1 && gcount == 1)
above if it is actually needed) in a separate statement
before that loop.

I.e. directly before the loop, have a separate check
whether the current process only has a single thread,
whose ptid_t is still in the pid-only format, and if
so, upgrade it to full TID format using the main thread's
TID.  Only after that, go through the loop to handle
any other threads we may also have.  (At that point,
all GDB threads should already always be in TID format.)

>-  if (!PD_TID (ptid))
>+  if (!(ptid.tid () != 0))

That should just be "if (ptid.tid () == 0)" then.
(Here and in a few other places.)

>@@ -1741,7 +1823,7 @@ aix_thread_target::mourn_inferior ()
> {
>   target_ops *beneath = this->beneath ();
> 
>-  pd_deactivate ();
>+  pd_disable ();
>   beneath->mourn_inferior ();
> }

Why is this necessary?  If it is, do we even need two
separate pd_deactivate and pd_disable routines any more?

>@@ -618,6 +618,16 @@ solib_aix_bfd_open (const char *pathname)
>       if (member_name == bfd_get_filename (object_bfd.get ()))
> 	break;
> 
>+      std::string s = bfd_get_filename (object_bfd.get ());
>+
>+      /* For every inferior after first int bfd system we 
>+	 will have the pathname instead of the member name
>+	 registered. Hence the below condition exists.  */
>+
>+      if (s.find ('(') != std::string::npos
>+	  && s.find (member_name) != std::string::npos)
>+	return object_bfd; 

Ah, I guess you also need to ensure the member_name follows
immediately after the '(', otherwise there could be confusion
if the member name happens to be part of the file name as well.


Bye,
Ulrich