[PATCH 0/2] Copy inferior properties on inferior-clone

[PATCH 0/2] Copy inferior properties on inferior-clone

[Lancelot SIX]

When a user clones an inferior using the 'inferior-clone' command, he
looses any configuration that can influence the behavior of the program
being run.  The following session illustrates this:

    gdb -nx -quiet -batch \
         -ex "show args" \
         -ex "show inferior-tty" \
         -ex "clone-inferior" \
         -ex "inferior 2" \
         -ex "show args" \
         -ex "show inferior-tty" \
         -tty=/some/tty \
         -args echo foo bar
    Argument list to give program being debugged when it is started is "foo bar".
    Terminal for future runs of program being debugged is "/some/tty".
    [New inferior 2]
    Added inferior 2.
    [Switching to inferior 2 [<null>] (/bin/echo)]
    Argument list to give program being debugged when it is started is "".
    Terminal for future runs of program being debugged is "".


In this series, I propose to change this by ensuring that the
clone-inferior command copies all relevant information form the original
inferior to the new one.  The properties this series currently copies
are:
  - args
  - cwd
  - tty
  - user set environment variables

This is done in patch 1.  Patch 2 adds an observable to notify attached
observers for each cloned inferior.  This patch allows target dependent
code to react to the clone-inferior event and perform any target-related
action they see fit.  At the moment, this is not used in upstream GDB,
but this extension mechanism will be used in downstream extensions of
gdb usch as the ROCm GDB[1].

All feedback are welcome.

Tested on x86_64-linux.

[1] https://github.com/ROCm-Developer-Tools/ROCgdb 

Lancelot SIX (2):
  gdb: Copy inferior properties in clone-inferior
  gdb: Add new inferior_cloned observable

 gdb/NEWS                                  |  7 ++
 gdb/doc/gdb.texinfo                       |  8 ++-
 gdb/inferior.c                            | 20 ++++++
 gdb/observable.c                          |  1 +
 gdb/observable.h                          |  6 ++
 gdb/testsuite/gdb.base/inferior-clone.exp | 86 +++++++++++++++++++++++
 6 files changed, 126 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/inferior-clone.exp

-- 
2.25.1

[PATCH 1/2] gdb: Copy inferior properties in clone-inferior

[Lancelot SIX]

This commit ensures that the following settings are cloned from one
inferior to the new one when processing the clone-inferior command:
  - inferior-tty
  - environment variables
  - cwd
  - args

Some of those parameters can be passed as command line arguments to GDB
(-args and -tty), so one could expect the clone-inferior to respect
those flags.  The following debugging session illustrates that:

    gdb -nx -quiet -batch \
         -ex "show args" \
         -ex "show inferior-tty" \
         -ex "clone-inferior" \
         -ex "inferior 2" \
         -ex "show args" \
         -ex "show inferior-tty" \
         -tty=/some/tty \
         -args echo foo bar
    Argument list to give program being debugged when it is started is "foo bar".
    Terminal for future runs of program being debugged is "/some/tty".
    [New inferior 2]
    Added inferior 2.
    [Switching to inferior 2 [<null>] (/bin/echo)]
    Argument list to give program being debugged when it is started is "".
    Terminal for future runs of program being debugged is "".

The other properties this commit copies on clone (i.e. CWD and the
environment variables) are included since they are related (in the sense
that they influence the runtime behavior of the program) even if they
cannot be directly set using command line switches.

There is a chance that this patch changes existing user workflow.  I
think that this change is mostly harmless.  If users wants to start a new
inferior based on an existing one, they probably already propagate those
settings to the new inferior in some way.

Tested on x86_64-linux.

Change-Id: I3b1f28b662f246228b37bb24c2ea1481567b363d
---
 gdb/NEWS                                  |  7 ++
 gdb/doc/gdb.texinfo                       |  8 ++-
 gdb/inferior.c                            | 16 +++++
 gdb/testsuite/gdb.base/inferior-clone.exp | 86 +++++++++++++++++++++++
 4 files changed, 115 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/inferior-clone.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 13b66286876..f4ed7d2c97e 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -53,6 +53,13 @@ maint packet
   as escaped hex, e.g. \x?? where '??' is replaces with the value of
   the non-printable character.
 
