[PATCH 1/5] gdb: make target_close check that the target isn't pushed in all inferiors

[PATCH 1/5] gdb: make target_close check that the target isn't pushed in all inferiors

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

The target_close function currently checks that the target to be closed
isn't pushed in the current inferior:

    gdb_assert (!current_inferior ()->target_is_pushed (targ));

When a target is closed, it's normally because its refcount has dropped
to 0, because it's not used in any inferior anymore.  I think it would
make sense to change that assert to not only check in the current
inferior, but to check in all inferiors.  It would be quite bad (and a
bug) to close a target while it's still pushed in one of the non-current
inferiors.

gdb/ChangeLog:

	* target.c (target_close): Check in all inferiors that the
	target is not pushed.

Change-Id: I6e37fc3f3476a0593da1e476604642b2de90f1d5
---
 gdb/target.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/target.c b/gdb/target.c
index 1f0741471d82..00f0acde7586 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -3734,7 +3734,8 @@ debug_target::info () const
 void
 target_close (struct target_ops *targ)
 {
-  gdb_assert (!current_inferior ()->target_is_pushed (targ));
+  for (inferior *inf : all_inferiors ())
+    gdb_assert (!inf->target_is_pushed (targ));
 
   fileio_handles_invalidate_target (targ);
 
-- 
2.30.1

[PATCH 2/5] gdb: remove reference to current inferior in target_stack::unpush

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

target_stack::unpush needs to get the target beneath the target being
unpushed to update the m_top field (which keeps the stratum of the
top-most target).  It currently does so using target_ops::beneath, which
uses the target stack of the current inferior.  The target stack of the
current inferior is the same as the `this` in the unpush method.

Avoid this detour and remove this reference to the current inferior by
calling target_ops::find_beneath and passing `this` to find the target
beneath `t` in the target stack that is `this`.

gdb/ChangeLog:

	* target.c (target_stack::unpush): Call target_ops::find_beneath
	to get the target beneath `t`.

Change-Id: If9d9661567c5c16f655d270bd2ec9f1b3aa6dadc
---
 gdb/target.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/target.c b/gdb/target.c
index 00f0acde7586..78327a2a6c33 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1214,7 +1214,7 @@ target_stack::unpush (target_ops *t)
   m_stack[stratum] = NULL;
 
   if (m_top == stratum)
-    m_top = t->beneath ()->stratum ();
+    m_top = this->find_beneath (t)->stratum ();
 
   /* Finally close the target, if there are no inferiors
      referencing this target still.  Note we do this after unchaining,
-- 
2.30.1

Re: [PATCH 2/5] gdb: remove reference to current inferior in target_stack::unpush

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-06 10:28:03 -0400]:

> From: Simon Marchi <simon.marchi@efficios.com>
> 
> target_stack::unpush needs to get the target beneath the target being
> unpushed to update the m_top field (which keeps the stratum of the
> top-most target).  It currently does so using target_ops::beneath, which
> uses the target stack of the current inferior.  The target stack of the
> current inferior is the same as the `this` in the unpush method.
> 
> Avoid this detour and remove this reference to the current inferior by
> calling target_ops::find_beneath and passing `this` to find the target
> beneath `t` in the target stack that is `this`.
> 
> gdb/ChangeLog:
> 
> 	* target.c (target_stack::unpush): Call target_ops::find_beneath
> 	to get the target beneath `t`.

LGTM.

Thanks,
Andrew

> 
> Change-Id: If9d9661567c5c16f655d270bd2ec9f1b3aa6dadc
> ---
>  gdb/target.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/target.c b/gdb/target.c
> index 00f0acde7586..78327a2a6c33 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1214,7 +1214,7 @@ target_stack::unpush (target_ops *t)
>    m_stack[stratum] = NULL;
>  
>    if (m_top == stratum)
> -    m_top = t->beneath ()->stratum ();
> +    m_top = this->find_beneath (t)->stratum ();
>  
>    /* Finally close the target, if there are no inferiors
>       referencing this target still.  Note we do this after unchaining,
> -- 
> 2.30.1
> 

Re: [PATCH 2/5] gdb: remove reference to current inferior in target_stack::unpush

[Simon Marchi]

On 2021-05-06 12:15 p.m., Andrew Burgess wrote:
> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-06 10:28:03 -0400]:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> target_stack::unpush needs to get the target beneath the target being
>> unpushed to update the m_top field (which keeps the stratum of the
>> top-most target).  It currently does so using target_ops::beneath, which
>> uses the target stack of the current inferior.  The target stack of the
>> current inferior is the same as the `this` in the unpush method.
>>
>> Avoid this detour and remove this reference to the current inferior by
>> calling target_ops::find_beneath and passing `this` to find the target
>> beneath `t` in the target stack that is `this`.
>>
>> gdb/ChangeLog:
>>
>> 	* target.c (target_stack::unpush): Call target_ops::find_beneath
>> 	to get the target beneath `t`.
> 
> LGTM.
> 
> Thanks,
> Andrew

Thanks, I pushed the first two patches on their own.

Simon

[PATCH 3/5] gdb: call target_follow_exec when "set follow-exec-mode" is "same"

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

target_follow_exec is currently only called in the "follow-exec-mode ==
new" branch of follow_exec, not the "follow-exec-mode == same" branch.
I think it would make sense to call it regardless of the mode to let
targets do some necessary handling.

This is needed in the context of rocm-gdb [1], where a target is pushed
on top of the linux-nat target.  On exec, it needs to do some
bookkeeping, close some file descriptors / handles that were related to
the process pre-exec and open some new ones for the process post-exec.

However, by looking at the only in-tree implementation of
target_ops::follow_exec, remote_target::follow_exec, I found that it
would be useful for the extended-remote target too, to align its
behavior with native debugging (although I think that behavior is not
very user-friendly, see PR 27745 [2]).

Using two programs, one (let's call it "execer") that execs the other
(let's call it "execee"), with native:

    $ ./gdb -q -nx --data-directory=data-directory ./execer
    Reading symbols from ./execer...
    (gdb) r
    Starting program: /home/simark/build/binutils-gdb/gdb/execer
    I am execer
    process 1495622 is executing new program: /home/simark/build/binutils-gdb/gdb/execee
    I am execee
    [Inferior 1 (process 1495622) exited normally]
    (gdb) r
    Starting program: /home/simark/build/binutils-gdb/gdb/execee
    I am execee
    [Inferior 1 (process 1495626) exited normally]

And now with gdbserver (some irrelevant output lines removed for brevity):

    $ ./gdbserver --once --multi :1234
    ...

    $ ./gdb -q -nx --data-directory=data-directory ./execer -ex "set remote exec-file /home/simark/build/binutils-gdb/gdb/execer" -ex "tar ext :1234"
    Reading symbols from ./execer...
    Remote debugging using :1234
    (gdb) r
    Starting program: /home/simark/build/binutils-gdb/gdb/execer
    process 1495724 is executing new program: /home/simark/build/binutils-gdb/gdb/execee
    [Inferior 1 (process 1495724) exited normally]
    (gdb) r
    `target:/home/simark/build/binutils-gdb/gdb/execee' has disappeared; keeping its symbols.
    Starting program: target:/home/simark/build/binutils-gdb/gdb/execee
    warning: Build ID mismatch between current exec-file target:/home/simark/build/binutils-gdb/gdb/execee
    and automatically determined exec-file target:/home/simark/build/binutils-gdb/gdb/execer
    exec-file-mismatch handling is currently "ask"
    Reading /home/simark/build/binutils-gdb/gdb/execer from remote target...
    Load new symbol table from "target:/home/simark/build/binutils-gdb/gdb/execer"? (y or n)

When handling the exec, GDB updates the exec-file of the inferior to be
the execee.  This means that a subsequent "run" will run the execee, not
the original executable (execer).

remote_target::follow_exec is meant to update the "remote exec-file",
which is the file on the remote system that will be executed if you
"run" the inferior, to the execee as well.  However, this is not called
when follow-exec-mode is same, because target_follow_exec is not called
in this branch.  As a result, GDB thinks the inferior is executing
execee but the remote side is really executing execer, hence the
mismatch message.

By calling target_follow_exec in the "same" branch of the follow_exec
function, we ensure that everybody agrees, and we get the same behavior
with the extended-remote target as we get with the native target, the
execee is executed on the second run:

    $ ./gdbserver --once --multi :1234
    ...

    $ ./gdb -q -nx --data-directory=data-directory ./execer -ex "set remote exec-file /home/simark/build/binutils-gdb/gdb/execer" -ex "tar ext :1234"
    Reading symbols from ./execer...
    Remote debugging using :1234
    (gdb) r
    Starting program: /home/simark/build/binutils-gdb/gdb/execer
    process 1501445 is executing new program: /home/simark/build/binutils-gdb/gdb/execee
    [Inferior 1 (process 1501445) exited normally]
    (gdb) r
    `target:/home/simark/build/binutils-gdb/gdb/execee' has disappeared; keeping its symbols.
    Starting program: target:/home/simark/build/binutils-gdb/gdb/execee
    [Inferior 1 (process 1501447) exited normally]
    (gdb)

This scenario is tested in gdb.base/foll-exec-mode.exp, and in fact this
patch fixes the test for me when using
--target_board=native-extended-gdbserver.

gdb/ChangeLog:

	* infrun.c (follow_exec): Call target_follow_fork when
	follow-exec-mode is same.
	* target.h (target_follow_fork): Improve doc.

[1] https://github.com/ROCm-Developer-Tools/ROCgdb
[2] https://sourceware.org/bugzilla/show_bug.cgi?id=27745

Change-Id: I4ee84a875e39bf3f8eaf3e6789a4bfe23a2a430e
---
 gdb/infrun.c | 1 +
 gdb/target.h | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 90bab8d984f5..3e386aa587ca 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1198,6 +1198,7 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
 	 around (its description is later cleared/refetched on
 	 restart).  */
       target_clear_description ();
+      target_follow_exec (inf, exec_file_target);
     }
 
   gdb_assert (current_program_space == inf->pspace);
