On 6/14/24 18:49, Pedro Alves wrote:
> Hi!
> 
> Sorry for the delay.  I've been super swamped.  :-/
> 

Hi Pedro,

thanks for the review.

> On 2024-06-07 07:35, Tom de Vries wrote:
> 
>> PR tdep/31834
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31834
>> PR tdep/31705
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31705
>> ---
>>   gdb/linux-nat.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>> index c95d420d416..d8b5a99269b 100644
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>> @@ -454,6 +454,18 @@ linux_init_ptrace_procfs (pid_t pid, int attached)
>>     linux_ptrace_init_warnings ();
>>     linux_proc_init_warnings ();
>>     proc_mem_file_is_writable ();
>> +
>> +  /* Some targets (for instance ppc and arm) may call ptrace to answer a
>> +     target_can_use_hardware_watchpoint query, and cache the result.  However,
>> +     the ptrace call will fail with errno ESRCH if the tracee is not
>> +     ptrace-stopped, making the query fail.  And if the caching mechanism does
>> +     not disregard an ESRCH result, all subsequent queries will also fail.
>> +     Call it now, where we known the tracee is ptrace-stopped.
>> +
>> +     Other targets (for instance aarch64) do the relevant ptrace call and
>> +     caching in their implementation of post_attach and post_startup_inferior,
>> +     in which case this call is expected to have no effect.  */
>> +  target_can_use_hardware_watchpoint (bp_hardware_watchpoint, 1, 0);
> 
> To be honest, I kind of preferred the other version of the patch.  This is a single call,
> yes, but then you have to explain details about the different backend implementations,
> anyhow, and it raises questions like, what if bp_hardware_watchpoint is the right
> type?  What if some architecture caches the resources for bp_hardware_breakpoint
> differently?
> 

I did think about that, and as a solution considered looping over all 
types of breakpoints.  But it seemed somewhat of an overkill, so I went 
with just bp_hardware_watchpoint.

Of course, if your specific concern is bp_hardware_breakpoint, then this:
...
   target_can_use_hardware_watchpoint (bp_hardware_watchpoint, 1, 0);
   target_can_use_hardware_watchpoint (bp_hardware_breakpoint, 1, 0);
...
addresses that.

> And, from another angle, why isn't aarch64 doing the same, why two mechanisms?

Well, the patch adds a fallback, that aarch64 doesn't need, but that 
powerpc and arm do need.  There might be other targets that needs such a 
fallback, but that we don't know about.

So, I don't mind your patch, it's certainly cleaner, but I don't mind a 
functional default implementation either.  So, I'd move the 
target_can_use_hardware_watchpoint call to the default implementation of 
low_init_process.

We should consider fixing things in a way that minimizes efforts for 
target maintainers.

> I guess the wart with the other approach would be that you have to handle
> this from both post_startup_inferior and post_attach?  I think we can fix
> that -- add a new low_init_process method that is called in both scenarios,
> where the backend can do what it needs to.
> 
> I was going to write small draft patch that just adds the method in question,
> for discussion, but then as I was already looking at the code, I ended up
> implementing the arm, aarch64, ppc backend versions of it.  I noticed
> that all the m_dreg_interface.detect and m_dreg_interface.detected_p
> calls throughout ppc-linux-nat.c could be removed, since we now
> always call m_dreg_interface.detect() early.
> 
> I only build-tested this on x86_64, which of course is not sufficient
> testing.
> 
> Overall it's a net reduction of code, which seems nice to me.
> 

I ran into trouble building the patch due to type of pid parameter 
mismatches.

After fixing that, I ran into trouble on ppc64le, because 
low_prepare_to_resume is called before low_init_process.  I fixed that 
by sinking this code in the function a bit:
...
   if (m_dreg_interface.unavailable_p ())
     return;
...

And after fixing this, I still ran into failures and identified at least 
two more locations that needed fixing due to the cleanup, at which point 
I decided that the cleanup part is out-of-scope for the patch fixing the PR.

So, this is what I have tested on x86_64-linux, aarch64-linux, arm-linux 
and ppc64le-linux.

Thanks,
- Tom