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

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

[Lancelot SIX]

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
---
 gdb/amd-dbgapi-target.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/amd-dbgapi-target.c b/gdb/amd-dbgapi-target.c
index 5565cf907fa..40f24b5fc2f 100644
--- a/gdb/amd-dbgapi-target.c
+++ b/gdb/amd-dbgapi-target.c
@@ -1463,7 +1463,7 @@ amd_dbgapi_target::detach (inferior *inf, int from_tty)
      Breakpoints may still be inserted because the inferior may be running in
      non-stop mode, or because GDB changed the default setting to leave all
      breakpoints inserted in all-stop mode when all threads are stopped.  */
-  remove_breakpoints_inf (current_inferior ());
+  remove_breakpoints_inf (inf);
 
   detach_amd_dbgapi (inf);
   beneath ()->detach (inf, from_tty);

base-commit: b1c792568662d7e00158d19e0439b64f98b78e47
-- 
2.34.1

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

[Pedro Alves]

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.

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

[Lancelot SIX]

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.

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

[Pedro Alves]

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

Pedro Alves

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

[Simon Marchi]

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

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

[Lancelot SIX]

> 
> Please go ahead and merge your patch.
Thanks,

I have pushed this patch.

Best,
Lancelot.