diff --git a/gdb/target.h b/gdb/target.h
index 48bf734279af..b80cf88db813 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1714,8 +1714,11 @@ extern int target_remove_vfork_catchpoint (int pid);
 
 void target_follow_fork (bool follow_child, bool detach_fork);
 
-/* Handle the target-specific bookkeeping required when the inferior
-   makes an exec call.  INF is the exec'd inferior.  */
+/* Handle the target-specific bookkeeping required when the inferior makes an
+   exec call.  The current inferior is the inferior that has executed the exec
+   call.  INF is the inferior in which execution continues post-exec.  It is the
+   same inferior as the current one if "follow-exec-mode" is "same" but is a new
+   one if "follow-exec-mode" is "new".  */
 
 void target_follow_exec (struct inferior *inf, const char *execd_pathname);
 
-- 
2.30.1

[PATCH 4/5] gdb: on exec, delegate pushing / unpushing target and adding thread to target_ops::follow_exec

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

On "exec", some targets need to unpush themselves from the inferior,
and do some bookkeeping, like forgetting the data associated to the
exec'ing inferior.

One such example is the thread-db target.  It does so in
a special case in thread_db_target::wait, just before returning the
TARGET_WAITKIND_EXECD event to its caller.

We have another such case in the context of rocm-gdb [1], where the
"rocm" target is pushed on top of the linux-nat target.  When an exec
happens, we want to unpush the rocm target from the exec'ing inferior to
close some file descriptors that refer to the pre-exec address space and
forget about that inferior.  We then want to push the target on the
inferior in which execution continues, to open the file descriptors for
the post-exec address space.