+clone-inferior
+  The clone-inferior command now ensures that the TTY, CMD and ARGS
+  settings are copied from the original inferior to the new one.
+  All modifications to  the environment variables done using the 'set
+  environment' or 'unset environment' commands are also copied to the new
+  inferior.
+
 * Python API
 
   ** New function gdb.add_history(), which takes a gdb.Value object
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index d0c5bcf18e1..cb68d6193f2 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3329,8 +3329,12 @@ use @code{run} to spawn a local program, etc.
 @item clone-inferior [ -copies @var{n} ] [ @var{infno} ]
 Adds @var{n} inferiors ready to execute the same program as inferior
 @var{infno}; @var{n} defaults to 1, and @var{infno} defaults to the
-number of the current inferior.  This is a convenient command when you
-want to run another instance of the inferior you are debugging.
+number of the current inferior.  This command copies the values of the
+@var{args}, @w{@var{inferior-tty}} and @var{cwd} properties from the
+current inferior to the new one.  It also propagates changes the user
+made to environment variables using the @w{@code{set environment}} and
+@w{@code{unset environment}} commands.  This is a convenient command
+when you want to run another instance of the inferior you are debugging.
 
 @smallexample
 (@value{GDBP}) info inferiors
diff --git a/gdb/inferior.c b/gdb/inferior.c
index 40ab3e7e90b..8fbc4109178 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -932,6 +932,22 @@ clone_inferior_command (const char *args, int from_tty)
 	copy_inferior_target_desc_info (inf, orginf);
 
       clone_program_space (pspace, orginf->pspace);
+
+      /* Copy properties from the original inferior to the new one.  */
+      inf->set_args (orginf->args ());
+      inf->set_cwd (orginf->cwd ());
+      inf->set_tty (orginf->tty ());
+      for (auto set_var : orginf->environment.user_set_env ())
+	{
+	  /* set_var has the form NAME=value.  Split on the first '='.  */
+	  const auto pos = set_var.find ('=');
+	  gdb_assert (pos != std::string::npos);
+	  const auto varname = set_var.substr (0, pos).c_str ();
+	  inf->environment.set
+	    (varname, orginf->environment.get (varname));
+	}
+      for (auto unset_var : orginf->environment.user_unset_env ())
+	inf->environment.unset (unset_var.c_str ());
     }
 }
 
diff --git a/gdb/testsuite/gdb.base/inferior-clone.exp b/gdb/testsuite/gdb.base/inferior-clone.exp
new file mode 100644
index 00000000000..c8ee152d7d0
--- /dev/null
+++ b/gdb/testsuite/gdb.base/inferior-clone.exp
@@ -0,0 +1,86 @@
+# Copyright (C) 2021 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This testcase checks that the clone-inferior command copies important
+# properties from the original to the new inferior such CWD, args and env.
+
+# Add an environment variable that can be modified later for each inferior
+# in GDB.
+set env(ENVVAR) var
+
+# This test does not need a binfile.
+clean_restart
+
+# Set custom properties for Inferior 1
+gdb_test_no_output "set args foo bar 'b a z'"
+gdb_test_no_output "set cwd /any/where"
+gdb_test_no_output "set environment FOO foo"
+gdb_test_no_output "set environment BAR bar"
+gdb_test_no_output "set inferior-tty some_tty"
+
+gdb_test "clone-inferior" "Added inferior 2" "First clone-inferior"
+# Check that properties of inferior 1 have been copied
+with_test_prefix "Check inferior 2" {
+    gdb_test "inferior 2" "\[Switching to inferior 2 \[<null>\] (.*)\]" "change inferior"
+    gdb_test "show args" \
+	"Argument list to give program being debugged when it is started is \"foo bar 'b a z'\"\."
+    gdb_test "show cwd" \
+	"Current working directory that will be used when starting the inferior is \"/any/where\"\."
+    gdb_test "show environment FOO" "FOO = foo"
+    gdb_test "show environment BAR" "BAR = bar"
+    gdb_test "show environment ENVVAR" "ENVVAR = var"
+    gdb_test "show inferior-tty" "Terminal for future runs of program being debugged is \"some_tty\"\."
+}
+
+# Change this second inferior, to check that the next clone-inferior
+# clones from the active inferior, and changing the values for inferior 2
+# does not interfere with values on inferior 1.
+gdb_test_no_output "set args foo"
+gdb_test_no_output "set cwd /somewhere/else"
+gdb_test_no_output "set environment FOO oof"
+
+with_test_prefix "Check inferior 1" {
+    gdb_test "inferior 1" "\[Switching to inferior 1 \[<null>\] (.*)\]" "change inferior"
+    gdb_test "show args" \
+	"Argument list to give program being debugged when it is started is \"foo bar 'b a z'\"\."
+    gdb_test "show cwd" \
+	"Current working directory that will be used when starting the inferior is \"/any/where\"\."
+    gdb_test "show environment FOO" "FOO = foo"
+    gdb_test "show environment BAR" "BAR = bar"
+    gdb_test "show environment ENVVAR" "ENVVAR = var"
+    gdb_test "show inferior-tty" \
+	"Terminal for future runs of program being debugged is \"some_tty\"\."
+}
+
+# Tweak inferior I a bit more
+gdb_test_no_output "unset environment FOO"
+gdb_test_no_output "unset environment ENVVAR"
+
+gdb_test "clone-inferior" "Added inferior 3" "Second clone-inferior"
+
+# Check that the values observed on inferior 3 are those copied from
+# inferior 1
+with_test_prefix "Check inferior 3" {
+    gdb_test "inferior 3" "\[Switching to inferior 3 \[<null>\] (.*)\]" "change inferior"
+    gdb_test "show args" \
+	"Argument list to give program being debugged when it is started is \"foo bar 'b a z'\"\."
+    gdb_test "show cwd" \
+	"Current working directory that will be used when starting the inferior is \"/any/where\"\."
+    gdb_test "show environment FOO" "Environment variable \"FOO\" not defined\."
+    gdb_test "show environment BAR" "BAR = bar"
+    gdb_test "show environment ENVVAR" "Environment variable \"ENVVAR\" not defined\."
+    gdb_test "show inferior-tty" \
+	"Terminal for future runs of program being debugged is \"some_tty\"\."
+}
-- 
2.25.1

