Re: [PATCH] [gdb/tdep] Fix gdb.base/watchpoint-running on {arm, ppc64le}-linux

[Tom de Vries <tdevries@suse.de>]

On 6/17/24 20:22, Tom de Vries wrote:
> 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.
> 

Hi Pedro,

does the tested patch look acceptable?

If so, I can do an actual submission.  I'm on vacation for three weeks 
starting coming Saturday, so I might have time for that tomorrow.

I'd like this fix to be in gdb 15, I'm not sure what the time-line is on 
that though.

Thanks,
- Tom