I think that a good way to address this cleanly is to do all this in the
target_ops::follow_exec implementations.  Make the
process_stratum_target::follow_exec implementation have the default
behavior of pushing itself to the new inferior's target stack (if
execution continues in a new inferior) and add the initial thread.

remote_target::follow_exec is an example of process target that wants to
do a bit more than the default behavior.  So it calls
process_stratum_target::follow_exec first and does the extra work
second.

linux-thread-db (a non-process target) implements follow_exec to do some
bookeeping (forget about that process' data), before handing down the
event down to the process target (which hits
process_stratum_target::follow_exec).

gdb/ChangeLog:

	* target.h (struct target_ops) <follow_exec>: Add ptid_t
	parameter.
	(target_follow_exec): Likewise.
	* target.c (target_follow_exec): Add ptid_t parameter.
	* infrun.c (follow_exec): Adjust call to target_follow_exec,
	don't push target nor create thread.
	* linux-thread-db.c (class thread_db_target) <follow_exec>: New.
	(thread_db_target::wait): Just return on TARGET_WAITKIND_EXECD.
	(thread_db_target::follow_exec): New.
	* remote.c (class remote_target) <follow_exec>: Add ptid_t parameter.
	(remote_target::follow_exec): Call
	process_stratum_target::follow_exec.
	* target-delegates.c: Re-generate.

Change-Id: I3f96d0ba3ea0dde6540b7e1b4d5cdb01635088c8
---
 gdb/infrun.c                 | 24 ++++++++++++------------
 gdb/linux-thread-db.c        | 25 +++++++++++++++----------
 gdb/process-stratum-target.c | 20 ++++++++++++++++++++
 gdb/process-stratum-target.h |  8 ++++++++
 gdb/remote.c                 | 14 +++++++-------
 gdb/target-delegates.c       | 20 +++++++++++---------
 gdb/target.c                 |  8 +++++---
 gdb/target.h                 | 19 +++++++++++++------
 8 files changed, 91 insertions(+), 47 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 3e386aa587ca..7fc56dc51f02 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1064,7 +1064,6 @@ show_follow_exec_mode_string (struct ui_file *file, int from_tty,
 static void
 follow_exec (ptid_t ptid, const char *exec_file_target)
 {
-  struct inferior *inf = current_inferior ();
   int pid = ptid.pid ();
   ptid_t process_ptid;
 
@@ -1167,6 +1166,8 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
      previous incarnation of this process.  */
   no_shared_libraries (NULL, 0);
 
+  struct inferior *inf = current_inferior ();
+
   if (follow_exec_mode_string == follow_exec_mode_new)
     {
       /* The user wants to keep the old inferior and program spaces
@@ -1176,18 +1177,16 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
 	 inferior's pid.  Having two inferiors with the same pid would confuse
 	 find_inferior_p(t)id.  Transfer the terminal state and info from the
 	  old to the new inferior.  */
-      inf = add_inferior_with_spaces ();
-      swap_terminal_info (inf, current_inferior ());
-      exit_inferior_silent (current_inferior ());
+      inferior *new_inferior = add_inferior_with_spaces ();
 
-      inf->pid = pid;
-      target_follow_exec (inf, exec_file_target);
+      swap_terminal_info (new_inferior, inf);
+      exit_inferior_silent (inf);
 
-      inferior *org_inferior = current_inferior ();
-      switch_to_inferior_no_thread (inf);
-      inf->push_target (org_inferior->process_target ());
-      thread_info *thr = add_thread (inf->process_target (), ptid);
-      switch_to_thread (thr);
+      new_inferior->pid = pid;
+      target_follow_exec (new_inferior, ptid, exec_file_target);
+
+      /* We continue with the new inferior.  */
+      inf = new_inferior;
     }
   else
     {
@@ -1198,9 +1197,10 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
 	 around (its description is later cleared/refetched on
 	 restart).  */
       target_clear_description ();
-      target_follow_exec (inf, exec_file_target);
+      target_follow_exec (inf, ptid, exec_file_target);
     }
 
+  gdb_assert (current_inferior () == inf);
   gdb_assert (current_program_space == inf->pspace);
 
   /* Attempt to open the exec file.  SYMFILE_DEFER_BP_RESET is used
diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c
index 1c6ea4debd88..2c75cd607949 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -95,6 +95,7 @@ class thread_db_target final : public target_ops
   ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
   void resume (ptid_t, int, enum gdb_signal) override;
   void mourn_inferior () override;
+  void follow_exec (inferior *, ptid_t, const char *) override;
   void update_thread_list () override;
   std::string pid_to_str (ptid_t) override;
   CORE_ADDR get_thread_local_address (ptid_t ptid,
@@ -1384,6 +1385,7 @@ thread_db_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
     case TARGET_WAITKIND_EXITED:
     case TARGET_WAITKIND_THREAD_EXITED:
     case TARGET_WAITKIND_SIGNALLED:
+    case TARGET_WAITKIND_EXECD:
       return ptid;
     }
 
@@ -1393,16 +1395,6 @@ thread_db_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
   if (info == NULL)
     return ptid;
 
-  if (ourstatus->kind == TARGET_WAITKIND_EXECD)
-    {
-      /* New image, it may or may not end up using thread_db.  Assume
-	 not unless we find otherwise.  */
-      delete_thread_db_info (beneath, ptid.pid ());
-      current_inferior ()->unpush_target (this);
-
-      return ptid;
-    }
-
   /* Fill in the thread's user-level thread id and status.  */
   thread_from_lwp (find_thread_ptid (beneath, ptid), ptid);
 
@@ -1423,6 +1415,19 @@ thread_db_target::mourn_inferior ()
   current_inferior ()->unpush_target (this);
 }
 
+void
+thread_db_target::follow_exec (inferior *follow_inf, ptid_t ptid,
+			       const char *execd_pathname)
+{
+  process_stratum_target *beneath
+    = as_process_stratum_target (this->beneath ());
+
+  delete_thread_db_info (beneath, ptid.pid ());
+
+  current_inferior ()->unpush_target (this);
+  beneath->follow_exec (follow_inf, ptid, execd_pathname);
+}
+
 struct callback_data
 {
   struct thread_db_info *info;
diff --git a/gdb/process-stratum-target.c b/gdb/process-stratum-target.c
index 719167803fff..c851090a7f2e 100644
--- a/gdb/process-stratum-target.c
+++ b/gdb/process-stratum-target.c
@@ -84,6 +84,26 @@ process_stratum_target::has_execution (inferior *inf)
   return inf->pid != 0;
 }
 
+void
+process_stratum_target::follow_exec (inferior *follow_inf, ptid_t ptid,
+				     const char *execd_pathname)
+{
+  inferior *orig_inf = current_inferior ();
+
+  if (orig_inf != follow_inf)
+    {
+      /* Execution continues in a new inferior, push the original inferior's
+         process target on the new inferior's target stack.  The process target
+	 may decide to unpush itself from the original inferior's target stack
+	 after that, at its discretion.  */
+      follow_inf->push_target (orig_inf->process_target ());
+      thread_info *t = add_thread (follow_inf->process_target (), ptid);
+
+      /* Leave the new inferior / thread as the current inferior / thread.  */
+      switch_to_thread (t);
+    }
+}
+
 /* See process-stratum-target.h.  */
 
 std::set<process_stratum_target *>
diff --git a/gdb/process-stratum-target.h b/gdb/process-stratum-target.h
index 6fddbba90c72..31a97753db9c 100644
--- a/gdb/process-stratum-target.h
+++ b/gdb/process-stratum-target.h
@@ -63,6 +63,14 @@ class process_stratum_target : public target_ops
   bool has_registers () override;
   bool has_execution (inferior *inf) override;
 
+  /* Default implementation of follow_exec.
+
+     If the current inferior and FOLLOW_INF are different (execution continues
+     in a new inferior), push this process target to FOLLOW_INF's target stack
+     and add an initial thread to FOLLOW_INF.  */
+  void follow_exec (inferior *follow_inf, ptid_t ptid,
+		    const char *execd_pathname) override;
+
   /* True if any thread is, or may be executing.  We need to track
      this separately because until we fully sync the thread list, we
      won't know whether the target is fully stopped, even if we see
diff --git a/gdb/remote.c b/gdb/remote.c
index a7312a9fc2d1..58d9b5bd24de 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -683,7 +683,7 @@ class remote_target : public process_stratum_target
   const struct btrace_config *btrace_conf (const struct btrace_target_info *) override;
   bool augmented_libraries_svr4_read () override;
   void follow_fork (bool, bool) override;
-  void follow_exec (struct inferior *, const char *) override;
+  void follow_exec (inferior *, ptid_t, const char *) override;
   int insert_fork_catchpoint (int) override;
   int remove_fork_catchpoint (int) override;
   int insert_vfork_catchpoint (int) override;
@@ -5925,20 +5925,20 @@ remote_target::follow_fork (bool follow_child, bool detach_fork)
 }
 
 /* Target follow-exec function for remote targets.  Save EXECD_PATHNAME
-   in the program space of the new inferior.  On entry and at return the
-   current inferior is the exec'ing inferior.  INF is the new exec'd
-   inferior, which may be the same as the exec'ing inferior unless
-   follow-exec-mode is "new".  */
+   in the program space of the new inferior.  */
 
 void
-remote_target::follow_exec (struct inferior *inf, const char *execd_pathname)
+remote_target::follow_exec (inferior *follow_inf, ptid_t ptid,
+			    const char *execd_pathname)
 {
+  process_stratum_target::follow_exec (follow_inf, ptid, execd_pathname);
+
   /* We know that this is a target file name, so if it has the "target:"
      prefix we strip it off before saving it in the program space.  */
   if (is_target_filename (execd_pathname))
     execd_pathname += strlen (TARGET_SYSROOT_PREFIX);
 
-  set_pspace_remote_exec_file (inf->pspace, execd_pathname);
+  set_pspace_remote_exec_file (follow_inf->pspace, execd_pathname);
 }
 
 /* Same as remote_detach, but don't send the "D" packet; just disconnect.  */
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index ef8c94cec8c6..3e759a2f80ea 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -59,7 +59,7 @@ struct dummy_target : public target_ops
   void follow_fork (bool arg0, bool arg1) override;
   int insert_exec_catchpoint (int arg0) override;
   int remove_exec_catchpoint (int arg0) override;
-  void follow_exec (struct inferior *arg0, const char *arg1) override;
+  void follow_exec (inferior *arg0, ptid_t arg1, const char *arg2) override;
   int set_syscall_catchpoint (int arg0, bool arg1, int arg2, gdb::array_view<const int> arg3) override;
   void mourn_inferior () override;
   void pass_signals (gdb::array_view<const unsigned char> arg0) override;
@@ -234,7 +234,7 @@ struct debug_target : public target_ops
   void follow_fork (bool arg0, bool arg1) override;
   int insert_exec_catchpoint (int arg0) override;
   int remove_exec_catchpoint (int arg0) override;
-  void follow_exec (struct inferior *arg0, const char *arg1) override;
+  void follow_exec (inferior *arg0, ptid_t arg1, const char *arg2) override;
   int set_syscall_catchpoint (int arg0, bool arg1, int arg2, gdb::array_view<const int> arg3) override;
   void mourn_inferior () override;
   void pass_signals (gdb::array_view<const unsigned char> arg0) override;
@@ -1595,25 +1595,27 @@ debug_target::remove_exec_catchpoint (int arg0)
 }
 
 void