[PATCH 2/2] gdb: Add new inferior_cloned observable

[Lancelot SIX]

Add inferior_cloned in gdb::observers.  Callbacks called when this event
is triggered receive a pointer to the original inferior as well as a
pointer to the newly created inferior.

This event is triggered in clone_inferior_command.

This observable is not yet used in core GDB but is added as an extension
mechanism so target dependent codes can react to the clone-inferior
event if necessary.

Tested on x86_64-linux.

Change-Id: I4b384fd489cfca56bc63c3e4256992c991d0fbed
---
 gdb/inferior.c   | 4 ++++
 gdb/observable.c | 1 +
 gdb/observable.h | 6 ++++++
 3 files changed, 11 insertions(+)

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 8fbc4109178..1b70f1e7a00 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -948,6 +948,10 @@ clone_inferior_command (const char *args, int from_tty)
 	}
       for (auto unset_var : orginf->environment.user_unset_env ())
 	inf->environment.unset (unset_var.c_str ());
+
+      /* Notify that the inferior has been cloned so target dependent logic
+	 can be applied if necessary.  */
+      gdb::observers::inferior_cloned.notify (orginf, inf);
     }
 }
 
diff --git a/gdb/observable.c b/gdb/observable.c
index fe88b0bf702..740c50613d0 100644
--- a/gdb/observable.c
+++ b/gdb/observable.c
@@ -64,6 +64,7 @@ DEFINE_OBSERVABLE (inferior_added);
 DEFINE_OBSERVABLE (inferior_appeared);
 DEFINE_OBSERVABLE (inferior_exit);
 DEFINE_OBSERVABLE (inferior_removed);
+DEFINE_OBSERVABLE (inferior_cloned);
 DEFINE_OBSERVABLE (memory_changed);
 DEFINE_OBSERVABLE (before_prompt);
 DEFINE_OBSERVABLE (gdb_datadir_changed);
diff --git a/gdb/observable.h b/gdb/observable.h
index d3c441dc764..29a00cfacef 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -188,6 +188,12 @@ extern observable<struct inferior */* inf */> inferior_exit;
    This method is called immediately before freeing INF.  */
 extern observable<struct inferior */* inf */> inferior_removed;
 
+/* The inferior CLONE has been created by cloning INF.
+   This method is called after all required operations for the clone have
+   been performed.  */
+extern observable<struct inferior */* inf */, struct inferior */* clone */>
+    inferior_cloned;
+
 /* Bytes from DATA to DATA + LEN have been written to the inferior
    at ADDR.  */
 extern observable<struct inferior */* inferior */, CORE_ADDR /* addr */,
