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

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

On 6/21/24 14:44, Pedro Alves wrote:
> 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.
> 

Ok, this makes sense to me now, thank you for the explanation.

- Tom

>>
>> 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!