-target_ops::follow_exec (struct inferior *arg0, const char *arg1)
+target_ops::follow_exec (inferior *arg0, ptid_t arg1, const char *arg2)
 {
-  this->beneath ()->follow_exec (arg0, arg1);
+  this->beneath ()->follow_exec (arg0, arg1, arg2);
 }
 
 void
-dummy_target::follow_exec (struct inferior *arg0, const char *arg1)
+dummy_target::follow_exec (inferior *arg0, ptid_t arg1, const char *arg2)
 {
 }
 
 void
-debug_target::follow_exec (struct inferior *arg0, const char *arg1)
+debug_target::follow_exec (inferior *arg0, ptid_t arg1, const char *arg2)
 {
   fprintf_unfiltered (gdb_stdlog, "-> %s->follow_exec (...)\n", this->beneath ()->shortname ());
-  this->beneath ()->follow_exec (arg0, arg1);
+  this->beneath ()->follow_exec (arg0, arg1, arg2);
   fprintf_unfiltered (gdb_stdlog, "<- %s->follow_exec (", this->beneath ()->shortname ());
-  target_debug_print_struct_inferior_p (arg0);
+  target_debug_print_inferior_p (arg0);
   fputs_unfiltered (", ", gdb_stdlog);
-  target_debug_print_const_char_p (arg1);
+  target_debug_print_ptid_t (arg1);
+  fputs_unfiltered (", ", gdb_stdlog);
+  target_debug_print_const_char_p (arg2);
   fputs_unfiltered (")\n", gdb_stdlog);
 }
 
