[PATCH] Don't compare the pid returned from wait() against inferior_ptid.

[PATCH] Don't compare the pid returned from wait() against inferior_ptid.

[John Baldwin]

inf_ptrace::wait() needs to discard termination events reported by
detached child processes.  Previously it compared the returned pid
against the pid in inferior_ptid to determine if a termination event
should be discarded or reported.  inferior_ptid is now null_ptid in
wait() target methods, so this was always failing and never reporting
exit events.  Instead, report termination events whose pid matches any
inferior belonging to the current target.

gdb/ChangeLog:

	* inf-ptrace.c (inf_ptrace_target::wait): Don't compare against
	inferior_ptid.
---
 gdb/ChangeLog    | 5 +++++
 gdb/inf-ptrace.c | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c86d7e4647..d4cd0d014e 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2020-07-07  John Baldwin  <jhb@FreeBSD.org>
+
+	* inf-ptrace.c (inf_ptrace_target::wait): Don't compare against
+	inferior_ptid.
+
 2020-07-06  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	PR python/22748
diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index d25d226abb..ae0b0f7ff0 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -347,7 +347,7 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
 	}
 
       /* Ignore terminated detached child processes.  */
-      if (!WIFSTOPPED (status) && pid != inferior_ptid.pid ())
+      if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) == nullptr)
 	pid = -1;
     }
   while (pid == -1);
-- 
2.25.1

Re: [PATCH] Don't compare the pid returned from wait() against inferior_ptid.

[Gary Benson]

Hi John,

John Baldwin wrote:
> inf_ptrace::wait() needs to discard termination events reported by
> detached child processes.  Previously it compared the returned pid
> against the pid in inferior_ptid to determine if a termination event
> should be discarded or reported.  inferior_ptid is now null_ptid in
> wait() target methods, so this was always failing and never
> reporting exit events.  Instead, report termination events whose pid
> matches any inferior belonging to the current target.
> 
> gdb/ChangeLog:
> 
> 	* inf-ptrace.c (inf_ptrace_target::wait): Don't compare against
> 	inferior_ptid.

Did the bug you've fixed cause any tests to fail?  And, if not, is
this something it's possible to write a test for?

Cheers,
Gary

-- 
Gary Benson - he / him / his
Principal Software Engineer, Red Hat

Re: [PATCH] Don't compare the pid returned from wait() against inferior_ptid.

[John Baldwin]

On 7/8/20 2:48 AM, Gary Benson wrote:
> Hi John,
> 
> John Baldwin wrote:
>> inf_ptrace::wait() needs to discard termination events reported by
>> detached child processes.  Previously it compared the returned pid
>> against the pid in inferior_ptid to determine if a termination event
>> should be discarded or reported.  inferior_ptid is now null_ptid in
>> wait() target methods, so this was always failing and never
>> reporting exit events.  Instead, report termination events whose pid
>> matches any inferior belonging to the current target.
>>
>> gdb/ChangeLog:
>>
>> 	* inf-ptrace.c (inf_ptrace_target::wait): Don't compare against
>> 	inferior_ptid.
> 
> Did the bug you've fixed cause any tests to fail?  And, if not, is
> this something it's possible to write a test for?

It did cause regressions in some tests on FreeBSD.  I am still letting
a before-after testsuite run, but gdb.base/a2-run.exp at least failed
all tests due to this bug and is back to passing on FreeBSD with the fix.

-- 
John Baldwin

Re: [PATCH] Don't compare the pid returned from wait() against inferior_ptid.

[Simon Marchi]

On 2020-07-08 12:46 p.m., John Baldwin wrote:
> On 7/8/20 2:48 AM, Gary Benson wrote:
>> Hi John,
>>
>> John Baldwin wrote:
>>> inf_ptrace::wait() needs to discard termination events reported by
>>> detached child processes.  Previously it compared the returned pid
>>> against the pid in inferior_ptid to determine if a termination event
>>> should be discarded or reported.  inferior_ptid is now null_ptid in
>>> wait() target methods, so this was always failing and never
>>> reporting exit events.  Instead, report termination events whose pid
>>> matches any inferior belonging to the current target.
>>>
>>> gdb/ChangeLog:
>>>
>>> 	* inf-ptrace.c (inf_ptrace_target::wait): Don't compare against
>>> 	inferior_ptid.
>>
>> Did the bug you've fixed cause any tests to fail?  And, if not, is
>> this something it's possible to write a test for?
> 
> It did cause regressions in some tests on FreeBSD.  I am still letting
> a before-after testsuite run, but gdb.base/a2-run.exp at least failed
> all tests due to this bug and is back to passing on FreeBSD with the fix.

Here's the context, for Gary and others:

https://sourceware.org/pipermail/gdb-patches/2020-July/170238.html

In the commit message, you should point out that commit xYZ caused some
existing tests to fail, and that this patch makes them pass again.  This
way it's implied that there doesn't need to be new tests added, and it
gives a paper trail to understand why this fix has come to be needed.

Simon

Re: [PATCH] Don't compare the pid returned from wait() against inferior_ptid.

[Pedro Alves]

My Internet is down today (and mobile data is crawling slow so no
tethering, probably same root issue, as its the same provider), so reply
via mobile it is.

This is OK.

A Qua, 8/07/2020, 01:54, John Baldwin <jhb@freebsd.org> escreveu:

> inf_ptrace::wait() needs to discard termination events reported by
> detached child processes.  Previously it compared the returned pid
> against the pid in inferior_ptid to determine if a termination event
> should be discarded or reported.  inferior_ptid is now null_ptid in
> wait() target methods, so this was always failing and never reporting
> exit events.  Instead, report termination events whose pid matches any
> inferior belonging to the current target.
>
> gdb/ChangeLog:
>
>         * inf-ptrace.c (inf_ptrace_target::wait): Don't compare against
>         inferior_ptid.
> ---
>  gdb/ChangeLog    | 5 +++++
>  gdb/inf-ptrace.c | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index c86d7e4647..d4cd0d014e 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,8 @@
> +2020-07-07  John Baldwin  <jhb@FreeBSD.org>
> +
> +       * inf-ptrace.c (inf_ptrace_target::wait): Don't compare against
> +       inferior_ptid.
> +
>  2020-07-06  Andrew Burgess  <andrew.burgess@embecosm.com>
>
>         PR python/22748
> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
> index d25d226abb..ae0b0f7ff0 100644
> --- a/gdb/inf-ptrace.c
> +++ b/gdb/inf-ptrace.c
> @@ -347,7 +347,7 @@ inf_ptrace_target::wait (ptid_t ptid, struct
> target_waitstatus *ourstatus,
>         }
>
>        /* Ignore terminated detached child processes.  */
> -      if (!WIFSTOPPED (status) && pid != inferior_ptid.pid ())
> +      if (!WIFSTOPPED (status) && find_inferior_pid (this, pid) ==
> nullptr)
>         pid = -1;
>      }
>    while (pid == -1);
> --
> 2.25.1
>
>

Re: [PATCH] Don't compare the pid returned from wait() against inferior_ptid.

[John Baldwin]

On 7/8/20 9:52 AM, Simon Marchi wrote:
> On 2020-07-08 12:46 p.m., John Baldwin wrote:
>> On 7/8/20 2:48 AM, Gary Benson wrote:
>>> Hi John,
>>>
>>> John Baldwin wrote:
>>>> inf_ptrace::wait() needs to discard termination events reported by
>>>> detached child processes.  Previously it compared the returned pid
>>>> against the pid in inferior_ptid to determine if a termination event
>>>> should be discarded or reported.  inferior_ptid is now null_ptid in
>>>> wait() target methods, so this was always failing and never
>>>> reporting exit events.  Instead, report termination events whose pid
>>>> matches any inferior belonging to the current target.
>>>>
>>>> gdb/ChangeLog:
>>>>
>>>> 	* inf-ptrace.c (inf_ptrace_target::wait): Don't compare against
>>>> 	inferior_ptid.
>>>
>>> Did the bug you've fixed cause any tests to fail?  And, if not, is
>>> this something it's possible to write a test for?
>>
>> It did cause regressions in some tests on FreeBSD.  I am still letting
>> a before-after testsuite run, but gdb.base/a2-run.exp at least failed
>> all tests due to this bug and is back to passing on FreeBSD with the fix.
> 
> Here's the context, for Gary and others:
> 
> https://sourceware.org/pipermail/gdb-patches/2020-July/170238.html
> 
> In the commit message, you should point out that commit xYZ caused some
> existing tests to fail, and that this patch makes them pass again.  This
> way it's implied that there doesn't need to be new tests added, and it
> gives a paper trail to understand why this fix has come to be needed.

So after some git bisecting on the a2-run.exp test, I found that this
actually broke in the multi-target commit.  This is ironic as I had tested
that change pre-commit and given Pedro patches to fix it on the FreeBSD
native target, but clearly I didn't test it well enough.

FYI, about 400 tests are fixed on FreeBSD after applying this patch
and some other failures go back to failing for an original reason
instead of timeouts / GDB internal errors.

How's this for an updated log:

    Don't compare the pid returned from wait() against inferior_ptid.
    
    inf_ptrace::wait() needs to discard termination events reported by
    detached child processes.  Previously it compared the returned pid
    against the pid in inferior_ptid to determine if a termination event
    should be discarded or reported.  The multi-target changes cleared
    inferior_ptid to null_ptid in wait() target methods, so this was
    always failing and never reporting exit events.  Instead, report
    termination events whose pid matches any inferior belonging to the
    current target.
    
    Several tests started failing on FreeBSD after the multi-target
    changes and pass again after this change.


-- 
John Baldwin

Re: [PATCH] Don't compare the pid returned from wait() against inferior_ptid.

[Eli Zaretskii]

> From: John Baldwin <jhb@FreeBSD.org>
> Date: Thu, 9 Jul 2020 09:37:05 -0700
> Cc: Pedro Alves <palves@redhat.com>, gdb-patches@sourceware.org
> 
>     Don't compare the pid returned from wait() against inferior_ptid.
>     
>     inf_ptrace::wait() needs to discard termination events reported by
>     detached child processes.  Previously it compared the returned pid
>     against the pid in inferior_ptid to determine if a termination event
>     should be discarded or reported.  The multi-target changes cleared
>     inferior_ptid to null_ptid in wait() target methods, so this was
>     always failing and never reporting exit events.  Instead, report
>     termination events whose pid matches any inferior belonging to the
>     current target.

Thanks.

Please allow me a minor stylistic nit: using "foo()" to indicate a
function call is suboptimal, and potentially confusing: it looks like
calling a function with no arguments, which is not what you want.  It
is better to use 'foo' instead.