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

[Lancelot SIX <Lancelot.Six@amd.com>]

On 08/05/2026 02:37, Simon Marchi wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On 5/7/26 1:56 PM, Lancelot SIX wrote:
>>> 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?
> 
> Yes (it's not a nitpick, you're objectively right).
> 
>>> @@ -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?
> 
> I also hesitated between the two options:
> 
>   1) do a very targeted check + checked_static_cast
>   2) just do a dynamic cast and see
> 
> We can afford to do 1), because we control both sides (core and target).
> It's not like it's an external lib that could change out of our contorl.
> If the core ever changes in this regard, yes the checked_static_cast
> will fail (in debug mode), then we simply update the target accordingly.
> I like doing it this way, so that our expectations are written in code
> and verified.  It also has the potential to catch some unwanted changes
> in the core, which the dynamic_cast would hide.
> 

My main concern would be that most people contributing to GDB do not 
build the amdgpu target (requires the rocm-dbgapi library to be 
available), so it would be easy for many to build with 
--enable-targets-all and not enable this code path.

That being said, we (at AMD) would pull such change from master and hit 
a CI failure quite promptly, so I do not expect this would be un-noticed 
for very long.

> So I'm still team 1) for the moment.  I guess in general I'm team "use
> asserts generously".
> 

I am happy to follow you there and keep this approach.

>> And last question, shouldn't this be in solib-rocm.c rather than
>> amd-dbgapi-target.c?
> 
> I'm confused, it is in solib-rocm.c currently, isn't it?

You are absolutely right, somehow I thought I was in amd-dbgapi-target.c…

Best,
Lancelot.

> 
> Simon