public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Shivam Saxena <saxens12@mcmaster.ca>
To: Kevin Buettner <kevinb@redhat.com>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: RE: [RFC] [PATCH] [PR gdb/29796] Fix gdbserver delay before quitting in extended-remote
Date: Sun, 15 Oct 2023 16:33:16 +0000	[thread overview]
Message-ID: <YT2PR01MB87419CC6586738F2735C60F7D5D0A@YT2PR01MB8741.CANPRD01.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20231014195236.2b0241c9@f37-zws-nv>

>> 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;
>>         }
>> ```
>> ---
>>


  reply	other threads:[~2023-10-15 16:33 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <YT2PR01MB8741E0CC1A99F17F60674D14D5D1A@YT2PR01MB8741.CANPRD01.PROD.OUTLOOK.COM>
2023-10-15  2:53 ` Kevin Buettner
2023-10-15 16:33   ` Shivam Saxena [this message]
2023-10-17  1:47     ` Kevin Buettner
2023-10-17 13:38 ` Tom Tromey

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YT2PR01MB87419CC6586738F2735C60F7D5D0A@YT2PR01MB8741.CANPRD01.PROD.OUTLOOK.COM \
    --to=saxens12@mcmaster.ca \
    --cc=gdb-patches@sourceware.org \
    --cc=kevinb@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).