RE: [PATCH v2] Speed up next/step while debugging multithreaded programs on AIX.

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

[-- Attachment #1: Type: text/plain, Size: 4957 bytes --]



Hi everyone,

I am sending a 3 patch series to address the concerns, Thank you very much for the review. Here are my thoughts.

>> 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?

Made the corrections.


>> 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.

Yes, in the v3 version I have handle this. GDB resumes all threads (to let the callee execute), using ptid.tid() == 0. That path in resume() sets last_resume_step = 0. So when the return-address breakpoint fires, step_stop = false and the full sync runs and any new thread is caught.


>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:

The first patch in the coming series addresses these. Thanks for pointing out.

>Ulrich already pointed out that you could have one thread appear and one
>disappear, and the count will not change.

Yes, this is something I did not think. Thanks to both of you. The v3 patch implementation is based on single step only.  No thread counts used.

>So yeah, what about doing "next" over a call that spawns a thread, or a
>background thread existing while you do a next?

The fix is safe for both scenarios because step_stop=true is only set when software single-step breakpoints were inserted for the resumed thread which means GDB knows the thread executed exactly one instruction and could not have called pthread_create or pthread_exit. For next over a function call that spawns a thread, GDB resumes all threads (ptid.tid()==0), which explicitly clears last_resume_step=0 at line 1087, so step_stop is never true and sync_threadlists() runs in full, picking up the new thread. For a background thread exiting during next, the single-step skips only apply to the internal one-instruction steps of the stepped thread; as soon as GDB resumes all threads for the next source-line boundary it clears last_resume_step=0, so the following stop calls sync_threadlists() and detects the exit. In both cases sync_threadlists() is always called on any stop where thread population could have changed. The optimization only fires for stops that are pure single-step traps of a single thread. No new thread can be created or destroyed in one instruction, so skipping the session update on those stops is correct. This is what my thought process is.


>> +/* 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.

Sure, corrected the same in second patch of the coming series.


>> +#define GETTHRDS_BATCH 64
>Prefer:
>constexpr int GETTHRDS_BATCH = 64;

Done this in v3 version of the patch.

>> +
>>  /* 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.

I have corrected this.


>> -  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)

>>     {

>> +      for (i = 0; i < count; i++)
>> +     if (thrinf[i].ti_cursig)
>> +       return thrinf[i].ti_tid;

>Declare variable `i` in the for loop.

The above two are addressed in v3 version of this patch. Simon, that works with for loop and not with while.

Have a nice day ahead.

Thanks and regards,
Aditya.



[-- Attachment #2: Type: text/html, Size: 13975 bytes --]