-- 
2.25.1

RE: [PATCH 1/2] gdb: Copy inferior properties in clone-inferior

[Aktemur, Tankut Baris]

On Tuesday, December 14, 2021 12:02 PM, Lancelot SIX wrote:
> This commit ensures that the following settings are cloned from one
> inferior to the new one when processing the clone-inferior command:
>   - inferior-tty
>   - environment variables
>   - cwd
>   - args
> 
> Some of those parameters can be passed as command line arguments to GDB
> (-args and -tty), so one could expect the clone-inferior to respect
> those flags.  The following debugging session illustrates that:
> 
>     gdb -nx -quiet -batch \
>          -ex "show args" \
>          -ex "show inferior-tty" \
>          -ex "clone-inferior" \
>          -ex "inferior 2" \
>          -ex "show args" \
>          -ex "show inferior-tty" \
>          -tty=/some/tty \
>          -args echo foo bar
>     Argument list to give program being debugged when it is started is "foo bar".
>     Terminal for future runs of program being debugged is "/some/tty".
>     [New inferior 2]
>     Added inferior 2.
>     [Switching to inferior 2 [<null>] (/bin/echo)]
>     Argument list to give program being debugged when it is started is "".
>     Terminal for future runs of program being debugged is "".
> 
> The other properties this commit copies on clone (i.e. CWD and the
> environment variables) are included since they are related (in the sense
> that they influence the runtime behavior of the program) even if they
> cannot be directly set using command line switches.
> 
> There is a chance that this patch changes existing user workflow.  I
> think that this change is mostly harmless.  If users wants to start a new

wants -> want

> inferior based on an existing one, they probably already propagate those
> settings to the new inferior in some way.
> 
> Tested on x86_64-linux.
> 
> Change-Id: I3b1f28b662f246228b37bb24c2ea1481567b363d
> ---
>  gdb/NEWS                                  |  7 ++
>  gdb/doc/gdb.texinfo                       |  8 ++-
>  gdb/inferior.c                            | 16 +++++
>  gdb/testsuite/gdb.base/inferior-clone.exp | 86 +++++++++++++++++++++++
>  4 files changed, 115 insertions(+), 2 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/inferior-clone.exp
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 13b66286876..f4ed7d2c97e 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -53,6 +53,13 @@ maint packet
>    as escaped hex, e.g. \x?? where '??' is replaces with the value of
>    the non-printable character.
> 
> +clone-inferior
> +  The clone-inferior command now ensures that the TTY, CMD and ARGS
> +  settings are copied from the original inferior to the new one.
> +  All modifications to  the environment variables done using the 'set

Extra space between 'to' and 'the'.

