[RFC] [PATCH] [PR gdb/29796] Fix gdbserver delay before quitting in extended-remote

[RFC] [PATCH] [PR gdb/29796] Fix gdbserver delay before quitting in extended-remote

[Shivam Saxena]

[-- Attachment #1: Type: text/html, Size: 5564 bytes --]

Re: [RFC] [PATCH] [PR gdb/29796] Fix gdbserver delay before quitting in extended-remote

[Kevin Buettner]

On Sat, 14 Oct 2023 02:01:42 +0000
Shivam Saxena <saxens12@mcmaster.ca> wrote:

> Investigating PR29796, where it was noticed that there was a delay when trying to quit the gdb session if connected to gdbserver via stdio. It was also mentioned that using --once​ when launching gdbserver​ would fix this problem and a recommendation was made to always apply --once​ when launching gdbserver​ with stdio​.
> 
> I've explicitly set the run_once​ flag when stdio​ is detected at launch of gdbserver. This does fix the problem as suggested in the bug since the conditional at gdbserver/server.cc:3958​ is always triggered.
> ```
>          if (run_once || (!extended_protocol && !target_running ()))
>             throw_quit ("Quit");
> ```
> However, would it be better to create a new flag for this to facilitate quitting despite
> extended_protocol​ instead of piggybacking on --once​ logic?

I don't see a problem with using 'run_once' for this purpose.  But if there's
something I'm missing, please explain...

> 
> Tested on x86_64 Ubuntu 22.04 natively
> Testsuite had no regressions.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29796
> ---
> ```
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index e02cdb83b51..3c9e8bc3720 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -3760,6 +3760,8 @@ captured_main (int argc, char *argv[])
>           /* "-" specifies a stdio connection and is a form of port
>              specification.  */
>           port = STDIO_CONNECTION_NAME;
> +        /* auto-imply --once if invoked via stdio. */

Comments are supposed to be sentence-like.  So the first word is
capitalized and it should end with a period (which it does here)
followed by two spaces.  (So... capitalize "auto" and add an extra
space after the period.)

Aside from this nit, your patch looks good to me.

> +        run_once = true;
>           next_arg++;
>           break;
>         }
> ```
> ---
> 

RE: [RFC] [PATCH] [PR gdb/29796] Fix gdbserver delay before quitting in extended-remote

[Shivam Saxena]

>> Investigating PR29796, where it was noticed that there was a delay when trying to quit the gdb session if connected to gdbserver via stdio. It was also mentioned that using --once​ when launching gdbserver​ would fix this problem and a recommendation was made to always apply --once​ when launching gdbserver​ with stdio​.
>>
>> I've explicitly set the run_once​ flag when stdio​ is detected at launch of gdbserver. This does fix the problem as suggested in the bug since the conditional at gdbserver/server.cc:3958​ is always triggered.
>> ```
>>          if (run_once || (!extended_protocol && !target_running ()))
>>             throw_quit ("Quit");
>> ```
>> However, would it be better to create a new flag for this to 
>> facilitate quitting despite extended_protocol​ instead of piggybacking on --once​ logic?

> I don't see a problem with using 'run_once' for this purpose.  But if there's something I'm missing, please explain...

By the conditional it seems the intent was to not allow quitting immediately if in extended protocol mode. We are adding another condition to quit - if launched in stdio. But nothing in this logic alone would make that apparent. (no comment, no flag). I thought this reduced traceability. But I might be overthinking things. So if the fix seems fine for this purpose I'll fix the patch and reply. It seems I might have to use a different email account to submit the patch via git send-email. 

>>
>> Tested on x86_64 Ubuntu 22.04 natively Testsuite had no regressions.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29796
>> ---
>> ```
>> diff --git a/gdbserver/server.cc b/gdbserver/server.cc index 
>> e02cdb83b51..3c9e8bc3720 100644
>> --- a/gdbserver/server.cc
>> +++ b/gdbserver/server.cc
>> @@ -3760,6 +3760,8 @@ captured_main (int argc, char *argv[])
>>           /* "-" specifies a stdio connection and is a form of port
>>              specification.  */
>>           port = STDIO_CONNECTION_NAME;
>> +        /* auto-imply --once if invoked via stdio. */

> Comments are supposed to be sentence-like.  So the first word is capitalized and it should end with a period (which it does here) followed by two spaces.  (So... capitalize "auto" and add an extra space after the period.)

> Aside from this nit, your patch looks good to me.

>> +        run_once = true;
>>           next_arg++;
>>           break;
>>         }
>> ```
>> ---
>>

Re: [RFC] [PATCH] [PR gdb/29796] Fix gdbserver delay before quitting in extended-remote

[Kevin Buettner]

On Sun, 15 Oct 2023 16:33:16 +0000
Shivam Saxena <saxens12@mcmaster.ca> wrote:

> >> Investigating PR29796, where it was noticed that there was a delay when trying to quit the gdb session if connected to gdbserver via stdio. It was also mentioned that using --once​ when launching gdbserver​ would fix this problem and a recommendation was made to always apply --once​ when launching gdbserver​ with stdio​.
> >>
> >> I've explicitly set the run_once​ flag when stdio​ is detected at launch of gdbserver. This does fix the problem as suggested in the bug since the conditional at gdbserver/server.cc:3958​ is always triggered.
> >> ```
> >>          if (run_once || (!extended_protocol && !target_running ()))
> >>             throw_quit ("Quit");
> >> ```
> >> However, would it be better to create a new flag for this to 
> >> facilitate quitting despite extended_protocol​ instead of piggybacking on --once​ logic?  
> 
> > I don't see a problem with using 'run_once' for this purpose.  But if there's something I'm missing, please explain...  
> 
> By the conditional it seems the intent was to not allow quitting immediately if in extended protocol mode. We are adding another condition to quit - if launched in stdio. But nothing in this logic alone would make that apparent. (no comment, no flag). I thought this reduced traceability. But I might be overthinking things. So if the fix seems fine for this purpose I'll fix the patch and reply. It seems I might have to use a different email account to submit the patch via git send-email. 

I think it's fine.

(But don't forget to fix the nit that I found regarding the comment.)

Kevin

Re: [RFC] [PATCH] [PR gdb/29796] Fix gdbserver delay before quitting in extended-remote

[Tom Tromey]

>>>>> "Shivam" == Shivam Saxena <saxens12@mcmaster.ca> writes:

Shivam> I've explicitly set the run_once​ flag when stdio​ is detected at
Shivam> launch of gdbserver. This does fix the problem as suggested in
Shivam> the bug since the conditional at gdbserver/server.cc:3958​ is
Shivam> always triggered.

This patch probably should also update the documentation.

thanks,
Tom