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

[Pedro Alves <pedro@palves.net>]

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.