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

[Simon Marchi <simark@simark.ca>]

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.

Simon