public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Andrew Burgess <aburgess@redhat.com>
To: Muhammad Umair Sair <umair_sair@hotmail.com>,
	"gdb-patches\@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: Making mi -add-inferior command consistent with ci
Date: Wed, 23 Feb 2022 14:37:01 +0000	[thread overview]
Message-ID: <87fso9bqeq.fsf@redhat.com> (raw)
In-Reply-To: <OS3PR01MB6291E827D9F9DD369C8CB0A4FB3B9@OS3PR01MB6291.jpnprd01.prod.outlook.com>

Muhammad Umair Sair via Gdb-patches <gdb-patches@sourceware.org> writes:

> On 2/22/22 7:08 PM, Andrew Burgess wrote:
>> Muhammad Umair Sair via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>>>  From 3729f798d1f75a74eefe0c240d8c4c9849dac308 Mon Sep 17 00:00:00 2001
>>> From: Umair Sair <umair_sair@hotmail.com>
>>> Date: Tue, 22 Feb 2022 01:40:06 +0500
>>> Subject: [PATCH] Making mi -add-inferior command consistent with ci
>>>
>>> With multi-target debugging support in gdb, add-inferior command without
>>> arguments in console interpreter adds a new inferior and sets the connection
>>> of previously selected inferior as the connection of new inferior as well.
>>> But the same command for machine interpreter is not setting the connection
>>> for new inferior. This commit fixes this issue and makes the console and
>>> machine interpreter behavior consistent for add-inferior command.
>> 
>> We're usually pretty conservative about changing MI behaviour in non
>> backward compatible ways.  I'm not entirely sure that this change wont
>> cause problems for an existing f/e that is using -add-inferior?
>> 
>> Maybe I'm worrying about nothing?  I'm open to being convinced.
>
> The older change broke it in non backward compatibility way and this 
> commit is fixing it. I faced issues compared to older versions of gdb 
> and on investigation I found this issue. Following is the commit where 
> we supported multi-target.

Ah, OK.  I didn't get that from the commit message.  Maybe it would be
worth expanding the commit message to make it clearer that this is a
regression.  Ideally, include an example of how GDB used to behave, and
the now broken behaviour, highlighting how things have regressed.

>
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=5b6d1e4fa4fc6827c7b3f0e99ff120dfa14d65d2
>
>> 
>> Another option would be to add a new option, e.g.:
>> 
>>    -add-inferior [--connection NUM]
>> 
>> or
>> 
>>    -add-inferior [--inherit-connection]
>> 
>
> IMO we should support the same options as in console. Currently we don't 
> allow any arguments for -add-inferior MI command though console 
> add-inferior command allows several arguments. I was thinking to support 
> at least "--no-connection" argument which is also supported in console. 
> IMO we can add it separate commit and keep this commit for fixing the 
> regression. WDYS?

I'd agree that ideally the same functionality would be available via the
MI as from the CLI.  As I said in my previous commit we try very hard to
maintain backward compatibility for the MI, so I'm pretty sure there are
some MI commands where the default command behaviour is not the same as
the CLI, and there are extra/different flags for the MI to allow CLI
like behaviour.  Though I can't think of any examples off the top of my
head, so maybe I'm wrong.

I 100% agree that adding new flags should be done in a separate commit
though.

Additionally, I forgot to mention in the first reviewl; we really should
have a test for this behaviour.

Thanks,
Andrew

