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

[John Baldwin <jhb@FreeBSD.org>]

On 12/1/21 4:04 AM, Andrew Burgess wrote:
> * John Baldwin <jhb@FreeBSD.org> [2021-11-25 08:38:16 -0800]:
> 
>> On 11/25/21 2:30 AM, Andrew Burgess wrote:
>>> * John Baldwin <jhb@FreeBSD.org> [2021-11-24 14:00:16 -0800]:
>>>
>>>> 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.
>>>
>>> I don't think that perror_with_name implies this is a kernel bug.  The
>>> perror function is just a wrapper around 'error' and safe_strerror -
>>> so basically what you have already except using error, not
>>> internal_error.
>>>
>>> Though I guess if we get -1 back then this implies something has gone
>>> wrong somewhere, right?  We expect to see some event for the child no
>>> matter how the kernel kills it, so, if we get -1, that implies the
>>> child disappeared without a trace, right?  But possibly the bug could
>>> be that we already collected the event, and lost it somehow, so it
>>> could be a GDB bug...
>>
>> Yes, it could be either one.  I had chosen internal_error to replace the
>> existing fprintf somewhat arbitrarily.  I don't have a strong opinion
>> on what type of error it should be, only that it is best to throw the
>> error closer to the real problem than to get a later assertion
>> failure.
> 
> For this sort of situation we should assume GDB doesn't have bugs, and
> that this situation must be caused by an external problem, so
> `error`.  If we later find there are bugs in GDB, we'd then fix them.

To be clear, is perror_with_name what you'd prefer here or explicitly
using error()?

-- 
John Baldwin