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

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

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.

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.

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

>> 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.
> 
> I'd rather not.  Let low level handle low level things using low level details.
> 
>> 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 guess it's while running through the shell, now that I think about it.
> 
>> I fixed that by sinking this code in the function a bit:
>> ...
>>    if (m_dreg_interface.unavailable_p ())
>>      return;
>> ...
> 
> Makes sense.
> 
>>
>> 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.
> 
> OK.
> 
>>
>> 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.

Thanks,
- Tom