>
>> 
>>> ---
>>>   gdb/ChangeLog    | 9 +++++++++
>>>   gdb/inferior.c   | 4 ++--
>>>   gdb/inferior.h   | 5 ++++-
>>>   gdb/mi/mi-main.c | 4 +++-
>>>   4 files changed, 18 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
>>> index dfee3dad157..1f342cc628f 100644
>>> --- a/gdb/ChangeLog
>>> +++ b/gdb/ChangeLog
>>> @@ -1,3 +1,12 @@
>>> +2022-02-22  Umair Sair  <umair_sair@hotmail.com>
>>> +
>>> +	* gdb/inferior.c (switch_to_inferior_and_push_target): allowing
>>> +	access to this function outside of file.
>>> +	* gdb/inferior.h (switch_to_inferior_and_push_target): added extern
>>> +	entry for function in header.
>>> +	* gdb/mi/mi-main.c (mi_cmd_add_inferior): new inferior should use
>>> +	the connection from previously selected inferior.
>>> +
>> 
>> We no longer maintain ChangeLog in gdb, so this part is not needed.
>> You're welcome to keep the ChangeLog text in the commit message in
>> _addition_ to the long form description, but this is not a requirement.
>> 
>
> Yes. I actually created this patch for gdb-10-branch. Is there any 
> policy to submit patches for older releases? Or should I submit patch 
> for master?
>
>>>   2021-04-23  Tom Tromey  <tromey@adacore.com>
>>>   
>>>   	PR gdb/27743:
>>> diff --git a/gdb/inferior.c b/gdb/inferior.c
>>> index 4fdddbcb55b..16e4279bc1a 100644
>>> --- a/gdb/inferior.c
>>> +++ b/gdb/inferior.c
>>> @@ -1,6 +1,6 @@
>>>   /* Multi-process control for GDB, the GNU debugger.
>>>   
>>> -   Copyright (C) 2008-2021 Free Software Foundation, Inc.
>>> +   Copyright (C) 2008-2022 Free Software Foundation, Inc.
>>>   
>> 
>> Not sure what's going on here.  The patch shows all the dates changing,
>> but all the files you've changed already have 2022 in the upstream
>> master branch.
>
> The patch was for gdb-10-branch. I'll update patch according to the 
> responses of queries above.
>
> Thanks,
> Umair Sair
>
>> 
>> Thanks,
>> Andrew
>> 
>>>      This file is part of GDB.
>>>   
>>> @@ -740,7 +740,7 @@ add_inferior_with_spaces (void)
>>>      NO_CONNECTION is true, push the process_stratum_target of ORG_INF
>>>      to NEW_INF.  */
>>>   
>>> -static void
>>> +void
>>>   switch_to_inferior_and_push_target (inferior *new_inf,
>>>   				    bool no_connection, inferior *org_inf)
>>>   {
>>> diff --git a/gdb/inferior.h b/gdb/inferior.h
>>> index fb7d0c2bc1a..d4abd00489a 100644
>>> --- a/gdb/inferior.h
>>> +++ b/gdb/inferior.h
>>> @@ -1,7 +1,7 @@
>>>   /* Variables that describe the inferior process running under GDB:
>>>      Where it is, why it stopped, and how to step it.
>>>   
>>> -   Copyright (C) 1986-2021 Free Software Foundation, Inc.
>>> +   Copyright (C) 1986-2022 Free Software Foundation, Inc.
>>>   
>>>      This file is part of GDB.
>>>   
>>> @@ -686,4 +686,7 @@ extern struct inferior *add_inferior_with_spaces (void);
>>>   /* Print the current selected inferior.  */
>>>   extern void print_selected_inferior (struct ui_out *uiout);
>>>   
>>> +extern void switch_to_inferior_and_push_target (inferior *new_inf,
>>> +            bool no_connection, inferior *org_inf);
>>> +
>>>   #endif /* !defined (INFERIOR_H) */
>>> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
>>> index e38520a98e6..65c0aa5be0b 100644
>>> --- a/gdb/mi/mi-main.c
>>> +++ b/gdb/mi/mi-main.c
>>> @@ -1,6 +1,6 @@
>>>   /* MI Command Set.
>>>   
>>> -   Copyright (C) 2000-2021 Free Software Foundation, Inc.
>>> +   Copyright (C) 2000-2022 Free Software Foundation, Inc.
>>>   
>>>      Contributed by Cygnus Solutions (a Red Hat company).
>>>   
>>> @@ -1702,6 +1702,8 @@ mi_cmd_add_inferior (const char *command, char **argv, int argc)
>>>   
>>>     inf = add_inferior_with_spaces ();
>>>   
>>> +  switch_to_inferior_and_push_target (inf, false, current_inferior ());
>>> +
>>>     current_uiout->field_fmt ("inferior", "i%d", inf->num);
>>>   }
>>>   
>>> -- 
>>> 2.31.1
>> 


  reply	other threads:[~2022-02-23 14:37 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-21 21:08 Muhammad Umair Sair
2022-02-22 14:08 ` Andrew Burgess
2022-02-22 16:56   ` Muhammad Umair Sair
2022-02-23 14:37     ` Andrew Burgess [this message]
2022-02-24 17:27       ` Muhammad Umair Sair

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=87fso9bqeq.fsf@redhat.com \
    --to=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=umair_sair@hotmail.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).