[PATCH] gdb: iterate over targets, not inferiors, to commit resumed

[PATCH] gdb: iterate over targets, not inferiors, to commit resumed

[Tankut Baris Aktemur]

When committing resumed threads of targets, iterate over targets, not
inferiors, so that we don't call commit_resumed multiple times for
targets that have multiple inferiors.  This gives more concise code.

Similarly, iterate over targets when setting the commit_resumed_state
of targets.

No behavioral change is expected or intended with this patch.
---
 gdb/infrun.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index bbb98f6dcdb..af9c00d32e3 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3210,16 +3210,10 @@ maybe_set_commit_resumed_all_targets ()
 {
   scoped_restore_current_thread restore_thread;
 
-  for (inferior *inf : all_non_exited_inferiors ())
+  for (auto *proc_target : all_non_exited_process_targets ())
     {
-      process_stratum_target *proc_target = inf->process_target ();
-
       if (proc_target->commit_resumed_state)
-	{
-	  /* We already set this in a previous iteration, via another
-	     inferior sharing the process_stratum target.  */
-	  continue;
-	}
+	continue;
 
       /* If the target has no resumed threads, it would be useless to
 	 ask it to commit the resumed threads.  */
@@ -3243,7 +3237,7 @@ maybe_set_commit_resumed_all_targets ()
 	  continue;
 	}
 
-      switch_to_inferior_no_thread (inf);
+      switch_to_target_no_thread (proc_target);
 
       if (target_has_pending_events ())
 	{
@@ -3267,14 +3261,12 @@ maybe_call_commit_resumed_all_targets ()
 {
   scoped_restore_current_thread restore_thread;
 
-  for (inferior *inf : all_non_exited_inferiors ())
+  for (auto *proc_target : all_non_exited_process_targets ())
     {
-      process_stratum_target *proc_target = inf->process_target ();
-
       if (!proc_target->commit_resumed_state)
 	continue;
 
-      switch_to_inferior_no_thread (inf);
+      switch_to_target_no_thread (proc_target);
 
       infrun_debug_printf ("calling commit_resumed for target %s",
 			   proc_target->shortname());
-- 
2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH] gdb: iterate over targets, not inferiors, to commit resumed

[Simon Marchi]

On 3/1/24 10:52, Tankut Baris Aktemur wrote:
> When committing resumed threads of targets, iterate over targets, not
> inferiors, so that we don't call commit_resumed multiple times for
> targets that have multiple inferiors.  This gives more concise code.
> 
> Similarly, iterate over targets when setting the commit_resumed_state
> of targets.
> 
> No behavioral change is expected or intended with this patch.

Hi Baris,

The change in maybe_call_commit_resumed_all_targets might not play well
with the amd-dbgapi target (and is probably the reason this code was
written like this in the first place).  Right now, it's kind of a hack,
but the amd-dbgapi target sits on top of the linux-nat target, in the
arch stratum layer.  It intercepts some target calls, and really acts as
a second process target for the inferior, without being an actual
process_stratum_target.

Imagine you have two inferiors, one debugging a plain Linux program and
the other a program using the GPU.  The target stacks will look like:

                  inf1       inf2
    arch stratum: <empty>    amd-dbgapi
 process stratum: linux-nat  linux-nat

By iterating only on process stratum targets, commit_resumed will only
be called on inferior 1, and therefore the amd-dbgapi target will never
see it.  The GPU threads will not effectively be resumed.

Our plan is to try to make amd-dbgapi a proper process_stratum_target
and allow inferiors to have more than one process target, in which case
your change would probably be fine.  We haven't started on that front
yet, unfortunately.

The change in maybe_set_commit_resumed_all_targets is probably fine
though.

Simon

RE: [PATCH] gdb: iterate over targets, not inferiors, to commit resumed

[Aktemur, Tankut Baris]

On Friday, March 1, 2024 8:02 PM, Simon Marchi wrote:
> On 3/1/24 10:52, Tankut Baris Aktemur wrote:
> > When committing resumed threads of targets, iterate over targets, not
> > inferiors, so that we don't call commit_resumed multiple times for
> > targets that have multiple inferiors.  This gives more concise code.
> >
> > Similarly, iterate over targets when setting the commit_resumed_state
> > of targets.
> >
> > No behavioral change is expected or intended with this patch.
> 
> Hi Baris,
> 
> The change in maybe_call_commit_resumed_all_targets might not play well
> with the amd-dbgapi target (and is probably the reason this code was
> written like this in the first place).  Right now, it's kind of a hack,
> but the amd-dbgapi target sits on top of the linux-nat target, in the
> arch stratum layer.  It intercepts some target calls, and really acts as
> a second process target for the inferior, without being an actual
> process_stratum_target.
> 
> Imagine you have two inferiors, one debugging a plain Linux program and
> the other a program using the GPU.  The target stacks will look like:
> 
>                   inf1       inf2
>     arch stratum: <empty>    amd-dbgapi
>  process stratum: linux-nat  linux-nat
> 
> By iterating only on process stratum targets, commit_resumed will only
> be called on inferior 1, and therefore the amd-dbgapi target will never
> see it.  The GPU threads will not effectively be resumed.
> 
> Our plan is to try to make amd-dbgapi a proper process_stratum_target
> and allow inferiors to have more than one process target, in which case
> your change would probably be fine.  We haven't started on that front
> yet, unfortunately.
> 
> The change in maybe_set_commit_resumed_all_targets is probably fine
> though.

Hi Simon,

I see; thanks for pointing this out.  I'd then retract the patch.  Let's
keep maybe_set_commit_resumed_all_targets and maybe_call_commit_resumed_all_targets
both have the same iteration for consistency.  We can do the change in the future
when the amd-dbgapi target is converted to a process_stratum_target.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH] gdb: iterate over targets, not inferiors, to commit resumed

[Simon Marchi]

On 3/4/24 04:28, Aktemur, Tankut Baris wrote:
> Hi Simon,
> 
> I see; thanks for pointing this out.  I'd then retract the patch.  Let's
> keep maybe_set_commit_resumed_all_targets and maybe_call_commit_resumed_all_targets
> both have the same iteration for consistency.  We can do the change in the future
> when the amd-dbgapi target is converted to a process_stratum_target.
> 
> Thanks
> -Baris

Great, thanks for your understanding.  We'll hopefully get that sorted
out this year.

In the back of my mind, I'm also worried about this for other (mostly
theoritical at this point) cases where we do things per-process-target,
ignoring that there might be other targets on top that we're bypassing.

I recently thought about this scenario, imagine we have the following
inferiors:

                | inf1             | inf2
target stratum  | my-thread-target |
process stratum | linux-nat        | linux-nat

Imagine the thread target is meant to pull events from the linux-nat
target below and translate them to some higher level events about
userspace thread.  When infrun pulls events, it selects a random
inferior and calls target_ops::wait with minus_one_ptid.  What if
inferior 2 is selected, and linux-nat returns an event about inferior 1,
that was meant to be processed by my-thread-target?

Simon

RE: [PATCH] gdb: iterate over targets, not inferiors, to commit resumed

[Aktemur, Tankut Baris]

On Monday, March 4, 2024 5:34 PM, Simon Marchi wrote:
> On 3/4/24 04:28, Aktemur, Tankut Baris wrote:
> > Hi Simon,
> >
> > I see; thanks for pointing this out.  I'd then retract the patch.  Let's
> > keep maybe_set_commit_resumed_all_targets and maybe_call_commit_resumed_all_targets
> > both have the same iteration for consistency.  We can do the change in the future
> > when the amd-dbgapi target is converted to a process_stratum_target.
> >
> > Thanks
> > -Baris
> 
> Great, thanks for your understanding.  We'll hopefully get that sorted
> out this year.
> 
> In the back of my mind, I'm also worried about this for other (mostly
> theoritical at this point) cases where we do things per-process-target,
> ignoring that there might be other targets on top that we're bypassing.

