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

[Kevin Buettner <kevinb@redhat.com>]

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.

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