From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id A+6rBQawomoWzDwAWB0awg (envelope-from ) for ; Thu, 10 Sep 2026 09:26:30 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=polymtl.ca header.i=@polymtl.ca header.a=rsa-sha256 header.s=oct2025 header.b=EpMwM1AO; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 012821E09E; Thu, 10 Sep 2026 09:26:29 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 Received: from vm01.sourceware.org (vm01.sourceware.org [38.145.34.32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id E8BD01E091 for ; Thu, 10 Sep 2026 09:26:28 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 8DBF74BB3B81 for ; Thu, 10 Sep 2026 13:26:27 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8DBF74BB3B81 Authentication-Results: sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=polymtl.ca header.i=@polymtl.ca header.a=rsa-sha256 header.s=oct2025 header.b=EpMwM1AO Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 15F5A4BB3B81 for ; Thu, 10 Sep 2026 13:26:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 15F5A4BB3B81 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=polymtl.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=polymtl.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 15F5A4BB3B81 Authentication-Results: sourceware.org; arc=none smtp.remote-ip=132.207.4.11 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1789046761; cv=none; b=nepa1O+TivH34K1rXjVkQuGoZJyrkcXyRlFgPvg5m/xYYgJZmmTVZliOJGbu3ux14x0s4p+1GggcFW86GH5Dc0tAp9r05GbeL0Ith/fjV2MiPwr5b0PtMc7US/ctC3EHJkwiE2znCeqRSKeA2Gak7k5aITYEYSG/2BOp1SBgOb8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1789046761; c=relaxed/simple; bh=fD/WlEhsuWLl/1xGIE8bB5fkVt8DV7Wol4sWIOXAhw8=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=FjoIMG9uZizqs6RnHBE0BukxPaHBXOp7plJp3OPKY3jV7s5yrdUymkO09sB2vEMCivYdhduGOrTumA6zVuzAFmOneWLotahpGWvagjUhIJ+iLjZCCOiauexcGwGD66YnQPBl/gZtN6W6GwomeRcTRYareBW7H/0vp99VOyZqG1M= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=polymtl.ca header.i=@polymtl.ca header.a=rsa-sha256 header.s=oct2025 header.b=EpMwM1AO DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 15F5A4BB3B81 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 68ADPoJA199009 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 10 Sep 2026 09:25:55 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 68ADPoJA199009 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=oct2025; t=1789046757; bh=hB8znrDpwaulWfWRxtuno+Jie64kmakLu059vfbBMT4=; h=Date:Subject:To:Cc:From:In-Reply-To:From; b=EpMwM1AOInr1Rssre7kuRbp9kU8wubqhhDgk9OqwnsB6TFdw7mA+NcTZnNIrHgzWR QkVlpbBPRyRk585L1DuZr8hPPqyKH6X2FY61BTIFO/ev4/RA3xJyGKPd+Ie/bkA4LU ovhXzJtZwWplBQGzZbn9ueQ/X5viCtwE1Vv2R3YQxaWreHm+Ab+8ObBJM2usWp0J0H HeXYnGfO2JzDKY3dLoqDjrBySc7Y5nqrEhNt8pBSQ8+R4/2qJ97TXUFeVtPIsdxpf6 UXfxSxkyri/QPH7wBJ5NWH1bPFnShS/4SIKnnkuWBTNTUJ9B169oWdcxqCXRfagd4J EVqWxjICzWvuQ== Received: by simark.ca (Postfix) id 0DC461E091; Thu, 10 Sep 2026 09:25:50 -0400 (EDT) Message-ID: <13694747-63ee-44a0-ae1a-fdd1527dd009@polymtl.ca> Date: Thu, 10 Sep 2026 09:25:49 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] Speed up next/step while debugging multithreaded programs on AIX. To: Aditya Vidyadhar Kamath , ulrich.weigand@de.ibm.com, tom@tromey.com Cc: gdb-patches@sourceware.org, Aditya.Kamath1@ibm.com, sangamesh.swamy@in.ibm.com References: <20260910101643.85955-2-akamath996@gmail.com> Content-Language: en-US From: Simon Marchi In-Reply-To: <20260910101643.85955-2-akamath996@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 10 Sep 2026 13:25:51 +0000 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org On 2026-09-10 06:16, Aditya Vidyadhar Kamath wrote: > From: Aditya Vidyadhar Kamath > > During go compiler debugging on AIX my collegue and I noticed that > the next command was pretty slow compared to Linux - around 7 seconds > per step when program has atleast 3 or more threads running. A simple > walk through a function took around a minute. In compiler codes this is > slow. > > TO measure this properly I wrote a benchmark pasted below. The program spawns > 20 worker threads , then stops inside a function with 10 simple > that GDB steps over one by one. The GDB batch script breaks at that Seems to be missing a word after "simple". Statements? > function and issues 10 next commands and then quits. Timing the whole > run gives a fair picture of how much overhead each `next` carries. > Both the .c file and batch file are pasted below. > > Before this patch, on AIX 7.3 ppc64 with 20 background threads: > > real 1m10.05s > user 0m17.59s > sys 0m20.43s > > That is roughly 7 seconds per `next` step. > > After this patch: > > real 0m20.37s > user 0m5.94s > sys 0m6.53s > > About 2 seconds per step. 3.5x faster for the same workload. > > Now why was it slow? > > Every time the inferior stops -- even for a single-step trap from next > - the AIX thread layer called pd_update(), which did two very expensive > things unconditionally: > > 1. pthdb_session_update () > > This function walks through all of the pthread library's internal > data structures inside the inferior process, reading them out via > pdc_read_data() callbacks. With 20 threads that is over 300 memory > reads from the inferior on every single stop, even when absolutely > nothing about the threads has changed. > > 2. sync_threadlists() > > This follows the session update with a full rebuild of GDB's thread > list: iterate every thread via pthdb_pthread(), call > pthdb_pthread_tid() for each one, allocate two sorted arrays, diff > them against GDB's internal list, and reconcile the differences. > Again, all of this on every stop, even when the thread population > has not changed at all since the last stop. > > On top of that, get_signaled_thread() was calling getthrds() asking for > one kernel thread descriptor at a time, which costs one syscall per > thread in the process. > > None of this work is wrong but it is needed when threads actually come > and go. But during a next or step sequence through a function, the > thread list does not change. Paying for a full rescan on every stop is > something not needed I think. I mean, it *could* change, you could do "next" over a function that spawns a thread, a background thread (one other than the one you step) could spawn a thread, another thread could have exited, etc. > So what I changed in this patch is: > > 1. Batch getthrds() calls get_signaled_thread () > > The original code passed 1 as the count argument to getthrds(), so > it did one syscall per kernel thread to find the one that stopped. > Changed to fetch 64 descriptors per call and scan the batch in a > loop. This alone cuts the getthrds() cost from O(N) syscalls to > O(N/64). It's unrelated to this patch, but I happen to be looking at aix-thread.c because of it and I must ask if these things are still relevant, given the AIX versions and configurations you support nowadays: 1. #if !HAVE_DECL_GETTHRDS extern int getthrds (pid_t, struct thrdsinfo64 *, int, tid_t *, int); #endif The commit message that added this is: On newer versions of AIX (6.x and later), this function is actually declared in procinfo.h, thus causing a compilation warning when we re-declare it ourselves. This patch adds a configure check for that function allowing us to declare the function only if the declaration isn't already present in one of procinfo system header. 2. /* In AIX 5.1, functions use pthdb_tid_t instead of tid_t. */ #ifndef PTHDB_VERSION_3 #define pthdb_tid_t tid_t #endif 3. #ifdef HAVE_PTRACE64 # define ptracex(request, pid, addr, data, buf) \ ptrace64 (request, pid, addr, data, buf) #endif > 2. Track the thread count - aix_thread_variables, pd_activate > > Added a last_thread_count field to aix_thread_variables. It records > how many threads libpthdebug reported the last time a full sync was > done. Initialised to -1 in the constructor so the very > first stop always does a full sync. Also reset to -1 in pd_activate() > so that whenever a new debug session is opened the first stop is > always fully synced regardless of the stop kind. Ulrich already pointed out that you could have one thread appear and one disappear, and the count will not change. > 3. Skip the sync on clean step stops > > pd_update() now takes a bool step_stop parameter. When that is true > and last_thread_count is not -1, it skips pthdb_session_update() and > sync_threadlists() entirely. > > When a sync is needed (not a step stop, or first stop after > activation), a new helper count_pthdb_threads() does a lightweight > pass through pthdb_pthread() just to count threads. It only calls > sync_threadlists() if that count differs from last_thread_count. > This avoids the expensive pthdb_pthread_tid() calls and array > allocations on stops where the thread count has not changed. So yeah, what about doing "next" over a call that spawns a thread, or a background thread existing while you do a next? > 4. Detect step stops in wait() and pass the flag down > > wait() already has the stop kind and signal number before it calls > pd_update(). A TARGET_WAITKIND_STOPPED with GDB_SIGNAL_TRAP is what > the kernel reports for a completed single-step. wait() now computes > step_stop from those two fields and passes it to pd_update(). This sounds wrong, as a regular breakpoint is also (typically) reported as a TARGET_WAITKIND_STOPPED with a SIGTRAP. In the implementation, you also check for data->last_resume_step, which is not mentioned here. > int current_count = count_pthdb_threads (data); > if (current_count != data->last_thread_count) > > This makes sure we never miss any genuinely coming new threads and are > in sync. > > ================== > Benchmark program (bench_next.c): > > /* Spawns NUM_THREADS worker threads that spin, then steps through > 10 assignments in do_steps() so GDB can time each next. */ > #include > #include > #include > #include > > #ifndef NUM_THREADS > #define NUM_THREADS 20 > #endif > > static volatile int keep_running = 1; > > static void * > worker (void *arg) > { > while (keep_running) > sched_yield (); > return NULL; > } > > void > do_steps (void) > { > volatile int a = 1; > volatile int b = 2; > volatile int c = a + b; > volatile int d = c * 2; > volatile int e = d - a; > volatile int f = e + c; > volatile int g = f / 2; > volatile int h = g + 1; > volatile int i2 = h * h; > volatile int j = i2 - b; > (void)j; > } > > int > main (void) > { > pthread_t threads[NUM_THREADS]; > int i; > > for (i = 0; i < NUM_THREADS; i++) > pthread_create (&threads[i], NULL, worker, NULL); > > do_steps (); > > keep_running = 0; > for (i = 0; i < NUM_THREADS; i++) > pthread_join (threads[i], NULL); > > return 0; > } > > ==================== > Build done with > gcc -O0 -g -gdwarf -maix64 -DNUM_THREADS=20 -o bench_next_bin bench_next.c -lpthread > ===================== > GDB batch script (bench_next.gdb): > > set pagination off > set confirm off > > break do_steps > run > > next > next > next > next > next > next > next > next > next > next > > quit > ======================== > Then run, > time gdb -batch -x bench_next.gdb ./bench_next_bin > --- > gdb/aix-thread.c | 153 ++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 130 insertions(+), 23 deletions(-) > > diff --git a/gdb/aix-thread.c b/gdb/aix-thread.c > index 5ac71d22237..3370736810a 100644 > --- a/gdb/aix-thread.c > +++ b/gdb/aix-thread.c > @@ -173,6 +173,11 @@ static pthdb_callbacks_t pd_callbacks = { > /* Aix variable structure. */ > struct aix_thread_variables > { > + aix_thread_variables () > + : pd_able (0), pd_active (0), pd_session (0), pd_brk_addr (0), > + arch64 (0), last_thread_count (-1), last_resume_step (0) > + {} > + > /* Whether the current application is debuggable by pthdb. */ > int pd_able; > > @@ -192,6 +197,19 @@ struct aix_thread_variables > > /* Describes the number of thread exit events reported. */ > std::unordered_set exited_threads; > + > + /* Last known libpthdebug thread count. Used to skip sync_threadlists() > + when the thread population has not changed since the previous stop, > + which is the common case during stepping. -1 means unknown/force sync. */ > + int last_thread_count; > + > + /* Set to non-zero by resume() when the resume was a software > + single-step, i.e. single-step breakpoints were inserted for the > + current thread before the inferior was set running. Cleared to > + zero for any other kind of resume. Used by wait() to distinguish > + a SIGTRAP from a completed single-step from one caused by a > + breakpoint, since both look identical at the signal level. */ > + int last_resume_step; > }; > > /* Key to our per-inferior data. */ > @@ -733,6 +751,11 @@ state2str (pthdb_state_t state) > } > } > > +/* Number of thread descriptors to fetch per getthrds() call. The original > + code fetched one at a time, which costs one syscall per thread. Fetching > + in batches reduces that to one syscall per GETTHRDS_BATCH threads. */ No need to document what the original code did. Just keep the first sentence. > +#define GETTHRDS_BATCH 64 Prefer: constexpr int GETTHRDS_BATCH = 64; > + > /* Search through the list of all kernel threads for the thread > that has stopped on a SIGTRAP signal, and return its TID. > Return 0 if none found. */ This predates your patch, but is the comment for the get_signaled_thread function accurate? It says it looks for a thread that has stopped on a SIGTRAP signal, but the implementation seems to look for any signal, not just SIGTRAP. > @@ -740,20 +763,18 @@ state2str (pthdb_state_t state) > static pthdb_tid_t > get_signaled_thread (int pid) > { > - struct thrdsinfo64 thrinf; > + struct thrdsinfo64 thrinf[GETTHRDS_BATCH]; > tid_t ktid = 0; > + int count, i; > > - while (1) > + while ((count = getthrds (pid, thrinf, sizeof (thrinf[0]), > + &ktid, GETTHRDS_BATCH)) > 0) Prefer declaring the variable in the while statement, and separating the comparison from the assignment. This should work: while (int count = getthrds (pid, thrinf, sizeof (thrinf[0]), &ktid, GETTHRDS_BATCH)); count > 0) > { > - if (getthrds (pid, &thrinf, > - sizeof (thrinf), &ktid, 1) != 1) > - break; > - > /* We also need to keep in mind Trap and interrupt or any > signal that needs to be handled in pd_update (). */ > - > - if (thrinf.ti_cursig) > - return thrinf.ti_tid; > + for (i = 0; i < count; i++) > + if (thrinf[i].ti_cursig) > + return thrinf[i].ti_tid; Declare variable `i` in the for loop. Simon