> +  environment' or 'unset environment' commands are also copied to the new
> +  inferior.
> +
>  * Python API
> 
>    ** New function gdb.add_history(), which takes a gdb.Value object
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index d0c5bcf18e1..cb68d6193f2 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -3329,8 +3329,12 @@ use @code{run} to spawn a local program, etc.
>  @item clone-inferior [ -copies @var{n} ] [ @var{infno} ]
>  Adds @var{n} inferiors ready to execute the same program as inferior
>  @var{infno}; @var{n} defaults to 1, and @var{infno} defaults to the
> -number of the current inferior.  This is a convenient command when you
> -want to run another instance of the inferior you are debugging.
> +number of the current inferior.  This command copies the values of the
> +@var{args}, @w{@var{inferior-tty}} and @var{cwd} properties from the
> +current inferior to the new one.  It also propagates changes the user
> +made to environment variables using the @w{@code{set environment}} and
> +@w{@code{unset environment}} commands.  This is a convenient command
> +when you want to run another instance of the inferior you are debugging.
> 
>  @smallexample
>  (@value{GDBP}) info inferiors
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index 40ab3e7e90b..8fbc4109178 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -932,6 +932,22 @@ clone_inferior_command (const char *args, int from_tty)
>  	copy_inferior_target_desc_info (inf, orginf);
> 
>        clone_program_space (pspace, orginf->pspace);
> +
> +      /* Copy properties from the original inferior to the new one.  */
> +      inf->set_args (orginf->args ());
> +      inf->set_cwd (orginf->cwd ());
> +      inf->set_tty (orginf->tty ());
> +      for (auto set_var : orginf->environment.user_set_env ())
> +	{
> +	  /* set_var has the form NAME=value.  Split on the first '='.  */
> +	  const auto pos = set_var.find ('=');
> +	  gdb_assert (pos != std::string::npos);
> +	  const auto varname = set_var.substr (0, pos).c_str ();
> +	  inf->environment.set
> +	    (varname, orginf->environment.get (varname));
> +	}
> +      for (auto unset_var : orginf->environment.user_unset_env ())
> +	inf->environment.unset (unset_var.c_str ());
>      }
>  }
> 
> diff --git a/gdb/testsuite/gdb.base/inferior-clone.exp b/gdb/testsuite/gdb.base/inferior-
> clone.exp
> new file mode 100644
> index 00000000000..c8ee152d7d0
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/inferior-clone.exp
> @@ -0,0 +1,86 @@
> +# Copyright (C) 2021 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This testcase checks that the clone-inferior command copies important
> +# properties from the original to the new inferior such CWD, args and env.
> +
> +# Add an environment variable that can be modified later for each inferior
> +# in GDB.
> +set env(ENVVAR) var
> +
> +# This test does not need a binfile.
> +clean_restart
> +
> +# Set custom properties for Inferior 1
> +gdb_test_no_output "set args foo bar 'b a z'"
> +gdb_test_no_output "set cwd /any/where"
> +gdb_test_no_output "set environment FOO foo"
> +gdb_test_no_output "set environment BAR bar"
> +gdb_test_no_output "set inferior-tty some_tty"
> +
> +gdb_test "clone-inferior" "Added inferior 2" "First clone-inferior"
> +# Check that properties of inferior 1 have been copied
> +with_test_prefix "Check inferior 2" {
> +    gdb_test "inferior 2" "\[Switching to inferior 2 \[<null>\] (.*)\]" "change inferior"
> +    gdb_test "show args" \
> +	"Argument list to give program being debugged when it is started is \"foo bar 'b a
> z'\"\."
> +    gdb_test "show cwd" \
> +	"Current working directory that will be used when starting the inferior is
> \"/any/where\"\."
> +    gdb_test "show environment FOO" "FOO = foo"
> +    gdb_test "show environment BAR" "BAR = bar"
> +    gdb_test "show environment ENVVAR" "ENVVAR = var"
> +    gdb_test "show inferior-tty" "Terminal for future runs of program being debugged is
> \"some_tty\"\."
> +}
> +
> +# Change this second inferior, to check that the next clone-inferior
> +# clones from the active inferior, and changing the values for inferior 2
> +# does not interfere with values on inferior 1.
> +gdb_test_no_output "set args foo"
> +gdb_test_no_output "set cwd /somewhere/else"
> +gdb_test_no_output "set environment FOO oof"
> +
> +with_test_prefix "Check inferior 1" {
> +    gdb_test "inferior 1" "\[Switching to inferior 1 \[<null>\] (.*)\]" "change inferior"
> +    gdb_test "show args" \
> +	"Argument list to give program being debugged when it is started is \"foo bar 'b a
> z'\"\."
> +    gdb_test "show cwd" \
> +	"Current working directory that will be used when starting the inferior is
> \"/any/where\"\."
> +    gdb_test "show environment FOO" "FOO = foo"
> +    gdb_test "show environment BAR" "BAR = bar"
> +    gdb_test "show environment ENVVAR" "ENVVAR = var"
> +    gdb_test "show inferior-tty" \
> +	"Terminal for future runs of program being debugged is \"some_tty\"\."
> +}
> +
> +# Tweak inferior I a bit more

I -> 1, and period at the end for consistency.

> +gdb_test_no_output "unset environment FOO"
> +gdb_test_no_output "unset environment ENVVAR"
> +
> +gdb_test "clone-inferior" "Added inferior 3" "Second clone-inferior"
> +
> +# Check that the values observed on inferior 3 are those copied from
> +# inferior 1

Period at the end for consistency.

Thanks.

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 2/2] gdb: Add new inferior_cloned observable

[Aktemur, Tankut Baris]

