Re: [RFC][PATCH v2 2/2][PR gdb/24069] [fix] Skip over WIFSTOPPED wait4 status

[Philippe Blain via Gdb-patches <gdb-patches@sourceware.org>]

Hi Simon,

Le 2022-02-19 à 10:59, Simon Marchi a écrit :
>> diff --git a/gdb/darwin-nat.c b/gdb/darwin-nat.c
>> index 8b0ecfd5b77..a3c6a978676 100644
>> --- a/gdb/darwin-nat.c
>> +++ b/gdb/darwin-nat.c
>> @@ -1063,7 +1063,7 @@ darwin_nat_target::decode_message (mach_msg_header_t *hdr,
>>      }
>>    else if (hdr->msgh_id == 0x48)
>>      {
>> -      /* MACH_NOTIFY_DEAD_NAME: notification for exit.  */
>> +      /* MACH_NOTIFY_DEAD_NAME: notification for exit *or* WIFSTOPPED. */
> 
> Two spaces after period.
> 
>>        int res;
>>  
>>        res = darwin_decode_notify_message (hdr, &inf);
>> @@ -1103,16 +1103,26 @@ darwin_nat_target::decode_message (mach_msg_header_t *hdr,
>>  		  return minus_one_ptid;
>>  		}
>>  	      if (WIFEXITED (wstatus))
>> -		status->set_exited (WEXITSTATUS (wstatus));
>> +		{
>> +		  status->set_exited (WEXITSTATUS (wstatus));
>> +	          inferior_debug (4, _("darwin_wait: pid=%d exit, status=0x%x\n"),
>> +				  res_pid, wstatus);
>> +		}
>> +	      else if (WIFSTOPPED (wstatus))
>> +		{
>> +		  status->set_ignore ();
>> +		  inferior_debug (4, _("darwin_wait: pid %d received WIFSTOPPED\n"),
>> +				  res_pid);
>> +		  return minus_one_ptid;
> 
> Can you add a comment explaining that the stop will be handled by a
> following exception?  It can be confusing why we ignore a WIFSTOPPED.  A
> bit like you explained in your commit message:
> 
>  - Read and ignore such messages (counting on the next exception
>  message to let us know of the inferior's new state again)
> 
> 
> 
>> +		}
>>  	      else
>>  		{
>>  		  status->set_signalled
>>  		    (gdb_signal_from_host (WTERMSIG (wstatus)));
>> +		  inferior_debug (4, _("darwin_wait: pid=%d received signal %d\n"),
>> +				  res_pid, status->sig());
>>  		}
> 
> Can you replace the else with
> 
>   else if (WIFSIGNALED (wstatus))
> 
> and add an else with a warning like:
> 
>   warning (_("Unexpected wait status after MACH_NOTIFY_DEAD_NAME "
>              "notification: 0x%x"), wstatus);
> 
> I think that having this warning would have made the bug you fix more
> visible, so easier to find.  Perhaps it will help catch another similar
> problem in the future.
> 
> The else should probably do status->set_ignore () and return
> minus_one_ptid.
> 
> Simon
> 

I'll address your feedback and send a v3 for 2/2. Thanks!

Philippe.