I agree.  From a developer perspective, I think the distinction between
inf->top_target () and inf->process_target () is not so easy to make.

> I recently thought about this scenario, imagine we have the following
> inferiors:
> 
>                 | inf1             | inf2
> target stratum  | my-thread-target |
> process stratum | linux-nat        | linux-nat
> 
> Imagine the thread target is meant to pull events from the linux-nat
> target below and translate them to some higher level events about
> userspace thread.  When infrun pulls events, it selects a random
> inferior and calls target_ops::wait with minus_one_ptid.  What if
> inferior 2 is selected, and linux-nat returns an event about inferior 1,
> that was meant to be processed by my-thread-target?

Would it make sense to fetch the events per inferior using the inferior's
pid as the filter ptid?  Is there a fundamental limitation against doing that?

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH] gdb: iterate over targets, not inferiors, to commit resumed

[Simon Marchi]

On 3/12/24 15:05, Aktemur, Tankut Baris wrote:
>> I recently thought about this scenario, imagine we have the following
>> inferiors:
>>
>>                 | inf1             | inf2
>> target stratum  | my-thread-target |
>> process stratum | linux-nat        | linux-nat
>>
>> Imagine the thread target is meant to pull events from the linux-nat
>> target below and translate them to some higher level events about
>> userspace thread.  When infrun pulls events, it selects a random
>> inferior and calls target_ops::wait with minus_one_ptid.  What if
>> inferior 2 is selected, and linux-nat returns an event about inferior 1,
>> that was meant to be processed by my-thread-target?
> 
> Would it make sense to fetch the events per inferior using the inferior's
> pid as the filter ptid?  Is there a fundamental limitation against doing that?

When I talked to Pedro about this issue, he came up with the same
suggestion.  It sounds good in theory, but I'm wondering if there are
cases where calling with ptid == minus_one_ptid is needed, because the
target wants to report an event for a ptid that isn't known to the core
yet.

Simon