On Tuesday, December 14, 2021 12:02 PM, Lancelot SIX wrote:
> Add inferior_cloned in gdb::observers.  Callbacks called when this event
> is triggered receive a pointer to the original inferior as well as a
> pointer to the newly created inferior.
> 
> This event is triggered in clone_inferior_command.
> 
> This observable is not yet used in core GDB but is added as an extension
> mechanism so target dependent codes can react to the clone-inferior
> event if necessary.
> 
> Tested on x86_64-linux.
> 
> Change-Id: I4b384fd489cfca56bc63c3e4256992c991d0fbed
> ---
>  gdb/inferior.c   | 4 ++++
>  gdb/observable.c | 1 +
>  gdb/observable.h | 6 ++++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index 8fbc4109178..1b70f1e7a00 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -948,6 +948,10 @@ clone_inferior_command (const char *args, int from_tty)
>  	}
>        for (auto unset_var : orginf->environment.user_unset_env ())
>  	inf->environment.unset (unset_var.c_str ());
> +
> +      /* Notify that the inferior has been cloned so target dependent logic
> +	 can be applied if necessary.  */
> +      gdb::observers::inferior_cloned.notify (orginf, inf);
>      }
>  }
> 
> diff --git a/gdb/observable.c b/gdb/observable.c
> index fe88b0bf702..740c50613d0 100644
> --- a/gdb/observable.c
> +++ b/gdb/observable.c
> @@ -64,6 +64,7 @@ DEFINE_OBSERVABLE (inferior_added);
>  DEFINE_OBSERVABLE (inferior_appeared);
>  DEFINE_OBSERVABLE (inferior_exit);
>  DEFINE_OBSERVABLE (inferior_removed);
> +DEFINE_OBSERVABLE (inferior_cloned);
>  DEFINE_OBSERVABLE (memory_changed);
>  DEFINE_OBSERVABLE (before_prompt);
>  DEFINE_OBSERVABLE (gdb_datadir_changed);
> diff --git a/gdb/observable.h b/gdb/observable.h
> index d3c441dc764..29a00cfacef 100644
> --- a/gdb/observable.h
> +++ b/gdb/observable.h
> @@ -188,6 +188,12 @@ extern observable<struct inferior */* inf */> inferior_exit;
>     This method is called immediately before freeing INF.  */
>  extern observable<struct inferior */* inf */> inferior_removed;
> 
> +/* The inferior CLONE has been created by cloning INF.
> +   This method is called after all required operations for the clone have
> +   been performed.  */
> +extern observable<struct inferior */* inf */, struct inferior */* clone */>
> +    inferior_cloned;

As far as I can tell, the tendency is to omit the redundant 'struct' keyword in
newly-written code.

Thanks.
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH 1/2] gdb: Copy inferior properties in clone-inferior

[Six, Lancelot]

[AMD Official Use Only]

Hi,

>>
>> There is a chance that this patch changes existing user workflow.  I 
>> think that this change is mostly harmless.  If users wants to start a 
>> new
> 
> wants -> want

Done

>>
>> +clone-inferior
>> +  The clone-inferior command now ensures that the TTY, CMD and ARGS
>> +  settings are copied from the original inferior to the new one.
>> +  All modifications to  the environment variables done using the 'set
>
> Extra space between 'to' and 'the'.

Done

>> +
>> +# Tweak inferior I a bit more
>
> I -> 1, and period at the end for consistency.

Done

>> +# Check that the values observed on inferior 3 are those copied from 
>> +# inferior 1
>
> Period at the end for consistency.

Done

> Thanks.
> 
> -Baris

I have fixed all the problems you pointed out in my local copy.
Thanks for the review!

Best,
Lancelot.

RE: [PATCH 2/2] gdb: Add new inferior_cloned observable

[Six, Lancelot]

[AMD Official Use Only]

>> +/* The inferior CLONE has been created by cloning INF.
>> +   This method is called after all required operations for the clone have
>> +   been performed.  */
>> +extern observable<struct inferior */* inf */, struct inferior */* clone */>
>> +    inferior_cloned;
>
> As far as I can tell, the tendency is to omit the redundant 'struct' keyword in newly-written code.

You are right.  I kept the struct keyword here because it seamed to be consistently used across the file, but it is indeed not necessary.

I have removed it in my local copy.

Thanks for the review.
Best,
Lancelot.

>
> Thanks.
> -Baris

Re: [PATCH 2/2] gdb: Add new inferior_cloned observable

[Pedro Alves]

