Re: [PATCH] gdb/amd-dbgapi-target: Use inf param in detach

Simon Marchi via Gdb-patches
2023-07-15T02:00:50Z

On 7/14/23 15:18, Pedro Alves wrote:
> On 2023-07-14 19:54, Lancelot SIX wrote:
>> On 14/07/2023 19:35, Pedro Alves wrote:
>
>>> IMO this is a case of the target method's inferior * parameter having
>>> been added too soon -- it would only make sense to have it if nothing in
>>> the body of the target method implementations is relying on inf being
>>> current_inferior on entry. But that is not the case, plenty of target_ops::detach
>>> code has that assumption. The presence of an explicit inferior pointer should mean
>>> that detach target method implementations that call code that depends
>>> on the inferior being the current inferior, should be using a
>>> scoped_restore_current_thread/inferior before calling such global-state-assuming
>>> code. But, they don't do that, instead, we have this mixed situation. IMO, it would
>>> be better to remove the parameter to avoid confusion and stick to the
>>> (if explicit param, then switch global state to it if you need it) rule.

I agree with this assessment. I was probably the one adding this
parameter, before I understood that having a function receive the
context by parameter _and_ depend on the global context is bad (worse
than just depending on global context).

>>> Anyhow, your patch doesn't make it worse, so it's fine with me.
>>>
>>
>> I can also go the other way around and always use `current_inferior ()` instead of the `inf` parameter in this detach implementation.
>>
>> What bugged me here is the inconsistency from one line to the next.
>>
>
> Please go ahead and merge your patch. I quickly looked at what it would like to remove
> the parameter throughout, and I think we'd just end up doing:
>
> inferior *inf = current_inferior ();
>
> at the top of the function, so using "inf" in the remove_breakpoints_inf call anyhow, as
> we have multiple references to the current inferior.

Either fix is fine with me.

Simon