public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Aditya Kamath1 <Aditya.Kamath1@ibm.com>
To: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>,
	"simark@simark.ca" <simark@simark.ca>,
	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] 0001-Fix-multi-thread-debug-bug-in-AIX.patch
Date: Sun, 13 Nov 2022 18:15:43 +0000	[thread overview]
Message-ID: <CH2PR15MB3544E198B0622C3AD22597A2D6029@CH2PR15MB3544.namprd15.prod.outlook.com> (raw)
In-Reply-To: <5df6ab523034d1997ffda5bb06c3bd87777dcccb.camel@de.ibm.com>


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

Hi Ulrich,

Please find attached the new patch. See 0001-Fix-Multi-thread-debug-bug-fix-in-AIX.patch ..

>So I think instead of adding a "priv" struct to that GDB thread
>identifying the main process, the sync_threadlists routine should
>actually just delete it (or replace it with the actual first thread,
>whatever is easier).

I have chosen not to add the first main thread as new thread. Instead, we carry on with main process thread itself adding private data to it. Kindly see the first if condition. I observed this with the linux folks where in their output as you mentioned do not add any new threads the first time on recognition of multi thread debugee for the main process.

>Looking at the code, there already seems to be a place where
>sync_threadlists deletes GDB threads that do not match onto
>and of the threads reported by the AIX thread library - can
>you verify why this doesn't trigger here?

So that is when there is a thread that exits, and it will reflect in pthread library thread buffer but not in the GDB thread buffer. In order to keep both of them in sync we delete extra one's in GDB thread library buffer to maintain sync in both of them. It is not going to hit to delete extra threads representing the same thread be it for the main process or anything else. pi == pcount will happen only in the thread exit case where pthread buf threads are deleted as it will immediately reflect but in GDB thread buf we need to take care of the same by deleting the exited threads.

A couple of things I want to inform you is that the way the second for loop is executing is not correct from here on to sync both the buffer lists [pthread and GDB thread]. Since we are now not adding multiple threads for the same process main thread one representing the GDB thread and the other by the pthread those conditions and indices like pi and gi will fail. Now there has not pcount - 1 threads in the GDB thread buffer always. Condition 2 and 3 in the patch take care of them for addition and deletion of threads.

Attaching a sample output with source code.

Let me know what you think and if any case that I may have missed out. If it is okay, then kindly push this patch.

Have a nice day ahead.

Thanks,
Regards,
Aditya.

------------------------------------------------------------------
Consider the program below:- [ Program Credits:-  GDB test case continue-pending-status.c]


#include <stdio.h>

#include <unistd.h>

#include <stdlib.h>

#include <pthread.h>

#include <assert.h>


pthread_barrier_t barrier;


#define NUM_THREADS 2


void *

thread_function (void *arg)

{



  pthread_barrier_wait (&barrier);


  while (1); /* break here */

}


int

main (void)

{

  int i;


  alarm (300);


  pthread_barrier_init (&barrier, NULL, NUM_THREADS);


  for (i = 0; i < NUM_THREADS; i++)

    {

      pthread_t thread;

      int res;


      res = pthread_create (&thread, NULL,

                            thread_function, NULL);

      assert (res == 0);

    }

  while (1)

    sleep (1);


  return 0;

}

Output after patch:-


Reading symbols from /home/XYZ/gdb_tests/continue-pending-status...

(gdb) r

Starting program: /home/XYZ/gdb_tests/continue-pending-status

^C[New Thread 258]

[New Thread 515]


Thread 1 received signal SIGINT, Interrupt.

0xd0595fb0 in _p_nsleep () from /usr/lib/libpthread.a(shr_xpg5.o)

(gdb) info threads

  Id   Target Id                          Frame

* 1    process 26149278                   0xd0595fb0 in _p_nsleep ()

   from /usr/lib/libpthread.a(shr_xpg5.o)

  2    Thread 258 (tid 24445361, running) thread_function (arg=0x0)

    at continue-pending-status.c:36

  3    Thread 515 (tid 16187681, running) thread_function (arg=warning: (Internal error: pc 0x0 in read in psymtab, but not in symtab.)


0x0)

    at continue-pending-status.c:36

(gdb)


________________________________
From: Ulrich Weigand <Ulrich.Weigand@de.ibm.com>
Sent: 08 November 2022 17:47
To: simark@simark.ca <simark@simark.ca>; Aditya Kamath1 <Aditya.Kamath1@ibm.com>; gdb-patches@sourceware.org <gdb-patches@sourceware.org>
Cc: Sangamesh Mallayya <sangamesh.swamy@in.ibm.com>
Subject: Re: [PATCH] 0001-Fix-multi-thread-debug-bug-in-AIX.patch

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

>>You should find out why the "priv" field isn't
>>set up correctly, and fix whatever was going
>>wrong there.  (I believe this should have been
>>done in sync_threadlists.)
>
>You were right about this. What is happening is the main process
>and the thread representing it are treated as two separate threads
>by the libpthread library. Main process had no private data set
>whereas the thread representing it had. Usually, both of them
>should have it and their private data must be the same.