diff --git a/gdb/target.c b/gdb/target.c
index 78327a2a6c33..9642d68c4c41 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2718,12 +2718,14 @@ target_follow_fork (bool follow_child, bool detach_fork)
   return target->follow_fork (follow_child, detach_fork);
 }
 
-/* Target wrapper for follow exec hook.  */
+/* See target.h.  */
 
 void
-target_follow_exec (struct inferior *inf, const char *execd_pathname)
+target_follow_exec (inferior *follow_inf, ptid_t ptid,
+		    const char *execd_pathname)
 {
-  current_inferior ()->top_target ()->follow_exec (inf, execd_pathname);
+  current_inferior ()->top_target ()->follow_exec (follow_inf, ptid,
+						   execd_pathname);
 }
 
 static void
diff --git a/gdb/target.h b/gdb/target.h
index b80cf88db813..d867a58e2a8b 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -642,7 +642,7 @@ struct target_ops
       TARGET_DEFAULT_RETURN (1);
     virtual int remove_exec_catchpoint (int)
       TARGET_DEFAULT_RETURN (1);
-    virtual void follow_exec (struct inferior *, const char *)
+    virtual void follow_exec (inferior *, ptid_t, const char *)
       TARGET_DEFAULT_IGNORE ();
     virtual int set_syscall_catchpoint (int, bool, int,
 					gdb::array_view<const int>)
