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

[Luis Machado <luis.machado@arm.com>]

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
>