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

[Pedro Alves <pedro@palves.net>]

Hi Tom,

On 2024-06-21 10:43, Tom de Vries wrote:
> On 6/20/24 20:15, Pedro Alves wrote:
>> On 2024-06-17 19:22, Tom de Vries wrote:
>>> On 6/14/24 18:49, Pedro Alves wrote:
>>
>>>> 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.
>>>
>>
>> Aarch64 absolutely needs it, it's just that it already has the fix in place (by checking early in
>> post_startup_inferior/post_attach).  We're adding code to the other backends to handle it too, but using a
>> somewhat different solution.  If arm / ppc were being adjusted to use the same approach as aarch64 (like in a
>> previous patch in bugzilla), then I wouldn't have asked that question.  I see no good reason for multiple ways
>> of doing the same thing.
>>
> 
> To formulate it a bit differently: the fallback implementation enables only lazy implementations.  Aarch64 doesn't have a lazy implementation, so it doesn't need the fallback.  Both arm and ppc do have lazy implementations, which is why the fallback works for those.

Yes, but with the fix, the arm and ppc backends aren't really lazy anymore, as in, the code makes it look like it, but they aren't,
as we do the check always even with no watchpoints.  The lazy-support support code could be removed in the line of Aarch64, as being
unnecessary, which is what I was doing on ppc.

> 
> If we'd drop the calls to aarch64_linux_get_debug_reg_capacity from both aarch64_linux_nat_target::post_startup_inferior and aarch64_linux_nat_target::post_attach, the fallback wouldn't help, we'd have to a add a call to aarch64_linux_get_debug_reg_capacity in probably an override of can_use_hw_breakpoint.  In which case I prefer the current solution for aarch64.
> 
>>> There might be other targets that needs such a fallback, but that we don't know about.
>>
>> We have a testcase that will show us if so.
>>
> 
> To which my first thought is: And why not have a trivial fix that might work for some of those targets and make that test-case pass.
> 
> I suppose we're weighing a trade-off between:
> - reducing complexity by ensuring to do things in one way only, and
> - catering for an unknown subset of target implementations in a common
>   implementation.
> 
> I don't see a good way or principle to decide between those, my preference is clearly on the latter but I do understand the importance of the former.

I'm not against a fallback in principle.  I am more against the fallback being to call a higher level function and assuming that it works because
some other functions in the backend do things in a specific way (the lazy checking).  It's crossing abstractions boundaries, calling into the target stack,
assuming the right inferior is the current one, and overall more than what is needed.  We have a hook at the low level to do low level things
that only concern the architecture, and it seems to me way cleaner and future-design-proof to let it do the strict things that
are specific to the architecture.    That even allows getting rid of most of the now unnecessary lazy code as I was doing on ppc (ok, it needs
some tweaks).  Baking assumptions into the default kind of masks such things.

> 
> Anyway, since this is a point of contention, I didn't include it in the tested patch, so ... moving on.

Thank you.

>>> So, this is what I have tested on x86_64-linux, aarch64-linux, arm-linux and ppc64le-linux.
>>
>> OK, let's go with this, then.  Thank for testing!
> 
> I've submitted a v2 ( https://sourceware.org/pipermail/gdb-patches/2024-June/210138.html ), with comments a bit expanded, and commit log copied from v1 and updated.

I will take a look now.  Thanks again!