Re: [PATCH v2 06/13] inf-ptrace: Raise an internal_error if waitpid() fails.

[John Baldwin <jhb@FreeBSD.org>]

On 11/24/21 12:16 PM, Simon Marchi wrote:
> On 2021-11-24 9:16 a.m., Andrew Burgess via Gdb-patches wrote:
>> * John Baldwin <jhb@FreeBSD.org> [2021-08-03 11:49:53 -0700]:
>>
>>> Previously this returned a TARGET_WAITKIND_SIGNALLED event for
>>> inferior_ptid.  Since the multi-target changes, inferior_ptid is now
>>> invalid during ::wait() methods, so this triggered an assertion
>>> further up the stack.
>>> ---
>>>   gdb/inf-ptrace.c | 11 +++--------
>>>   1 file changed, 3 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
>>> index afa38de6ef7..1f8e72d1aca 100644
>>> --- a/gdb/inf-ptrace.c
>>> +++ b/gdb/inf-ptrace.c
>>> @@ -319,14 +319,9 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>>>
>>>         if (pid == -1)
>>>   	{
>>> -	  fprintf_unfiltered (gdb_stderr,
>>> -			      _("Child process unexpectedly missing: %s.\n"),
>>> -			      safe_strerror (save_errno));
>>> -
>>> -	  /* Claim it exited with unknown signal.  */
>>> -	  ourstatus->kind = TARGET_WAITKIND_SIGNALLED;
>>> -	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
>>> -	  return inferior_ptid;
>>> +	  internal_error (__FILE__, __LINE__,
>>> +			  _("Child process unexpectedly missing: %s.\n"),
>>> +			  safe_strerror (save_errno));
>>>   	}
>>
>> Single statement if blocks should not have '{ ... }' around them.
>>
>> I wonder if we could use perror_with_name here instead of
>> internal_error, there's at least one place this is done in
>> linux-nat.c.
> 
> If we are being pedantic about the uses of internal_error vs throwing an
> error: factors external to GDB should not be able to cause an internal
> error.  The kernel is an external factor: a bug in the kernel could
> cause this branch to be taken, so it should probably not make GDB
> internal error.  In that regard, perror_with_name would be better.
> Although I would be fine with assuming that the kernel does not have
> bugs (except when actively working around known bugs).
> 
> Otherwise, I would suggest using gdb_assert_not_reached instead of
> internal_error, to achieve the same thing.

I think treating this as a kernel bug (and thus perror_with_name) is fine.
The only wrinkle is having to set errno back to save_errno explicitly before
calling perror_with_name, but that is minor.

-- 
John Baldwin