[PATCH] gdb/hurd: handle inferiors exiting

[PATCH] gdb/hurd: handle inferiors exiting

[Andrew Burgess]

While testing on GNU/Hurd (i386) I noticed that GDB crashes when an
inferior exits, with this error:

  inferior.c:293: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.

The problem appears to be in gnu_nat_target::wait.

When GDB is waiting for any active inferior to do something
interesting, we pass in the null_ptid to indicate that the target
should wait on all inferiors.

In gnu_nat_target::wait, when we get an exit event, we currently
return the passed in ptid (null_ptid) and there's a comment in the
code "let wait_for_inferior handle exit case".

This may have been the case once upon a time, when GDB could only
handle one inferior at a time, but is certainly not the case any more,
we expect ::wait to return a valid ptid if it saw an event.

I believe that the fix for this issue is pretty easy, when we see the
exit event, just return a ptid containing the process-id only.

For testing, running something like gdb.base/break.exp is enough to
show the original problem, and this test now fully passes with this
patch applied.

I've not run the full testsuite on GNU/Hurd as there appear to be lots
of other issues with this target that makes running the full testsuite
very painful, but I think this looks like a small easy improvement.

Feedback welcome.
---
 gdb/gnu-nat.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index b7b486904e8..4086aeb4569 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -1603,7 +1603,10 @@ gnu_nat_target::wait (ptid_t ptid, struct target_waitstatus *status,
 							       available
 							       thread.  */
       else
-	ptid = inferior_ptid;	/* let wait_for_inferior handle exit case */
+	{
+	  /* The process exited. */
+	  ptid = ptid_t (inf->pid);
+	}
     }
 
   if (thread
-- 
2.25.4

Re: [PATCH] gdb/hurd: handle inferiors exiting

[John Baldwin]

On 1/6/22 7:41 AM, Andrew Burgess via Gdb-patches wrote:
> While testing on GNU/Hurd (i386) I noticed that GDB crashes when an
> inferior exits, with this error:
> 
>    inferior.c:293: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.
> 
> The problem appears to be in gnu_nat_target::wait.
> 
> When GDB is waiting for any active inferior to do something
> interesting, we pass in the null_ptid to indicate that the target
> should wait on all inferiors.
> 
> In gnu_nat_target::wait, when we get an exit event, we currently
> return the passed in ptid (null_ptid) and there's a comment in the
> code "let wait_for_inferior handle exit case".
> 
> This may have been the case once upon a time, when GDB could only
> handle one inferior at a time, but is certainly not the case any more,
> we expect ::wait to return a valid ptid if it saw an event.
> 
> I believe that the fix for this issue is pretty easy, when we see the
> exit event, just return a ptid containing the process-id only.
> 
> For testing, running something like gdb.base/break.exp is enough to
> show the original problem, and this test now fully passes with this
> patch applied.
> 
> I've not run the full testsuite on GNU/Hurd as there appear to be lots
> of other issues with this target that makes running the full testsuite
> very painful, but I think this looks like a small easy improvement.
> 
> Feedback welcome.

This looks fine to me.  The multi-target changes from Pedro stopped
setting inferior_ptid for target ::wait methods and this is fallout
from that I suspect.  In particular, when wanting to wait for multiple ptids
I think the core passes in minus_one_ptid or some other wildcard in 'ptid'
rather than null_ptid, so that part of the log message might not be quite
correct.

-- 
John Baldwin

Re: [PATCH] gdb/hurd: handle inferiors exiting

[Andrew Burgess]

* John Baldwin <jhb@FreeBSD.org> [2022-01-06 09:35:26 -0800]:

> On 1/6/22 7:41 AM, Andrew Burgess via Gdb-patches wrote:
> > While testing on GNU/Hurd (i386) I noticed that GDB crashes when an
> > inferior exits, with this error:
> > 
> >    inferior.c:293: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.
> > 
> > The problem appears to be in gnu_nat_target::wait.
> > 
> > When GDB is waiting for any active inferior to do something
> > interesting, we pass in the null_ptid to indicate that the target
> > should wait on all inferiors.
> > 
> > In gnu_nat_target::wait, when we get an exit event, we currently
> > return the passed in ptid (null_ptid) and there's a comment in the
> > code "let wait_for_inferior handle exit case".
> > 
> > This may have been the case once upon a time, when GDB could only
> > handle one inferior at a time, but is certainly not the case any more,
> > we expect ::wait to return a valid ptid if it saw an event.
> > 
> > I believe that the fix for this issue is pretty easy, when we see the
> > exit event, just return a ptid containing the process-id only.
> > 
> > For testing, running something like gdb.base/break.exp is enough to
> > show the original problem, and this test now fully passes with this
> > patch applied.
> > 
> > I've not run the full testsuite on GNU/Hurd as there appear to be lots
> > of other issues with this target that makes running the full testsuite
> > very painful, but I think this looks like a small easy improvement.
> > 
> > Feedback welcome.
> 
> This looks fine to me.  The multi-target changes from Pedro stopped
> setting inferior_ptid for target ::wait methods and this is fallout
> from that I suspect.  In particular, when wanting to wait for multiple ptids
> I think the core passes in minus_one_ptid or some other wildcard in 'ptid'
> rather than null_ptid, so that part of the log message might not be quite
> correct.

Thanks, I'll double check where the null_ptid I'm see is coming from.
By "seeing" I'm actually "seeing" it in debug print out, so maybe the
minus_one_ptid is just being printed that way.  Anyway, I'll clarify
the commit message.

Thanks for the feedback,
Andrew

Re: [PATCH] gdb/hurd: handle inferiors exiting

[Pedro Alves]

On 2022-01-06 17:35, John Baldwin wrote:
> On 1/6/22 7:41 AM, Andrew Burgess via Gdb-patches wrote:
>> While testing on GNU/Hurd (i386) I noticed that GDB crashes when an
>> inferior exits, with this error:
>>
>>    inferior.c:293: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.
>>
>> The problem appears to be in gnu_nat_target::wait.
>>
>> When GDB is waiting for any active inferior to do something
>> interesting, we pass in the null_ptid to indicate that the target
>> should wait on all inferiors.
>>
>> In gnu_nat_target::wait, when we get an exit event, we currently
>> return the passed in ptid (null_ptid) and there's a comment in the
>> code "let wait_for_inferior handle exit case".
>>
>> This may have been the case once upon a time, when GDB could only
>> handle one inferior at a time, but is certainly not the case any more,
>> we expect ::wait to return a valid ptid if it saw an event.
>>
>> I believe that the fix for this issue is pretty easy, when we see the
>> exit event, just return a ptid containing the process-id only.
>>
>> For testing, running something like gdb.base/break.exp is enough to
>> show the original problem, and this test now fully passes with this
>> patch applied.
>>
>> I've not run the full testsuite on GNU/Hurd as there appear to be lots
>> of other issues with this target that makes running the full testsuite
>> very painful, but I think this looks like a small easy improvement.
>>
>> Feedback welcome.
> 
> This looks fine to me.  The multi-target changes from Pedro stopped
> setting inferior_ptid for target ::wait methods and this is fallout
> from that I suspect.  

That is correct.  We set inferior_ptid to null_ptid before calling target_wait.
The event that ends up being reported is unrelated to the inferior/thread that was
selected before target_wait was called.  This change has caught a number
of spots in the target backends that assumed inferior_ptid was set to something.
While working on multi-target, I was running into a number of weird bugs because
of those, and the change to always clear inferior_ptid made them a lot more obvious.
This spot went unnoticed up till now.  

This is done by this bit in infrun.c:do_target_wait_1, btw:

  /* We know that we are looking for an event in the target of inferior
     INF, but we don't know which thread the event might come from.  As
     such we want to make sure that INFERIOR_PTID is reset so that none of
     the wait code relies on it - doing so is always a mistake.  */
  switch_to_inferior_no_thread (inf);


In particular, when wanting to wait for multiple ptids
> I think the core passes in minus_one_ptid or some other wildcard in 'ptid'
> rather than null_ptid, so that part of the log message might not be quite
> correct.
> 

You're right.

Re: [PATCH] gdb/hurd: handle inferiors exiting

[Andrew Burgess]

Thanks for all the feedback.  I pushed the patch below.

Thanks,
Andrew

---

commit 038d8b4635eda079a63df176cfa48c47f8c32617
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Jan 6 15:32:55 2022 +0000

    gdb/hurd: handle inferiors exiting
    
    While testing on GNU/Hurd (i386) I noticed that GDB crashes when an
    inferior exits, with this error:
    
      inferior.c:293: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.
    
    The problem appears to be in gnu_nat_target::wait.
    
    We always set inferior_ptid to null_ptid before calling target_wait,
    this has been the case since the multi-target changes were made to GDB
    in commit:
    
      commit 5b6d1e4fa4fc6827c7b3f0e99ff120dfa14d65d2
      Date:   Fri Jan 10 20:06:08 2020 +0000
    
          Multi-target support
    
    With follow up changes in commit:
    
      commit 24ed6739b699f329c2c45aedee5f8c7d2f54e493
      Date:   Thu Jan 30 14:35:40 2020 +0000
    
          gdb/remote: Restore support for 'S' stop reply packet
    
    Unfortunately, the GNU/Hurd target is still relying on the value of
    inferior_ptid in the case where an inferior exits - we return the
    value of inferior_ptid as the pid of the process that exited.  This
    was fine in the single target world, where inferior_ptid identified
    the one running inferior, but this is no longer good enough.
    
    Instead, we should return a ptid containing the pid of the process
    that exited, as obtained from the wait event, and this is what this
    commit does.
    
    I've not run the full testsuite on GNU/Hurd as there appear to be lots
    of other issues with this target that makes running the full testsuite
    very painful, but I think this looks like a small easy improvement.

diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index 1d3b5f1a357..9c53e3c0c2f 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -1603,7 +1603,10 @@ gnu_nat_target::wait (ptid_t ptid, struct target_waitstatus *status,
 							       available
 							       thread.  */
       else
-	ptid = inferior_ptid;	/* let wait_for_inferior handle exit case */
+	{
+	  /* The process exited. */
+	  ptid = ptid_t (inf->pid);
+	}
     }
 
   if (thread