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:

>>Is there an existing gdb test case that exercises this code?
>>If not then it seems like a new test is warranted. 
>
>This I am not aware of at least when I tried finding. 

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.


I think we're getting pretty close now, but I do still have
some additional comments on the latest patch:

> /* Return whether to treat PID as a debuggable thread id.  */
> 
>-#define PD_TID(ptid)	(pd_active && ptid.tid () != 0)
>+#define PD_TID(ptid, data)	(data->pd_active && ptid.tid () != 0)

I'm not sure why the pd_active test is needed here
at all - if ptid.tid () != 0, thread debugging *must*
be active, otherwise you'd never have installed a ptid
with the tid field set, right?

If that check can indeed be omitted, that would simplify
the patch a bit since you wouldn't need to provide the
"data" struct in quite as many places.

>/* Address of the function that libpthread will call when libpthdebug
>   is ready to be initialized.  */
>
> static CORE_ADDR pd_brk_addr;

I believe this needs to go into the aix_thread_variables struct;
the address might be different if the pthread library is loaded
at different addresses into different inferiors.

> /* Whether the current architecture is 64-bit.  
>    Only valid when pd_able is true.  */
>
>static int arch64;

Likewise - some inferiors may be 64-bit and others 32-bit.

In general, *any* static variable in this file is suspect.


>+/* Callback for counting GDB threads for process pid.  */
> 
> static int
>-giter_count (struct thread_info *thread, void *countp)
>+giter_count (pid_t pid)

This only was a callback because it was called via
iterate_over_threads.  Now that you're using the
all_threads C++ iterator, I think both of those
routines should just be inlined into its caller.


>@@ -565,6 +565,7 @@ solib_aix_bfd_open (const char *pathname)
>   const char *sep;
>   int filename_len;
>   int found_file;
>+  std::string string_path = pathname;
> 
>   if (pathname[path_len - 1] != ')')
>     return solib_bfd_open (pathname);
>@@ -618,6 +619,15 @@ 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 (string_path.compare (s) == 0)
>+	return object_bfd;

That's still not quite right, as the pathname component
might have been changed here:
  /* Calling solib_find makes certain that sysroot path is set properly
     if program has a dependency on .a archive and sysroot is set via
     set sysroot command.  */
  gdb::unique_xmalloc_ptr<char> found_pathname
    = solib_find (filename.c_str (), &found_file);

I think a simple but correct way to check whether the BFD filename
already contains a member name is to check for the presence of an
opening parenthesis e.g. via:
    strrchr (bfd_get_filename (...), '(');


Bye,
Ulrich