I see.  I agree this is the root cause of the issue, but the fix
doesn't look quite right to me.

You should not even *have* the duplicate GDB thread in the first place.

>(gdb) info threads
>  Id   Target Id                          Frame
>* 1    process 12059046                   0xd0595fb0 in _p_nsleep () from /usr/lib/libpthread.a(shr_xpg5.o)
>  2    Thread 1 (tid 39125487, running)   0xd0595fb0 in _p_nsleep () from /usr/lib/libpthread.a(shr_xpg5.o)
>  3    Thread 258 (tid 23396809, running) thread_function (arg=0x0) at continue-pending-status.c:36
>  4    Thread 515 (tid 36503883, running) thread_function (arg=warning: (Internal error: pc 0x0 in read in psymtab, but not in symtab.)

This is not how GDB threads are handled on any other platform.  While
you do have a single dummy thread representing the process if it is
*non-threaded*, as soon as the process is recognized as multi-threaded,
you will only see a single GDB thread per target thread, and no
separate "thread" for the whole process.

So I think instead of adding a "priv" struct to that GDB thread
identifying the main process, the sync_threadlists routine should
actually just delete it (or replace it with the actual first thread,
whatever is easier).

Looking at the code, there already seems to be a place where
sync_threadlists deletes GDB threads that do not match onto
and of the threads reported by the AIX thread library - can
you verify why this doesn't trigger here?

Bye,
Ulrich


[-- Attachment #2: 0001-Fix-multi-thread-debug-bug-in-AIX.patch --]
[-- Type: application/octet-stream, Size: 4134 bytes --]

From b9af3062ad2037b8acb463a0bba0b8cfc6a1c405 Mon Sep 17 00:00:00 2001
From: Aditya Vidyadhar Kamath <Aditya.Kamath1@ibm.com>
Date: Sun, 13 Nov 2022 11:48:52 -0600
Subject: [PATCH] Fix multi thread debug bug in AIX

When a process is multi threaded then AIX was adding a new thread with a priv set.

This thread is the same as main process thread without a private data set.

Hence an assertion failure checked_static_cast: Assertion result != nullptr failed is seen

This patch is a fix for the same where only new threads created are added and once a program

 is multi threaded then the main process private data is set instead of adding a new thread for itself.
---
 gdb/aix-thread.c | 104 +++++++++++++++++++++--------------------------
 1 file changed, 47 insertions(+), 57 deletions(-)

diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c
index e556c153576..6f910fcf847 100644
--- a/gdb/aix-thread.c
+++ b/gdb/aix-thread.c
@@ -795,69 +795,59 @@ sync_threadlists (int pid)
   g = gbuf = XNEWVEC (struct thread_info *, gcount);
   iterate_over_threads (giter_accum, &g);
   qsort (gbuf, gcount, sizeof *gbuf, gcmp);
+  
+  /* If this is the first time the process is pthreaded then
+     the main process running as one thread must have a private
+     data.  */
+ 
+  if (pcount == 1 && gcount == 0)
+  {
+    pi = 0;
+    aix_thread_info *priv = new aix_thread_info;
+    priv->pdtid = pbuf[pi].pdtid;
+    priv->tid = pbuf[pi].tid;
+    process_stratum_target *proc_target
+                = current_inferior ()->process_target ();
+    thread_info *tp = find_thread_ptid (proc_target, ptid_t (pid));
+      if (tp->priv == NULL)
+        tp->priv.reset (priv);
+  }
 
-  /* Apply differences between the two arrays to GDB's thread list.  */
-  for (pi = gi = 0; pi < pcount || gi < gcount;)
-    {
-      if (pi == pcount)
-	{
-	  delete_thread (gbuf[gi]);
-	  gi++;
-	}
-      else if (gi == gcount)
-	{
-	  aix_thread_info *priv = new aix_thread_info;
-	  priv->pdtid = pbuf[pi].pdtid;
-	  priv->tid = pbuf[pi].tid;
-
-	  process_stratum_target *proc_target
-	    = current_inferior ()->process_target ();
-	  thread = add_thread_with_info (proc_target,
-					 ptid_t (pid, 0, pbuf[pi].pthid),
-					 priv);
-
-	  pi++;
-	}
-      else
-	{
-	  ptid_t pptid, gptid;
-	  int cmp_result;
+  /* This means that new threads have been created and
+     they need to be added in the GDB threads list.  */
 
-	  pptid = ptid_t (pid, 0, pbuf[pi].pthid);
-	  gptid = gbuf[gi]->ptid;
-	  pdtid = pbuf[pi].pdtid;
-	  tid = pbuf[pi].tid;
+  else if (pcount > 1 && gcount < pcount)
+  {
+    pi = 1;
+    while (pcount - 1 != gcount)
+    {
+      aix_thread_info *priv = new aix_thread_info;
+      priv->pdtid = pbuf[pi].pdtid;
+      priv->tid = pbuf[pi].tid;
 
-	  cmp_result = ptid_cmp (pptid, gptid);
+      process_stratum_target *proc_target
+        = current_inferior ()->process_target ();
+      thread = add_thread_with_info (proc_target,
+                                     ptid_t (pid, 0, pbuf[pi].pthid),
+                                     priv);
+      pi++;
+      gcount++; 
+    }    
+  }
 
-	  if (cmp_result == 0)
-	    {
-	      aix_thread_info *priv = get_aix_thread_info (gbuf[gi]);
+  /* This condition implies the threads have exited or died.  Hence
+     we delete those exited threads.  Since we don't add a thread
+     for main process pcount must be equal to gcount + 1.  */
 
-	      priv->pdtid = pdtid;
-	      priv->tid = tid;
-	      pi++;
-	      gi++;
-	    }
-	  else if (cmp_result > 0)
-	    {
-	      delete_thread (gbuf[gi]);
-	      gi++;
-	    }
-	  else
-	    {
-	      process_stratum_target *proc_target
-		= current_inferior ()->process_target ();
-	      thread = add_thread (proc_target, pptid);
-
-	      aix_thread_info *priv = new aix_thread_info;
-	      thread->priv.reset (priv);
-	      priv->pdtid = pdtid;
-	      priv->tid = tid;
-	      pi++;
-	    }
-	}
+  else if (pcount >= 1 && gcount > pcount)
+  {
+    gi  = 0;
+    while (gcount != pcount - 1)
+    {
+      delete_thread (gbuf[gi++]);
+      gcount--;
     }
+  }
 
   xfree (pbuf);
   xfree (gbuf);
-- 
2.31.1


  reply	other threads:[~2022-11-13 23:00 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-25  6:47 Aditya Kamath1
2022-10-28  9:49 ` Ulrich Weigand
2022-11-08 12:00   ` Aditya Kamath1
2022-11-08 12:17     ` Ulrich Weigand
2022-11-13 18:15       ` Aditya Kamath1 [this message]
2022-11-15 18:16         ` Ulrich Weigand
2022-11-21  8:27           ` Aditya Kamath1
2022-11-23 14:15             ` Ulrich Weigand
2022-11-23 16:03               ` Aditya Kamath1
2022-11-23 17:09                 ` Ulrich Weigand
2022-11-23 18:45                   ` Aditya Kamath1
2022-11-29  8:18                     ` Aditya Kamath1
2022-11-30 14:57                       ` Ulrich Weigand
2022-12-02  7:50                         ` Aditya Kamath1
2022-12-05 18:33                           ` Ulrich Weigand
2022-12-08 10:28                             ` Aditya Kamath1
2022-12-08 10:46                               ` Aditya Kamath1
2022-12-08 16:29                               ` Ulrich Weigand
2022-12-15 12:58                                 ` Aditya Kamath1
2022-12-15 15:53                                   ` Ulrich Weigand
2022-12-19  6:30                                     ` Aditya Kamath1
2022-12-22 12:50                                       ` Ulrich Weigand
2022-12-26 13:18                                         ` Aditya Kamath1
2023-01-09 14:04                                           ` Ulrich Weigand
2023-01-10 12:23                                             ` Aditya Kamath1
2023-01-11 13:31                                               ` Ulrich Weigand
2023-01-13 14:06                                                 ` Aditya Kamath1
2023-01-20 14:44                                                   ` Ulrich Weigand
2023-01-27 14:40                                                     ` Aditya Kamath1
2023-01-30 19:54                                                       ` Tom Tromey
2023-02-02  6:24                                                       ` Aditya Kamath1
2023-02-02  6:35                                                         ` Aditya Kamath1
2023-02-02 17:43                                                           ` Ulrich Weigand
2023-02-03 11:10                                                             ` Aditya Kamath1
2023-02-06 19:07                                                               ` Ulrich Weigand
2023-02-07 11:57                                                                 ` Aditya Kamath1
2023-02-08 18:44                                                                   ` Ulrich Weigand
2023-02-10 16:33                                                                     ` Aditya Kamath1
2023-02-10 16:46                                                                       ` Aditya Kamath1
2023-02-13 19:01                                                                       ` Ulrich Weigand
2023-02-14 14:13                                                                         ` Aditya Kamath1
2023-02-16 19:46                                                                           ` Ulrich Weigand
2023-02-17 11:26                                                                             ` Aditya Kamath1
2023-02-17 12:04                                                                               ` Ulrich Weigand
2023-02-17 13:22                                                                                 ` Aditya Kamath1
2023-02-17 14:18                                                                                   ` Ulrich Weigand
2023-02-17 15:15                                                                                     ` Aditya Kamath1
2023-02-17 19:14                                                                                       ` Ulrich Weigand
2022-11-08 12:00 Aditya Kamath1

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CH2PR15MB3544E198B0622C3AD22597A2D6029@CH2PR15MB3544.namprd15.prod.outlook.com \
    --to=aditya.kamath1@ibm.com \
    --cc=Ulrich.Weigand@de.ibm.com \
    --cc=gdb-patches@sourceware.org \
    --cc=sangamesh.swamy@in.ibm.com \
    --cc=simark@simark.ca \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).