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

[Tom Tromey <tom@tromey.com>]

>>>>> Aditya Kamath1 via Gdb-patches <gdb-patches@sourceware.org> writes:

> We now have all variables {pd_able, pd_active and pd_session} now in a
> map of process ID and structure. This will help us make AIX GDB code
> easy to manage them per process in the aix-thread.c file.

I don't really know what this is about, but it's probably better to
attach the data directly to the inferior using the registry system.
(You can't use private_inferior as apparently that's reserved for the
process stratum.)

Search for registry<inferior> for some examples.


> Secondly, in the function pid_to_str () there is a beneath () call,
> which is why I had to put this function in rs6000-aix-nat.c file.

I wonder why it's necessary, as it seems to me that
aix_thread_target::pid_to_str should have already handled the 'thread'
case, so the inherited method ought to be good enough.

> ---------------------------------------------------
> Output with patch applied:-

Is there an existing gdb test case that exercises this code?
If not then it seems like a new test is warranted.

>  static void
>  pd_disable (void)
>  {
> -  if (!pd_able)
> +  if (!tmap[inferior_ptid.pid ()].pd_able)
>      return;
> -  if (pd_active)
> +  if (tmap[inferior_ptid.pid ()].pd_active)
>      pd_deactivate ();
> -  pd_able = 0;
> +  tmap[inferior_ptid.pid ()].pd_able = 0;
>    current_inferior ()->unpush_target (&aix_thread_ops);
> +  tmap.erase (inferior_ptid.pid ());
>  }

It's better to pass in a ptid or even the aix_thread_variables object
itself than to rely on globals in low-level functions like this.

 
> diff --git a/gdb/solib-aix.c b/gdb/solib-aix.c
> index f483f54de13..74676cbfa50 100644
> --- a/gdb/solib-aix.c
> +++ b/gdb/solib-aix.c
> @@ -550,6 +550,10 @@ solib_aix_in_dynsym_resolve_code (CORE_ADDR pc)
>    return 0;
>  }
 
> +/* For multi inferiors, post object file name change
> +   we store the new names in this vector.  */
> +std::vector<std::string> aix_slib_name;

> +      std::string s = bfd_get_filename (object_bfd.get ());
> +      auto it = aix_slib_name.begin ();
> +      while (it != aix_slib_name.end ())
> +      {
> +	std::string s1 = *it;
> +	if (s1.compare(s) == 0)
> +	  return object_bfd;
> +	it++;

This doesn't look right to me at all.  Using a global means that BFDs
from one inferior might "leak" to another, based solely on whether a
certain name was ever seen.  Also nothing ever cleans out the global
vector.

It's better to attach this data to the relevant BFD using the registry
system, and not use a global at all.

Tom