On 2021-12-14 11:02, Lancelot SIX via Gdb-patches wrote:
> Add inferior_cloned in gdb::observers.  Callbacks called when this event
> is triggered receive a pointer to the original inferior as well as a
> pointer to the newly created inferior.
> 
> This event is triggered in clone_inferior_command.
> 
> This observable is not yet used in core GDB but is added as an extension
> mechanism so target dependent codes can react to the clone-inferior
> event if necessary.
> 
> Tested on x86_64-linux.

It there an upstream target that is going to make use of this very soon?

If not, let's not add unused code to upstream GDB.  Think of it this way -- when people
run into unused code, say, an unused observer, they'll rip it out.  You'd have to
play police to prevent people from doing that.  You could add a comment explaining
it is only useful for some downstream, but then, who's to say that in a couple
years downstream changes and stops using the observer?  It creates unfair burden
on upstream maintainers.

We can upstream this alongside its first user.

Pedro Alves

> 
> Change-Id: I4b384fd489cfca56bc63c3e4256992c991d0fbed
> ---
>  gdb/inferior.c   | 4 ++++
>  gdb/observable.c | 1 +
>  gdb/observable.h | 6 ++++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/gdb/inferior.c b/gdb/inferior.c
> index 8fbc4109178..1b70f1e7a00 100644
> --- a/gdb/inferior.c
> +++ b/gdb/inferior.c
> @@ -948,6 +948,10 @@ clone_inferior_command (const char *args, int from_tty)
>  	}
>        for (auto unset_var : orginf->environment.user_unset_env ())
>  	inf->environment.unset (unset_var.c_str ());
> +
> +      /* Notify that the inferior has been cloned so target dependent logic
> +	 can be applied if necessary.  */
> +      gdb::observers::inferior_cloned.notify (orginf, inf);
>      }
>  }
>  
> diff --git a/gdb/observable.c b/gdb/observable.c
> index fe88b0bf702..740c50613d0 100644
> --- a/gdb/observable.c
> +++ b/gdb/observable.c
> @@ -64,6 +64,7 @@ DEFINE_OBSERVABLE (inferior_added);
>  DEFINE_OBSERVABLE (inferior_appeared);
>  DEFINE_OBSERVABLE (inferior_exit);
>  DEFINE_OBSERVABLE (inferior_removed);
> +DEFINE_OBSERVABLE (inferior_cloned);
>  DEFINE_OBSERVABLE (memory_changed);
>  DEFINE_OBSERVABLE (before_prompt);
>  DEFINE_OBSERVABLE (gdb_datadir_changed);
> diff --git a/gdb/observable.h b/gdb/observable.h
> index d3c441dc764..29a00cfacef 100644
> --- a/gdb/observable.h
> +++ b/gdb/observable.h
> @@ -188,6 +188,12 @@ extern observable<struct inferior */* inf */> inferior_exit;
>     This method is called immediately before freeing INF.  */
>  extern observable<struct inferior */* inf */> inferior_removed;
>  
> +/* The inferior CLONE has been created by cloning INF.
> +   This method is called after all required operations for the clone have
> +   been performed.  */
> +extern observable<struct inferior */* inf */, struct inferior */* clone */>
> +    inferior_cloned;
> +
>  /* Bytes from DATA to DATA + LEN have been written to the inferior
>     at ADDR.  */
>  extern observable<struct inferior */* inferior */, CORE_ADDR /* addr */,
> 

Re: [PATCH 1/2] gdb: Copy inferior properties in clone-inferior

[Pedro Alves]

On 2021-12-14 11:02, Lancelot SIX via Gdb-patches wrote:

> @@ -932,6 +932,22 @@ clone_inferior_command (const char *args, int from_tty)
>  	copy_inferior_target_desc_info (inf, orginf);
>  
>        clone_program_space (pspace, orginf->pspace);
> +
> +      /* Copy properties from the original inferior to the new one.  */
> +      inf->set_args (orginf->args ());
> +      inf->set_cwd (orginf->cwd ());
> +      inf->set_tty (orginf->tty ());
> +      for (auto set_var : orginf->environment.user_set_env ())

Unnecessary copying here, could use const ref.  Please write:

      for (const std::string &set_var : orginf->environment.user_set_env ())

> +	{
> +	  /* set_var has the form NAME=value.  Split on the first '='.  */
> +	  const auto pos = set_var.find ('=');
> +	  gdb_assert (pos != std::string::npos);
> +	  const auto varname = set_var.substr (0, pos).c_str ();

std::string::substr returns a new string.  So here the temporary string
dies at the end of the expression, leaving you with a dangling pointer.

Please name the std::string type.

> +	  inf->environment.set
> +	    (varname, orginf->environment.get (varname));
> +	}
> +      for (auto unset_var : orginf->environment.user_unset_env ())
> +	inf->environment.unset (unset_var.c_str ());
>      }
>  }
>  