@@ -1715,12 +1715,19 @@ extern int target_remove_vfork_catchpoint (int pid);
 void target_follow_fork (bool follow_child, bool detach_fork);
 
 /* Handle the target-specific bookkeeping required when the inferior makes an
-   exec call.  The current inferior is the inferior that has executed the exec
-   call.  INF is the inferior in which execution continues post-exec.  It is the
-   same inferior as the current one if "follow-exec-mode" is "same" but is a new
-   one if "follow-exec-mode" is "new".  */
+   exec call.
 
-void target_follow_exec (struct inferior *inf, const char *execd_pathname);
+   The current inferior at the time of the call is the inferior that did the
+   exec.  FOLLOW_INF is the inferior in which execution continues post-exec.
+   If "follow-exec-mode" is "same", FOLLOW_INF is the same as the current
+   inferior, meaning that execution continues with the same inferior.  If
+   "follow-exec-mode" is "new", FOLLOW_INF is a different inferior, meaning
+   that execution continues in a new inferior.
+
+   On exit, the target must leave FOLLOW_INF as the current inferior.  */
+
+void target_follow_exec (inferior *follow_inf, ptid_t ptid,
+			 const char *execd_pathname);
 
 /* On some targets, we can catch an inferior exec event when it
    occurs.  These functions insert/remove an already-created
-- 
2.30.1

[PATCH 5/5] gdb: maybe unpush target from old inferior in inf_child_target::follow_exec

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

I realized that with "follow-exec-mode == new", the process target
stayed pushed in the original inferior.  This can cause a small
incoherence:

    $ ./gdb -q -nx --data-directory=data-directory -ex "set follow-exec-mode new" --args execer args-for-execer
    Reading symbols from execer...
    (gdb) r
    Starting program: /home/smarchi/build/binutils-gdb/gdb/execer args-for-execer
    I am execer and my argv[1] is: args-for-execer
    process 3562426 is executing new program: /home/smarchi/build/binutils-gdb/gdb/execee
    [New inferior 2]
    [New process 3562426]
    I am execee and my argv[1] is: arg-for-execee
    [Inferior 2 (process 3562426) exited normally]
    (gdb) info inferiors
      Num  Description       Connection           Executable
      1    <null>            1 (native)           /home/smarchi/build/binutils-gdb/gdb/execer
    * 2    <null>                                 /home/smarchi/build/binutils-gdb/gdb/execee
    (gdb) maintenance print target-stack
    The current target stack is:
      - exec (Local exec file)
      - None (None)
    (gdb) inferior 1
    [Switching to inferior 1 [<null>] (/home/smarchi/build/binutils-gdb/gdb/execer)]
    (gdb) maintenance print target-stack
    The current target stack is:
      - native (Native process)
      - exec (Local exec file)
      - None (None)

On exec, when execution continues into inferior 2, the native target
isn't unpushed from inferior 1.  When inferior 2's execution finishes
normally, inf_child_target::mourn_inferior unpushes the native target,
because the native target has been implicitly opened.

I think that if the native target was implicitly opened, it should be
unpushed from inferior 1, just like it is unpushed from an inferior
whose execution terminate.  This patch implements that.

gdb/ChangeLog:

	* inf-child.h (inf_child_target) <follow_exec>: New.
	* inf-child.c (inf_child_target::follow_exec): New.

Change-Id: I782cc08d73d93a990f4e53611107f68b2cb58af1
---
 gdb/inf-child.c                           | 18 ++++++++++++++++++
 gdb/inf-child.h                           |  3 +++
 gdb/testsuite/gdb.base/foll-exec-mode.exp | 14 +++++++++++++-
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index 0e68a40d7c04..f690aa77913f 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -409,6 +409,24 @@ inf_child_target::can_use_agent ()
   return agent_loaded_p ();
 }
 
+void
+inf_child_target::follow_exec (inferior *follow_inf, ptid_t ptid,
+			       const char *execd_pathname)
+{
+  inferior *orig_inf = current_inferior ();
+
+  process_stratum_target::follow_exec (follow_inf, ptid, execd_pathname);
+
+  if (orig_inf != follow_inf)
+    {
+      /* If the target was implicitly push in the original inferior, unpush
+         it.  */
+      scoped_restore_current_thread restore_thread;
+      switch_to_inferior_no_thread (orig_inf);
+      maybe_unpush_target ();
+    }
+}
+
 /* See inf-child.h.  */
 
 void
diff --git a/gdb/inf-child.h b/gdb/inf-child.h
index d697a3938ee6..aa33c5381381 100644
--- a/gdb/inf-child.h
+++ b/gdb/inf-child.h
@@ -57,6 +57,9 @@ class inf_child_target
 
   void post_startup_inferior (ptid_t) override;
 
+  void follow_exec (inferior *follow_inf, ptid_t ptid,
+		    const char *execd_pathname) override;
+
   void mourn_inferior () override;
 
   bool can_run () override;
diff --git a/gdb/testsuite/gdb.base/foll-exec-mode.exp b/gdb/testsuite/gdb.base/foll-exec-mode.exp
index 7d7b6605dafb..9087c8baf2c4 100644
--- a/gdb/testsuite/gdb.base/foll-exec-mode.exp
+++ b/gdb/testsuite/gdb.base/foll-exec-mode.exp
@@ -113,6 +113,8 @@ proc do_follow_exec_mode_tests { mode cmd infswitch } {
 	    return
 	}
 
