[PATCH v3 01/10] gdb/solib-rocm: assert that host ops isn't rocm_solib_ops

[simon.marchi@polymtl.ca]

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

In some fork cases, the rocm_solib_target_inferior_created observer gets
called while the child inferior (passed as a parameter) already has a
rocm_solib_ops installed.  Since we unconditionally wrap the existing
solib_ops with a new rocm_solib_ops, we can end up with a chain of
multiple rocm_solib_ops, like:

  rocm_solib_ops -> rocm_solib_ops -> svr4_solib_ops

I don't think it is technically harmful as of now (unless the process
does a ton of forks and the rocm_solib_ops accumulate), but it is for
sure useless.  Add an assert for this in the rocm_solib_ops constructor,
which reveals the cases where this happens.

When a fork happens with follow-fork-mode == child and detach-on-fork
on, infrun's follow_fork_inferior function directly moves the program
space from the parent the child inferior, as an optimization.  Coming
into rocm_solib_target_inferior_created, the inferior's pspace
unexpectedly already has a rocm_solib_ops pushed.

Fix this locally by using an inferior_forked observer.  In the scenario
described above, remove the rocm_solib_ops and restore the host
solib_ops as the program space's solib_ops, to make it look as if infrun
didn't do this trick.  This requires adding two parameters to the
inferior_forked observer (detach_on_fork and follow_child).

This should probably be done by infrun directly at some point.  The
logic being that it's fine to do an optimization, but it should look as
if it didn't occur.  If infrun created a brand new pspace for the child,
there wouldn't be a rocm_solib_ops there already.  But I prefer a local
fix for now.

Finally, there are also the vfork cases, where the child inferior shares
the program space with its parent, and therefore the child's program
space already has a rocm_solib_ops installed.  Address this case by
returning early (the `inf->vfork_parent != nullptr` check), because
there is nothing we want to do for a vfork child anyway.

Change-Id: I2e76d111e96f1e01b6799b04da9cdd6f6e2984c9
Approved-by: Lancelot Six <lancelot.six@amd.com> (amdgpu)
---
 gdb/amd-dbgapi-target.c |  3 ++-
 gdb/infrun.c            |  3 ++-
 gdb/observable.h        |  8 ++++++--
 gdb/solib-rocm.c        | 31 +++++++++++++++++++++++++++++++
 4 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c
