Re: [PATCH] gdbserver: Install single-step breakpoint for a pending thread whose last_resume_kind is resume_step

Luis Machado via Gdb-patches
2023-07-17T09:36:26Z

Hi,

On 7/11/23 19:42, Kevin Buettner wrote:
> Hi Zhiyong,
>
> See my comments below; there's still one formatting nit, but I also
> have a question about whether this is the right place to fix the bug
> that you're seeing.
>
> Also, I've added Luis to the CC list since he knows a lot more about
> the ARM architecture than I do.

I was aware of this one, but since it is generic code, I thought I'd defer it to the
global maintainers. It doesn't look like something that can/should be handled in arch-specific
layers. But I can revisit it if people think otherwise.

>
> On Mon, 3 Jul 2023 11:04:58 +0800
> <zhiyong.yan@windriver.com> wrote:
>
>> From: Zhiyong Yan <zhiyong.yan@windriver.com>
>>
>> Gdb should not assume pending threads always generate "a non-gdbserver trap event", for example "Signal 17" event could happen. Now that resume_stopped_resumed_lwps() -> may_hw_step() assumes that the break point must already exist, resume_one_thread() should ensure the software breaking point is installed although the thread is pending.
>>
>> Signed-off-by: Zhiyong Yan zhiyong.yan@windriver.com
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30387
>> ---
>> gdbserver/linux-low.cc | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
>> index e6a39202a98..7d8bbf71ddc 100644
>> --- a/gdbserver/linux-low.cc
>> +++ b/gdbserver/linux-low.cc
>> @@ -4671,7 +4671,16 @@ linux_process_target::resume_one_thread (thread_info *thread,
>> proceed_one_lwp (thread, NULL);
>> }
>> else
>> - threads_debug_printf ("leaving LWP %ld stopped", lwpid_of (thread));
>> + {
>> + threads_debug_printf ("leaving LWP %ld stopped", lwpid_of (thread));
>> + if (thread->last_resume_kind == resume_step)
>> + {
>> + /* If resume_step is required by GDB,
>> + install single-step breakpoint. */
>> + if (supports_software_single_step())
>
> Formatting nit: you need a space between 'supports_software_single_step'
> and the left paren.
>
>> + install_software_single_step_breakpoints (lwp);
>> + }
>> + }
>>
>> thread->last_status.set_ignore ();
>> lwp->resume = NULL;
>
> With regard to the change itself...
>
> If the debugging printf is accurate, the LWP is being left in a
> stopped state. If that's the case, then why are software single step
> breakpoints being inserted? It seems to me that you'd only want
> to insert these breakpoints when the thread is actually about to
> resume.
>
> That said, I have no doubt that this change works for you, so clearly
> there's something going on which I do not understand. I'd like to
> understand the situation better before approving it. Also, once you
> have an explanation, please update the code comments and/or commit log
> message as appropriate. My personal preference is for a concise
> explanation to be placed in the code comments with any additional,
> longer winded explanations or examples being placed in the commit log
> message.
>
> Kevin
>