+	set target_is_native [gdb_is_target_native]
+
 	# Set the follow-exec mode.
 	#
 	gdb_test_no_output "set follow-exec-mode $mode"
@@ -147,8 +149,18 @@ proc do_follow_exec_mode_tests { mode cmd infswitch } {
 	#
 	if {$mode == "same"} {
 	    set expected_re "\\* 1.*process.*"
+	} elseif { $mode == "new" } {
+	    # If using the native target, we want to specifically check that the
+	    # process target, which was automatically pushed when running, was
+	    # automatically unpushed from inferior 1 on exec.  Use a
+	    # different regexp that verifies the Connection field is empty.
+	    if { $target_is_native } {
+		set expected_re "  1.*<null> +[string_to_regexp $binfile].*\r\n\\* 2.*process.*$testfile2 .*"
+	    } else {
+		set expected_re "  1.*null.*$testfile.*\r\n\\* 2.*process.*$testfile2 .*"
+	    }
 	} else {
-	    set expected_re "  1.*null.*$testfile.*\r\n\\* 2.*process.*$testfile2 .*"
+	    error "Invalid mode: $mode"
 	}
 
 	# Check that the inferior list is correct:
-- 
2.30.1

Re: [PATCH 5/5] gdb: maybe unpush target from old inferior in inf_child_target::follow_exec

[Simon Marchi]

On 2021-05-06 10:28 a.m., Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> I realized that with "follow-exec-mode == new", the process target
> stayed pushed in the original inferior.  This can cause a small
> incoherence:
> 
>     $ ./gdb -q -nx --data-directory=data-directory -ex "set follow-exec-mode new" --args execer args-for-execer
>     Reading symbols from execer...
>     (gdb) r
>     Starting program: /home/smarchi/build/binutils-gdb/gdb/execer args-for-execer
>     I am execer and my argv[1] is: args-for-execer
>     process 3562426 is executing new program: /home/smarchi/build/binutils-gdb/gdb/execee
>     [New inferior 2]
>     [New process 3562426]
>     I am execee and my argv[1] is: arg-for-execee
>     [Inferior 2 (process 3562426) exited normally]
>     (gdb) info inferiors
>       Num  Description       Connection           Executable
>       1    <null>            1 (native)           /home/smarchi/build/binutils-gdb/gdb/execer
>     * 2    <null>                                 /home/smarchi/build/binutils-gdb/gdb/execee
>     (gdb) maintenance print target-stack
>     The current target stack is:
>       - exec (Local exec file)
>       - None (None)
>     (gdb) inferior 1
>     [Switching to inferior 1 [<null>] (/home/smarchi/build/binutils-gdb/gdb/execer)]
>     (gdb) maintenance print target-stack
>     The current target stack is:
>       - native (Native process)
>       - exec (Local exec file)
>       - None (None)
> 
> On exec, when execution continues into inferior 2, the native target
> isn't unpushed from inferior 1.  When inferior 2's execution finishes
> normally, inf_child_target::mourn_inferior unpushes the native target,
> because the native target has been implicitly opened.
> 
> I think that if the native target was implicitly opened, it should be
> unpushed from inferior 1, just like it is unpushed from an inferior
> whose execution terminate.  This patch implements that.

I pushed the rest of the series.  Note that Pedro did take a look at
this before it was sent to the list, so I have enough confidence it's
ok.

Simon

RE: [PATCH 1/5] gdb: make target_close check that the target isn't pushed in all inferiors

[Aktemur, Tankut Baris]

On Thursday, May 6, 2021 4:28 PM, Simon Marchi wrote:
> The target_close function currently checks that the target to be closed
> isn't pushed in the current inferior:
> 
>     gdb_assert (!current_inferior ()->target_is_pushed (targ));
> 
> When a target is closed, it's normally because its refcount has dropped
> to 0, because it's not used in any inferior anymore.

Is there a missing word in this sentence?

-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/5] gdb: make target_close check that the target isn't pushed in all inferiors

[Simon Marchi]

On 2021-05-06 12:01 p.m., Aktemur, Tankut Baris wrote:
> On Thursday, May 6, 2021 4:28 PM, Simon Marchi wrote:
>> The target_close function currently checks that the target to be closed
>> isn't pushed in the current inferior:
>>
>>     gdb_assert (!current_inferior ()->target_is_pushed (targ));
>>
>> When a target is closed, it's normally because its refcount has dropped
>> to 0, because it's not used in any inferior anymore.
> 
> Is there a missing word in this sentence?

Hmm I don't think so.  Can you point out what seems wrong with it?

Simon

RE: [PATCH 1/5] gdb: make target_close check that the target isn't pushed in all inferiors

[Aktemur, Tankut Baris]

On Thursday, May 6, 2021 6:36 PM, Simon Marchi Wrote:
> On 2021-05-06 12:01 p.m., Aktemur, Tankut Baris wrote:
> > On Thursday, May 6, 2021 4:28 PM, Simon Marchi wrote:
> >> The target_close function currently checks that the target to be closed
> >> isn't pushed in the current inferior:
> >>
> >>     gdb_assert (!current_inferior ()->target_is_pushed (targ));
> >>
> >> When a target is closed, it's normally because its refcount has dropped
> >> to 0, because it's not used in any inferior anymore.
> >
> > Is there a missing word in this sentence?
> 
> Hmm I don't think so.  Can you point out what seems wrong with it?
> 
> Simon