> +
> +# Set custom properties for Inferior 1
> +gdb_test_no_output "set args foo bar 'b a z'"
> +gdb_test_no_output "set cwd /any/where"
> +gdb_test_no_output "set environment FOO foo"
> +gdb_test_no_output "set environment BAR bar"
> +gdb_test_no_output "set inferior-tty some_tty"
> +
> +gdb_test "clone-inferior" "Added inferior 2" "First clone-inferior"

Please use lowercase in test names.  It's what 99% of the tests use, and mixing
upper/lower just leads to a mix salad, unnecessarily.

> +# Check that properties of inferior 1 have been copied
> +with_test_prefix "Check inferior 2" {

Ditto.  I always prefer to avoid saying "check" or "test", as they're redundant -- these are
all tests&checks by definition.  So just "inferior 2" would be fine.

> +    gdb_test "inferior 2" "\[Switching to inferior 2 \[<null>\] (.*)\]" "change inferior"
> +    gdb_test "show args" \
> +	"Argument list to give program being debugged when it is started is \"foo bar 'b a z'\"\."
> +    gdb_test "show cwd" \
> +	"Current working directory that will be used when starting the inferior is \"/any/where\"\."
> +    gdb_test "show environment FOO" "FOO = foo"
> +    gdb_test "show environment BAR" "BAR = bar"
> +    gdb_test "show environment ENVVAR" "ENVVAR = var"
> +    gdb_test "show inferior-tty" "Terminal for future runs of program being debugged is \"some_tty\"\."
> +}
> +
> +# Change this second inferior, to check that the next clone-inferior
> +# clones from the active inferior, and changing the values for inferior 2
> +# does not interfere with values on inferior 1.
> +gdb_test_no_output "set args foo"
> +gdb_test_no_output "set cwd /somewhere/else"
> +gdb_test_no_output "set environment FOO oof"

Just a small nit, feel free to ignore this one:

Wouldn't these be better under the test prefix still?  Would make it obvious which
inferior the commands apply to.  I guess I see why you'd want them split out given
the "check" in the prefix.  But without, I think they'd fit right in.

> +
> +with_test_prefix "Check inferior 1" {

Ditto.

> +    gdb_test "inferior 1" "\[Switching to inferior 1 \[<null>\] (.*)\]" "change inferior"
> +    gdb_test "show args" \
> +	"Argument list to give program being debugged when it is started is \"foo bar 'b a z'\"\."
> +    gdb_test "show cwd" \
> +	"Current working directory that will be used when starting the inferior is \"/any/where\"\."
> +    gdb_test "show environment FOO" "FOO = foo"
> +    gdb_test "show environment BAR" "BAR = bar"
> +    gdb_test "show environment ENVVAR" "ENVVAR = var"
> +    gdb_test "show inferior-tty" \
> +	"Terminal for future runs of program being debugged is \"some_tty\"\."
> +}
> +
> +# Tweak inferior I a bit more
> +gdb_test_no_output "unset environment FOO"
> +gdb_test_no_output "unset environment ENVVAR"
> +
> +gdb_test "clone-inferior" "Added inferior 3" "Second clone-inferior"

Lowercase.

> +
> +# Check that the values observed on inferior 3 are those copied from
> +# inferior 1
> +with_test_prefix "Check inferior 3" {

Ditto.

> +    gdb_test "inferior 3" "\[Switching to inferior 3 \[<null>\] (.*)\]" "change inferior"
> +    gdb_test "show args" \
> +	"Argument list to give program being debugged when it is started is \"foo bar 'b a z'\"\."
> +    gdb_test "show cwd" \
> +	"Current working directory that will be used when starting the inferior is \"/any/where\"\."
> +    gdb_test "show environment FOO" "Environment variable \"FOO\" not defined\."
> +    gdb_test "show environment BAR" "BAR = bar"
> +    gdb_test "show environment ENVVAR" "Environment variable \"ENVVAR\" not defined\."
> +    gdb_test "show inferior-tty" \
> +	"Terminal for future runs of program being debugged is \"some_tty\"\."
> +}
>