Re: [PATCH] Fix AIX thread NULL assertion failure during fork

[Aditya Kamath1 <Aditya.Kamath1@ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4484 bytes --]

Respected Ulrich and community members,

Please find attached the patch. {See: 0001-Fix-AIX-thr-NULL-assertion-failure-during-fork.patch}

>OK, so this raises two questions:

>- I think we never should call pd_activate twice on a process
>  where pd_active is already true.  This can now happen when
>  you call pd_activate from update_thread_list.

Yes.

>- Do we even need the in_initial_library_scan check at all
>  anymore?
No we do not it. It works without it, since now we use the update_thread_list () hook. Please see the output pasted below. This not required anymore. We used it as we wanted to control the pd_activate () call till libraries were loaded.

>Minor issues I noticed in the patch:

>- Variable "data" in update_thread_list looks unused?
>- Some whitespace issues - please watch the TAB settings,
>  in the GDB sources a TAB should be 8 spaces.

I set this. Now you should pd_update () aligned properly. I will take care of this going forward. Thank you.

If this patch is okay kindly push it. If not let me know.

Have a nice day ahead.

Thanks and regards,
Aditya.

-----------------------------------------------
Reading symbols from //gdb_tests/multi-thread-fork...
(gdb) set detach-on-fork off
(gdb) r
Starting program: /gdb_tests/multi-thread-fork
[New Thread 258]
[New Thread 515]
[New inferior 2 (process 23658790)]
Hello from Parent!
[New inferior 3 (process 8913402)]
Hello from Parent!
info threads

Thread 1.1 received signal SIGINT, Interrupt.
0xd0611d70 in _p_nsleep () from /usr/lib/libpthread.a(_shr_xpg5.o)
(gdb) info threads
  Id   Target Id                           Frame
* 1.1  Thread 1 (tid 9574159, running)     0xd0611d70 in _p_nsleep () from /usr/lib/libpthread.a(_shr_xpg5.o)
  1.2  Thread 258 (tid 8853455, running)   thread_function (arg=0x0) at //gdb_tests/multi-thread-fork.c:50
  1.3  Thread 515 (tid 9770981, running)   thread_function (arg=warning: (Internal error: pc 0x0 in read in psymtab, but not in symtab.)

0x0) at //gdb_tests/multi-thread-fork.c:50
  2.1  Thread 515 (tid 10360707, running)  0xd0610df0 in _sigsetmask () from /usr/lib/libpthread.a(_shr_xpg5.o)
  3.1  Thread 258 (tid 113118727, running) 0xd0610df0 in _sigsetmask () from /usr/lib/libpthread.a(_shr_xpg5.o)
(gdb) q
A debugging session is active.
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Date: Wednesday, 22 November 2023 at 9:03 PM
To: gdb-patches@sourceware.org <gdb-patches@sourceware.org>, Aditya Kamath1 <Aditya.Kamath1@ibm.com>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] Fix AIX thread NULL assertion failure during fork
Aditya Kamath1 <Aditya.Kamath1@ibm.com> wrote:

>>Yes. So we have a little more to understand here. So once pd_enable ()
>>was called and then pd_activate () was called it looks like either
>>the libraries were not fully loaded though the pthdb_session_pthreaded
>>successful or the create thread event breakpoint was not successful.
>>So the code execution did not reach >>pd_activate (). Therefore our
>>child process was still ptid_t (pid, 0 ,0). When I used the pd_activate ()
>>inside the update_threadlist () it succeeded to initialise a session
>>later on when it was called and then we were able to set pd_active
>>and get to sync_threadlists ().
>
>I can confirm that this condition
>/* When attaching / handling fork child, don't try activating
>     thread debugging until we know about all shared libraries.  */
>  if (inf->in_initial_library_scan)
>    return;
>
>is what is the reason we fail to reach pd_activate ().. Sorry for not being clear in my previous email..

OK, so this raises two questions:

