Re: [PATCH] gdb/linux-nat: Fix mem access ptrace fallback (PR threads/31579)

[Pedro Alves <pedro@palves.net>]

On 2024-04-12 17:05, Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
> 
>>
>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>> index 2602e1f240d..d2b9aea724d 100644
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
> 
>> @@ -2199,6 +2199,21 @@ mark_lwp_dead (lwp_info *lp, int status)
>>    lp->stopped = 1;
>>  }
>>  
>> +/* Return true if LP is dead, with a pending exit/signalled event.  */
>> +
>> +static bool
>> +is_lwp_marked_dead (lwp_info *lp)
>> +{
>> +  switch (lp->waitstatus.kind ())
>> +    {
>> +    case TARGET_WAITKIND_EXITED:
>> +    case TARGET_WAITKIND_THREAD_EXITED:
>> +    case TARGET_WAITKIND_SIGNALLED:
>> +      return true;
>> +    }
>> +  return false;
>> +}
> 
> I wonder if this would be better as a method on waitstatus?  There's at
> least one place in infrun.c (in handle_one) where we check for these
> three statuses, and there's a few places where we check
> TARGET_WAITKIND_EXITED and TARGET_WAITKIND_SIGNALLED ... and I suspect
> we either _should_ be checking for TARGET_WAITKIND_THREAD_EXITED, or it
> wouldn't matter if we did.
> 
> Not that I'm suggesting you should look at all those ... just I'm
> wondering if it would be better to make is_lwp_marked_dead a waitstatus
> method now, and we can then clean up other users later?
> 

I've named the function is_lwp_marked_dead because we have the corresponding
mark_lwp_dead just above.  If we add a method to waitstatus, we'd naturally want
to call it something else, like ... I don't know, it's hard, maybe ws.is_exit_like()
or some such.  I think we'd still want the lwp function wrapping that, as the fact
that we mark is in the pending status field is a bit of a lower level implementation
detail than the semantics of the is_lwp_marked_dead function.

> Consider this an optional suggestion, we can always make this a method
> later if needed.

Thanks, seems worth it of exploration, but I'll take the option and go with what
I have in this patch.

> 
>> +
>>  /* Wait for LP to stop.  Returns the wait status, or 0 if the LWP has
>>     exited.  */
>>  
>> @@ -3879,6 +3894,20 @@ linux_proc_xfer_memory_partial (int pid, gdb_byte *readbuf,
>>  				const gdb_byte *writebuf, ULONGEST offset,
>>  				LONGEST len, ULONGEST *xfered_len);
>>  
>> +/* Look for an LWP of PID that we know is ptrace-stopped.  Returns
>> +   NULL if none is found.  */
> 
> s/NULL/nullptr/ ?

Personally, I prefer writing NULL in comments than nullptr, because saying (I mean,
imagine reading the comment aloud) "null ptr" is kind of awkward, in my mind.
It see it more as NULL meaning "the null pointer value", i.e., the word "null"
uppercased, than literally the NULL macro.  I've seen other patches from others
using NULL in comments so I thought it was other's preference as well.

> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks.