was Re: [PATCH 2/3] [delete] Not-so-harmless spurious call to `wait4`

Message ID 133F84AF-3B74-47B1-BEA2-830151901ADE@epfl.ch
State New
Headers show
Series
  • was Re: [PATCH 2/3] [delete] Not-so-harmless spurious call to `wait4`
Related show

Commit Message

Lancelot SIX via Gdb-patches April 8, 2021, 8:25 p.m.
> Le 8 avr. 2021 à 21:26, Simon Marchi <simon.marchi@polymtl.ca> a écrit :

> 

>   Can you please explain in the commit message what harm

> that wait4 call does and why we want to remove it?



In trying to make a better case in the commit message, I found that swapping the two patches around made more sense. Here is an updated, rebased, single patch (working under the assumption that the other two will be merged first):

From 5c3756e9eb0342b1a5a23bcb54d5b8317743ce0d Mon Sep 17 00:00:00 2001
From: Dominique Quatravaux <dominique.quatravaux@epfl.ch>

Date: Thu, 8 Apr 2021 21:35:57 +0200
Subject: [PATCH] [delete] not-so-harmless spurious call to `wait4`
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

As seen in https://sourceware.org/bugzilla/show_bug.cgi?id=24069 this
code will typically wait4() a second time on the same process that was
already wait4()'d a few lines above. While this used to be
harmless/idempotent (when we assumed that the process already exited),
this now causes a deadlock in the WIFSTOPPED case.

The early (~2019) history of bug #24069 cautiously suggests to use
WNOHANG instead of outright deleting the call. However, tests on the
current version of Darwin (Big Sur) demonstrate that gdb runs just
fine without a redundant call to wait4(), as would be expected.
Notwithstanding the debatable value of conserving bug compatibility
with an OS release that is more than a decade old, there is scant
evidence of what that double-wait4() was supposed to achieve in the
first place - A cursory investigation with `git blame` pinpoints
commits bb00b29d7802 and a80b95ba67e2 from the 2008-2009 era, but
fails to answer the “why” question conclusively.
---
 gdb/darwin-nat.c | 3 ---
 1 file changed, 3 deletions(-)

-- 
2.31.1



— 
  Dominique Quatravaux
  +41 21 69 35624

Comments

Lancelot SIX via Gdb-patches April 9, 2021, 2:34 p.m. | #1
On 2021-04-08 4:25 p.m., Dominique Quatravaux wrote:> 
> 

>> Le 8 avr. 2021 à 21:26, Simon Marchi <simon.marchi@polymtl.ca <mailto:simon.marchi@polymtl.ca>> a écrit :

>>

>>   Can you please explain in the commit message what harm

>> that wait4 call does and why we want to remove it?

> 

> In trying to make a better case in the commit message, I found that swapping the two patches around made more sense. Here is an updated, rebased, single patch (working under the assumption that the other two will be merged first):

> 

> From 5c3756e9eb0342b1a5a23bcb54d5b8317743ce0d Mon Sep 17 00:00:00 2001

> From: Dominique Quatravaux <dominique.quatravaux@epfl.ch <mailto:dominique.quatravaux@epfl.ch>>

> Date: Thu, 8 Apr 2021 21:35:57 +0200

> Subject: [PATCH] [delete] not-so-harmless spurious call to `wait4`

> MIME-Version: 1.0

> Content-Type: text/plain; charset=UTF-8

> Content-Transfer-Encoding: 8bit

> 

> As seen in https://sourceware.org/bugzilla/show_bug.cgi?id=24069 <https://sourceware.org/bugzilla/show_bug.cgi?id=24069> this

> code will typically wait4() a second time on the same process that was

> already wait4()'d a few lines above. While this used to be

> harmless/idempotent (when we assumed that the process already exited),

> this now causes a deadlock in the WIFSTOPPED case.

> 

> The early (~2019) history of bug #24069 cautiously suggests to use

> WNOHANG instead of outright deleting the call. However, tests on the

> current version of Darwin (Big Sur) demonstrate that gdb runs just

> fine without a redundant call to wait4(), as would be expected.

> Notwithstanding the debatable value of conserving bug compatibility

> with an OS release that is more than a decade old, there is scant

> evidence of what that double-wait4() was supposed to achieve in the

> first place


Thanks, this looks good.  If we had more contributors for macOS,
especially some that cared about old macOS versions, we could aim at
supporting many versions back.  But given the current state, it's
perfectly reasonable to aim at making GDB work well for the latest
version.

 - A cursory investigation with `git blame` pinpoints
> commits bb00b29d7802 and a80b95ba67e2 from the 2008-2009 era, but

> fails to answer the “why” question conclusively.


Yes, the commits that predate the usage of git don't have the patch
message / rationale that people sent along with the patch, it just
wasn't recorded in CVS.  You can always dig into into the mailing
list.

a80b95ba67e2: https://pi.simark.ca/gdb-patches/F1826DD8-CC0F-4FF1-BC47-3F2ACBB42909@adacore.com/
bb00b29d7802: https://pi.simark.ca/gdb-patches/20090319141746.GA81236@ulanbator.act-europe.fr/

Although there isn't more details about the bit you are removing.

So let's just wait until we hear back from the FSF about your copyright
assignment, and then we can merge all of this.

Note that we currently require contributions to include ChangeLog
entries:

    https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog

I don't mind for this time, the patches are small enough that I can
write them when merging.  We are also discussing whether we keep using
those, so it may or may not apply in the future.

Simon

Patch

diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
index d3edcdf3a74..ac19d330ffb 100644
--- a/gdb/darwin-nat.c
+++ b/gdb/darwin-nat.c
@@ -1113,9 +1113,6 @@  darwin_nat_target::decode_message (mach_msg_header_t *hdr,
 			      res_pid, status->value.sig);
 		}
 
-	      /* Looks necessary on Leopard and harmless...  */
-	      wait4 (inf->pid, &wstatus, 0, NULL);
-
 	      return ptid_t (inf->pid);
 	    }
 	  else