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

[Simon Marchi <simark@simark.ca>]

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.

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

> 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?

Simon