Re: [PATCH 1/11] gdb/solib-rocm: assert that host ops isn't rocm_solib_ops

[Lancelot SIX <lancelot.six@amd.com>]

Hi Simon,

Please accept my appologies for the delay to get to this series.

On Tue, Dec 09, 2025 at 02:32:05PM -0500, Simon Marchi wrote:
> 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
> ---
>  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 d296f2830063..87b834739e2d 100644
> --- a/gdb/amd-dbgapi-target.c
> +++ b/gdb/amd-dbgapi-target.c
> @@ -2102,7 +2102,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 bd114e16b80e..e0ffe95e4e45 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -645,7 +645,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 5f064cf1fc8c..c8cbdbad97d1 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.  */

Just a nitpick here.

Should DETACH_ON_FORK and FOLLOW_CHILD be upper case?

>  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 d97ab25df5d0..a9573f8eefde 100644
> --- a/gdb/solib-rocm.c
> +++ b/gdb/solib-rocm.c
> @@ -164,8 +164,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;
> @@ -808,6 +814,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 ();
> @@ -839,6 +849,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 ());

I think I am overall OK with the approach, even if I would prefer not
have the target explicitly rely on assumption about what the core does.

If the core in infrun was to change at some point, I expect this would
fail: the pspace's solib would not be a rocm_solib_ops, so
checked_static_cast would fail.  Should this use dynimac_cast and test
for nullptr instead?

Would it work to do it unconditionnaly (not check for detach_on_fork and
follow_inf), check if the pspace's solib is the rocm one, and if so use
the host one instead?

And last question, shouldn't this be in solib-rocm.c rather than
amd-dbgapi-target.c?

Best,
Lancelot.

> +    }
> +}
> +
>  INIT_GDB_FILE (rocm_solib)
>  {
>    /* The dependency on the amd-dbgapi exists because solib-rocm's
> @@ -852,4 +880,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.52.0