After reading the sentence a few times, I convinced myself that a word
may not be missing.  Did you mean something like the following?

  Normally, a target is closed when its refcount has dropped to 0, due to
  not being used in any inferior anymore.

Anyway, not a big deal if you want to keep the original sentence.

-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/5] gdb: make target_close check that the target isn't pushed in all inferiors

[Simon Marchi]

On 2021-05-07 3:44 a.m., Aktemur, Tankut Baris wrote:
> On Thursday, May 6, 2021 6:36 PM, Simon Marchi Wrote:
>> On 2021-05-06 12:01 p.m., Aktemur, Tankut Baris wrote:
>>> On Thursday, May 6, 2021 4:28 PM, Simon Marchi wrote:
>>>> The target_close function currently checks that the target to be closed
>>>> isn't pushed in the current inferior:
>>>>
>>>>     gdb_assert (!current_inferior ()->target_is_pushed (targ));
>>>>
>>>> When a target is closed, it's normally because its refcount has dropped
>>>> to 0, because it's not used in any inferior anymore.
>>>
>>> Is there a missing word in this sentence?
>>
>> Hmm I don't think so.  Can you point out what seems wrong with it?
>>
>> Simon
> 
> After reading the sentence a few times, I convinced myself that a word
> may not be missing.  Did you mean something like the following?
> 
>   Normally, a target is closed when its refcount has dropped to 0, due to
>   not being used in any inferior anymore.
> 
> Anyway, not a big deal if you want to keep the original sentence.

Thanks, I'll use your version.

Simon

Re: [PATCH 1/5] gdb: make target_close check that the target isn't pushed in all inferiors

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-06 10:28:02 -0400]:

> From: Simon Marchi <simon.marchi@efficios.com>
> 
> The target_close function currently checks that the target to be closed
> isn't pushed in the current inferior:
> 
>     gdb_assert (!current_inferior ()->target_is_pushed (targ));
> 
> When a target is closed, it's normally because its refcount has dropped
> to 0, because it's not used in any inferior anymore.  I think it would
> make sense to change that assert to not only check in the current
> inferior, but to check in all inferiors.  It would be quite bad (and a
> bug) to close a target while it's still pushed in one of the non-current
> inferiors.
> 
> gdb/ChangeLog:
> 
> 	* target.c (target_close): Check in all inferiors that the
> 	target is not pushed.

LGTM.

Thanks,
Andrew

> 
> Change-Id: I6e37fc3f3476a0593da1e476604642b2de90f1d5
> ---
>  gdb/target.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/target.c b/gdb/target.c
> index 1f0741471d82..00f0acde7586 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -3734,7 +3734,8 @@ debug_target::info () const
>  void
>  target_close (struct target_ops *targ)
>  {
> -  gdb_assert (!current_inferior ()->target_is_pushed (targ));
> +  for (inferior *inf : all_inferiors ())
> +    gdb_assert (!inf->target_is_pushed (targ));
>  
>    fileio_handles_invalidate_target (targ);
>  
> -- 
> 2.30.1
> 

Re: [PATCH 1/5] gdb: make target_close check that the target isn't pushed in all inferiors

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> -  gdb_assert (!current_inferior ()->target_is_pushed (targ));
Simon> +  for (inferior *inf : all_inferiors ())
Simon> +    gdb_assert (!inf->target_is_pushed (targ));

I wonder if this would be better as a call to std::any_of or something
along those lines, the idea being that if asserts are disabled, then the
entire loop could be dropped.  With the explicit loop, maybe the
compiler has to assume that iteration could have a side effect and the
do-nothing loop would have to remain?

Tom

Re: [PATCH 1/5] gdb: make target_close check that the target isn't pushed in all inferiors

[Simon Marchi]

On 2021-05-07 10:36 a.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> -  gdb_assert (!current_inferior ()->target_is_pushed (targ));
> Simon> +  for (inferior *inf : all_inferiors ())
> Simon> +    gdb_assert (!inf->target_is_pushed (targ));
> 
> I wonder if this would be better as a call to std::any_of or something
> along those lines, the idea being that if asserts are disabled, then the
> entire loop could be dropped.  With the explicit loop, maybe the
> compiler has to assume that iteration could have a side effect and the
> do-nothing loop would have to remain?

I see what you mean.  Given that:

1. disabling gdb_assert is not possible today (and not for the
   foreseeable future)
2. this is really not a hot path

I would value code readability and debugging convenience over a
premature optimization like this.  I'm thinking that if I ever hit this
assert, then it will be possible to attach to GDB and print the inferior
where the target is still pushed.  Whereas if we have

  gdb_assert (target_is_not_pushed_in_any_inferior (targ))

it won't be as easy.

Simon

Re: [PATCH 1/5] gdb: make target_close check that the target isn't pushed in all inferiors

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> I see what you mean.  Given that:

Simon> 1. disabling gdb_assert is not possible today (and not for the
Simon>    foreseeable future)
Simon> 2. this is really not a hot path

Simon> I would value code readability and debugging convenience over a
Simon> premature optimization like this.

We discussed this on irc, and FAOD I agree.
Thanks.

Tom