- I think we never should call pd_activate twice on a process
  where pd_active is already true.  This can now happen when
  you call pd_activate from update_thread_list.

- Do we even need the in_initial_library_scan check at all
  anymore? I seem to recall we added that as the
  sync_threadlists call could cause confusion during early
  startup.  But now that we don't call sync_threadlists
  from pd_activate any more, maybe we can simply remove that
  check completely?  And then, maybe we no longer need to
  call pd_active from update_thread_list.

Minor issues I noticed in the patch:

- Variable "data" in update_thread_list looks unused?
- Some whitespace issues - please watch the TAB settings,
  in the GDB sources a TAB should be 8 spaces.

Bye,
Ulrich

[-- Attachment #2: 0001-Fix-AIX-thr-NULL-assertion-failure-during-fork.patch --]
[-- Type: application/octet-stream, Size: 3079 bytes --]

From 2bc9251be7c7342ef9776581685a60387aee8e55 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Wed, 22 Nov 2023 10:13:35 -0600
Subject: [PATCH] Fix AIX thr!= NULL assertion failure during fork.

In AIX, while we followed the child process and detach on fork was on we hit thr!= NULL assertion failure.

The reason for the same was GDB core trying to switch to a child thread with tid not set that does not
exist, since child's ptid was changed to ptid_t (pid, 0, tid) in sync_threadlists() as it was threaded.

The way this happened was when a new child process is born, its object file will be loaded, calling the new_objfile ()
in aix-thread.c file from clone_program_space, which is
called from within follow_fork_inferior. Therefore it end ups syncing threadlists via pd_update ().

This patch is a fix for the same where pd_update () is called in the wait () or in update_thread_list() hook only.
---
 gdb/aix-thread.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index 945b7f6c697..c8147defc04 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -136,6 +136,8 @@ class aix_thread_target final : public target_ops
   const char *extra_thread_info (struct thread_info *) override;
 
   ptid_t get_ada_task_ptid (long lwp, ULONGEST thread) override;
+
+  void update_thread_list () override;
 };
 
 static aix_thread_target aix_thread_ops;
@@ -1015,7 +1017,7 @@ pd_update (pid_t pid)
    If successful and there exists and we can find an event thread, return a ptid
    for that thread.  Otherwise, return a ptid-only ptid using PID.  */
 
-static ptid_t
+static void
 pd_activate (pid_t pid)
 {
   int status;
@@ -1030,9 +1032,24 @@ pd_activate (pid_t pid)
       return ptid_t (pid);
     }
   data->pd_active = 1;
-  return pd_update (pid);
+  return;
 }
 
+/* AIX implementation of update_thread_list.  */
+
+void
+aix_thread_target::update_thread_list ()
+{
+  for (inferior *inf : all_inferiors ())
+    {
+      if (inf->pid == 0)
+	continue;
+
+      pd_update (inf->pid);
+    }
+}
+
+
 /* An object file has just been loaded.  Check whether the current
    application is pthreaded, and if so, prepare for thread debugging.  */
 
@@ -1077,11 +1094,6 @@ pd_enable (inferior *inf)
   current_inferior ()->push_target (&aix_thread_ops);
   data->pd_able = 1;
 
-  /* When attaching / handling fork child, don't try activating
-     thread debugging until we know about all shared libraries.  */
-  if (inf->in_initial_library_scan)
-    return;
-
   /* If we're debugging a core file or an attached inferior, the
      pthread library may already have been initialized, so try to
      activate thread debugging.  */
@@ -1216,7 +1228,7 @@ aix_thread_target::wait (ptid_t ptid, struct target_waitstatus *status,
 
       if (regcache_read_pc (regcache)
 	  - gdbarch_decr_pc_after_break (gdbarch) == data->pd_brk_addr)
-	return pd_activate (ptid.pid ());
+	pd_activate (ptid.pid ());
     }
 
   return pd_update (ptid.pid ());
-- 
2.41.0