Re: [PATCH 02/23] Don't rely on inferior_ptid in record_full_wait

[John Baldwin <jhb@FreeBSD.org>]

On 8/1/20 9:14 AM, Simon Marchi wrote:
> On 2020-07-30 11:17 p.m., Tom Tromey wrote:
>>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>>
>> Pedro> The multi-target patch sets inferior_ptid to null_ptid before handling
>> Pedro> a target event, and thus before calling target_wait, in order to catch
>> Pedro> places in target_ops::wait implementations that are incorrectly
>> Pedro> relying on inferior_ptid (which could otherwise be a ptid of a
>> Pedro> different target, for example).  That caught this instance in
>> Pedro> record-full.c.
>>
>> I found a few target_ops::wait implementations doing "return
>> inferior_ptid" in error cases.  Based on this comment, and the comment
>> for target_wait, I suspect these should actually return minus_one_ptid
>> instead.
>>
>> I've appended the patch so you can see what it looks like.  I haven't
>> tried it at all.  Does this seem correct?
> 
> I think you are right that it's incorrect, but...
> 
>> diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
>> index ae0b0f7ff0d..2cae87023f9 100644
>> --- a/gdb/inf-ptrace.c
>> +++ b/gdb/inf-ptrace.c
>> @@ -343,7 +343,7 @@ inf_ptrace_target::wait (ptid_t ptid, struct target_waitstatus *ourstatus,
>>  	  /* Claim it exited with unknown signal.  */
>>  	  ourstatus->kind = TARGET_WAITKIND_SIGNALLED;
>>  	  ourstatus->value.sig = GDB_SIGNAL_UNKNOWN;
>> -	  return inferior_ptid;
>> +	  return minus_one_ptid;
>>  	}
> 
> I don't know if this makes sense: you tell the core of GDB that some
> process was signalled (terminated by a signal), but you don't tell
> what process it is.  I am not sure what the core of GDB can do with
> this information.
> 
> The contract that the `wait` implementations must respect is not
> very well documented I believe, so it's not clear which value is
> valid with which event.  But I expect that tying a
> TARGET_WAITKIND_SIGNALLED event with a minus_one_ptid (or null_ptid
> for that matter) is invalid, and will just break down later in
> handle_inferior_event, where we handle this kind of event:
> 
>     case TARGET_WAITKIND_EXITED:
>     case TARGET_WAITKIND_SIGNALLED:
>       {
> 	/* Depending on the system, ecs->ptid may point to a thread or
> 	   to a process.  On some targets, target_mourn_inferior may
> 	   need to have access to the just-exited thread.  That is the
> 	   case of GNU/Linux's "checkpoint" support, for example.
> 	   Call the switch_to_xxx routine as appropriate.  */
> 	thread_info *thr = find_thread_ptid (ecs->target, ecs->ptid);
> 	if (thr != nullptr)
> 	  switch_to_thread (thr);
> 	else
> 	  {
> 	    inferior *inf = find_inferior_ptid (ecs->target, ecs->ptid);
> 	    switch_to_inferior_no_thread (inf);
> 	  }
> 
> find_thread_ptid will return nullptr, so we'll go in the else.
> find_inferior_ptid will return nullptr, which we'll pass to
> switch_to_inferior_no_thread, and it will assert somewhere in there.

Note that prior to my fix to inf-ptrace.c in f37e5866aa, I was tripping
over this case and it did indeed result in an assertion failure as
wait was returning null_ptid.  I kind of think an error like an assertion
failure is the right behavior as it's really an indication of a bug.
Arguably though inf-ptrace.c should not return null_ptid that later
triggers an obscure assertion failure, but instead it should probably
just die with an explicit error() or the like.  It's not really clear
to me that we should be trying to do anything to gracefully handle an
ENOCHILD error.

-- 
John Baldwin