index b4ca15069063..607fa39497dd 100644
--- a/gdb/amd-dbgapi-target.c
+++ b/gdb/amd-dbgapi-target.c
@@ -2392,7 +2392,8 @@ amd_dbgapi_inferior_execd (inferior *exec_inf, inferior *follow_inf)
 
 static void
 amd_dbgapi_inferior_forked (inferior *parent_inf, inferior *child_inf,
-			    target_waitkind fork_kind)
+			    target_waitkind fork_kind, bool detach_on_fork,
+			    bool follow_child)
 {
   if (child_inf != nullptr)
     {
diff --git a/gdb/infrun.c b/gdb/infrun.c
index f51d301cf39a..ab3fde11cf94 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -699,7 +699,8 @@ holding the child stopped.  Try \"set %ps\" or \"%ps\".\n"),
   target_follow_fork (child_inf, child_ptid, fork_kind, follow_child,
 		      detach_fork);
 
-  gdb::observers::inferior_forked.notify (parent_inf, child_inf, fork_kind);
+  gdb::observers::inferior_forked.notify (parent_inf, child_inf, fork_kind,
+					  detach_fork, follow_child);
 
   /* target_follow_fork must leave the parent as the current inferior.  If we
      want to follow the child, we make it the current one below.  */
diff --git a/gdb/observable.h b/gdb/observable.h
index 9f1c33ba7100..979e9d1c3ace 100644
--- a/gdb/observable.h
+++ b/gdb/observable.h
@@ -92,9 +92,13 @@ extern observable<inferior */* exec_inf */, inferior */* follow_inf */>
    the child (because we follow only the child or we follow both), CHILD_INF
    is the child inferior.  Otherwise, CHILD_INF is nullptr.
 
-   FORK_KIND is TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED.  */
+   FORK_KIND is TARGET_WAITKIND_FORKED or TARGET_WAITKIND_VFORKED.
+
+   DETACH_ON_FORK and FOLLOW_CHILD represent the "detach-on-fork" and
+   "follow-fork-mode" settings.  */
 extern observable<inferior */* parent_inf */, inferior */* child_inf */,
-		  target_waitkind /* fork_kind */> inferior_forked;
+		  target_waitkind /* fork_kind */, bool /* detach_on_fork */,
+		  bool /* follow_child */> inferior_forked;
 
 /* The shared library specified by SOLIB has been loaded.  Note that
    when gdb calls this observer, the library's symbols probably
diff --git a/gdb/solib-rocm.c b/gdb/solib-rocm.c
index 73797801d6cd..48040111b196 100644
--- a/gdb/solib-rocm.c
+++ b/gdb/solib-rocm.c
@@ -165,8 +165,14 @@ struct rocm_solib_ops : public solib_ops
   explicit rocm_solib_ops (program_space *pspace, solib_ops_up host_ops)
     : solib_ops (pspace), m_host_ops (std::move (host_ops))
   {
+    gdb_assert (m_host_ops != nullptr);
+    gdb_assert (dynamic_cast<rocm_solib_ops *> (m_host_ops.get ()) == nullptr);
   }
 
+  /* Release the host solib_ops.  */
+  solib_ops_up release_host_ops ()
+  { return std::move (m_host_ops); }
+
   /* The methods implemented by rocm_solib_ops.  */
   owning_intrusive_list<solib> current_sos () const override;
   void create_inferior_hook (int from_tty) const override;
@@ -820,6 +826,10 @@ rocm_update_solib_list ()
 static void
 rocm_solib_target_inferior_created (inferior *inf)
 {
+  /* A vfork child shares its pspace with its parent, do not touch anything.  */
+  if (inf->vfork_parent != nullptr)
+    return;
+
   get_solib_info (inf)->solib_list.clear ();
 
   auto prev_ops = inf->pspace->release_solib_ops ();
@@ -851,6 +861,24 @@ rocm_solib_target_inferior_execd (inferior *exec_inf, inferior *follow_inf)
   get_solib_info (exec_inf)->solib_list.clear ();
 }
 
+static void
+rocm_solib_target_inferior_forked (inferior *parent_inf, inferior *child_inf,
+				   target_waitkind fork_kind,
+				   bool detach_on_fork, bool follow_child)
+{
+  if (detach_on_fork && follow_child && fork_kind == TARGET_WAITKIND_FORKED)
+    {
+      /* In this particular configuration, infrun's follow_fork_inferior
+	 function moves the parent pspace to the child directly.  Remove the
+	 existing rocm_solib_ops from the child and restore the host solib_ops,
+	 to make it look like a brand new pspace.  */
+      auto rocm_ops_holder = child_inf->pspace->release_solib_ops ();
+      auto rocm_ops
+	= gdb::checked_static_cast<rocm_solib_ops *> (rocm_ops_holder.get ());
+      child_inf->pspace->set_solib_ops (rocm_ops->release_host_ops ());
+    }
+}
+
 INIT_GDB_FILE (rocm_solib)
 {
   /* The dependency on the amd-dbgapi exists because solib-rocm's
@@ -864,4 +892,7 @@ INIT_GDB_FILE (rocm_solib)
   gdb::observers::inferior_execd.attach
     (rocm_solib_target_inferior_execd, "solib-rocm",
      { &get_amd_dbgapi_target_inferior_execd_observer_token () });
+
+  gdb::observers::inferior_forked.attach
+    (rocm_solib_target_inferior_forked, "solib-rocm");
 }
-- 
2.55.0