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

[John Baldwin <jhb@FreeBSD.org>]

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.

> OOI did you manage to hit this case during your work on this series?
> I guess from your comments you did, is that something that's still
> repeatable, or was this only when you had GDB in a broken state?

I think I only hit this when I had a bug earlier in this series and found
that the code was not only sketchy (in my mind, if you go back in annotate
it claims to be a workaround for a kernel bug added 20+ years ago), it no
longer worked after multi target as it always trips an assertion since
it effectively returns an event for null_ptid.

-- 
John Baldwin