Re: [PATCH v1][RFC] Speed up next/step while debugging multithreaded programs on AIX.

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

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

Hi Abhay and community members,


Thank you very much for the feedback. Please see my comments below to your concerns and I am sending v2 of this patch after this soon.

>Doesn't a breakpoint stop also arrive as >TARGET_WAITKIND_STOPPED with GDB_SIGNAL_TRAP?
>get_signaled_thread() already looks for the thread stopped >on SIGTRAP to find the
>current thread, and that has to work at breakpoint stops >too, so I would expect
>a breakpoint hit to look the same as a single-step here.

>If so, breakpoint stops also get step_stop = true and skip >the sync

>The last_thread_count < 0 check only covers the first stop >after pd_activate().
After that first sync, >last_thread_count is >= 0 for the rest of the session
and >need_sync becomes just !step_stop.  So from the second stop >onwards
a breakpoint hit skips the sync, and any threads >created since the previous
stop are missed.

You are right here. A user breakpoint hit, a software single-step completion, and the thread-creation stub breakpoint all arrive with exactly the same WAITKIND_STOPPED + GDB_SIGNAL_TRAP. The v1 version of this patch couldn't tell them apart, so step_stop was true for breakpoint hits too, and the sync was wrongly skipped. I checked this. I did not think in this angle and just thought about speeding up. Thanks for pointing it out. Yeah the benchmark will not point this out.

>resume() is already told whether GDB asked for a single >step, and it already
>has the aix_thread_variables pointer in hand.  Could you >save the flag there
>and use it here instead of inferring it from the signal?

>/* aix_thread_target::resume (), after the existing
>data = get_thread_data_helper_for_ptid (ptid); */
>data->last_resume_step = step;

>/* in wait () */
>bool step_stop = (data->last_resume_step
>&& status->kind () == TARGET_WAITKIND_STOPPED
>&& status->sig () == GDB_SIGNAL_TRAP);

>That would also handle "next" over a function call, where GDB puts a temporary
>breakpoint at the return address and continues rather than single-stepping
>through the callee.  resume() is called with step = 0 in that case, so the
>stop would correctly get a full sync even though the user typed "next”.


I like this idea but while implementing came across something else. On AIX rs6000_software_single_step() is registered as the architecture's next-PC provider. infrun.c's maybe_software_singlestep() calls it, which inserts breakpoints at the next instruction(s) and returns hw_step = false, so do_target_resume() always passes step=0. Saving that would have made last_resume_step always 0, killing the optimisation entirely.

In the debug log I saw every do_target_resume call showed step=0, even for next.

The right signal is whether software single-step breakpoints were inserted for the current thread at the time of the resume. That is what thread_has_single_step_breakpoints_set() reports, and it is set when and only when GDB is software-single-stepping through source lines.

So in resume() in v2 version of this patch you will see:

struct thread_info *tp = inferior_thread ();
data->last_resume_step = thread_has_single_step_breakpoints_set (tp);


This is 1 when GDB inserted single-step breakpoints which is a next/step resume, and 0 for any free continue or temporary-breakpoint resume. The wait() side then checks both last_resume_step and GDB_SIGNAL_TRAP — both must be true for the sync skip to apply.

Let me know your thoughts in the next version of this patch.

Have a nice day ahead.

Thank you and regards,

Aditya.



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