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

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

On 14/07/2023 19:35, Pedro Alves wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On 2023-06-16 10:25, Lancelot SIX via Gdb-patches wrote:
>> Current implementation of amd_dbgapi_target::detach (inferior *, int)
>> does the following:
>>
>>    remove_breakpoints_inf (current_inferior ());
>>    detach_amd_dbgapi (inf);
>>    beneath ()->detach (inf, from_tty);
>>
>> I find that using a mix of `current_inferior ()` and `inf` disturbing.
>> At this point, we know that both are the same (target_detach does assert
>> that `inf == current_inferior ()` before calling target_ops::detach).
>>
>> To improve consistency, this patch replaces `current_inferior ()` with
>> `inf` in amd_dbgapi_target::detach.
>>
>> Change-Id: I01b7ba2e661c25839438354b509d7abbddb7c5ed
> 
> 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.
> 
> 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.

Lancelot.