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

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

[Tom de Vries]

When running test-case gdb.base/watchpoint-running on ppc64le-linux, we get:
...
(gdb) watch global_var^M
warning: Error when detecting the debug register interface. \
  Debug registers will be unavailable.^M
Watchpoint 2: global_var^M
(gdb) FAIL: $exp: all-stop: hardware: watch global_var
FAIL: $exp: all-stop: hardware: watchpoint hit (timeout)
...

The problem is that ppc_linux_dreg_interface::detect fails to detect the
hardware watchpoint interface, because the calls to ptrace return with errno
set to ESRCH.

This is a feature of ptrace: if a call is done while the tracee is not
ptrace-stopped, it returns ESRCH.

Indeed, in the test-case "watch global_var" is executed while the inferior is
running, and that triggers the first call to ppc_linux_dreg_interface::detect.

And because the detection failure is cached, subsequent attempts at setting
hardware watchpoints will also fail, even if the tracee is ptrace-stopped.

Fix this by calling target_can_use_hardware_watchpoint from
linux_init_ptrace_procfs, which is called from both:
- linux_nat_target::post_attach, and
- linux_nat_target::post_startup_inferior.

By fixing this here, we also fix the same problem for arm-linux.

Tested on ppc64le-linux and arm-linux.

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);
 }
 
 linux_nat_target::~linux_nat_target ()

base-commit: f9478936896ada7786e8d68622f6e6ff78b97b0d
-- 
2.35.3

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

[Luis Machado]

On 6/7/24 07:35, Tom de Vries wrote:
> When running test-case gdb.base/watchpoint-running on ppc64le-linux, we get:
> ...
> (gdb) watch global_var^M
> warning: Error when detecting the debug register interface. \
>   Debug registers will be unavailable.^M
> Watchpoint 2: global_var^M
> (gdb) FAIL: $exp: all-stop: hardware: watch global_var
> FAIL: $exp: all-stop: hardware: watchpoint hit (timeout)
> ...
> 
> The problem is that ppc_linux_dreg_interface::detect fails to detect the
> hardware watchpoint interface, because the calls to ptrace return with errno
> set to ESRCH.
> 
> This is a feature of ptrace: if a call is done while the tracee is not
> ptrace-stopped, it returns ESRCH.
> 
> Indeed, in the test-case "watch global_var" is executed while the inferior is
> running, and that triggers the first call to ppc_linux_dreg_interface::detect.
> 
> And because the detection failure is cached, subsequent attempts at setting
> hardware watchpoints will also fail, even if the tracee is ptrace-stopped.
> 
> Fix this by calling target_can_use_hardware_watchpoint from
> linux_init_ptrace_procfs, which is called from both:
> - linux_nat_target::post_attach, and
> - linux_nat_target::post_startup_inferior.
> 
> By fixing this here, we also fix the same problem for arm-linux.
> 
> Tested on ppc64le-linux and arm-linux.
> 
> 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);
>  }
>  
>  linux_nat_target::~linux_nat_target ()
> 
> base-commit: f9478936896ada7786e8d68622f6e6ff78b97b0d

Looks good from arm-linux's side. Thanks!

Reviewed-By: Luis Machado <luis.machado@arm.com>
Tested-By: Luis Machado <luis.machado@arm.com>

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

[Tom de Vries]

On 6/7/24 12:18, Luis Machado wrote:
> On 6/7/24 07:35, Tom de Vries wrote:
>> When running test-case gdb.base/watchpoint-running on ppc64le-linux, we get:
>> ...
>> (gdb) watch global_var^M
>> warning: Error when detecting the debug register interface. \
>>    Debug registers will be unavailable.^M
>> Watchpoint 2: global_var^M
>> (gdb) FAIL: $exp: all-stop: hardware: watch global_var
>> FAIL: $exp: all-stop: hardware: watchpoint hit (timeout)
>> ...
>>
>> The problem is that ppc_linux_dreg_interface::detect fails to detect the
>> hardware watchpoint interface, because the calls to ptrace return with errno
>> set to ESRCH.
>>
>> This is a feature of ptrace: if a call is done while the tracee is not
>> ptrace-stopped, it returns ESRCH.
>>
>> Indeed, in the test-case "watch global_var" is executed while the inferior is
>> running, and that triggers the first call to ppc_linux_dreg_interface::detect.
>>
>> And because the detection failure is cached, subsequent attempts at setting
>> hardware watchpoints will also fail, even if the tracee is ptrace-stopped.
>>
>> Fix this by calling target_can_use_hardware_watchpoint from
>> linux_init_ptrace_procfs, which is called from both:
>> - linux_nat_target::post_attach, and
>> - linux_nat_target::post_startup_inferior.
>>
>> By fixing this here, we also fix the same problem for arm-linux.
>>
>> Tested on ppc64le-linux and arm-linux.
>>
>> 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);
>>   }
>>   
>>   linux_nat_target::~linux_nat_target ()
>>
>> base-commit: f9478936896ada7786e8d68622f6e6ff78b97b0d
> 
> Looks good from arm-linux's side. Thanks!
> 
> Reviewed-By: Luis Machado <luis.machado@arm.com>
> Tested-By: Luis Machado <luis.machado@arm.com>

Luis, thanks for the review and the testing.

Pedro, since you reviewed a target-specific patch for the arm PR, any 
comments?

Thanks,
- Tom

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

[Tom de Vries]

On 6/7/24 14:05, Tom de Vries wrote:
> On 6/7/24 12:18, Luis Machado wrote:
>> On 6/7/24 07:35, Tom de Vries wrote:
>>> When running test-case gdb.base/watchpoint-running on ppc64le-linux, 
>>> we get:
>>> ...
>>> (gdb) watch global_var^M
>>> warning: Error when detecting the debug register interface. \
>>>    Debug registers will be unavailable.^M
>>> Watchpoint 2: global_var^M
>>> (gdb) FAIL: $exp: all-stop: hardware: watch global_var
>>> FAIL: $exp: all-stop: hardware: watchpoint hit (timeout)
>>> ...
>>>
>>> The problem is that ppc_linux_dreg_interface::detect fails to detect the
>>> hardware watchpoint interface, because the calls to ptrace return 
>>> with errno
>>> set to ESRCH.
>>>
>>> This is a feature of ptrace: if a call is done while the tracee is not
>>> ptrace-stopped, it returns ESRCH.
>>>
>>> Indeed, in the test-case "watch global_var" is executed while the 
>>> inferior is
>>> running, and that triggers the first call to 
>>> ppc_linux_dreg_interface::detect.
>>>
>>> And because the detection failure is cached, subsequent attempts at 
>>> setting
>>> hardware watchpoints will also fail, even if the tracee is 
>>> ptrace-stopped.
>>>
>>> Fix this by calling target_can_use_hardware_watchpoint from
>>> linux_init_ptrace_procfs, which is called from both:
>>> - linux_nat_target::post_attach, and
>>> - linux_nat_target::post_startup_inferior.
>>>
>>> By fixing this here, we also fix the same problem for arm-linux.
>>>
>>> Tested on ppc64le-linux and arm-linux.
>>>
>>> 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);
>>>   }
>>>   linux_nat_target::~linux_nat_target ()
>>>
>>> base-commit: f9478936896ada7786e8d68622f6e6ff78b97b0d
>>
>> Looks good from arm-linux's side. Thanks!
>>
>> Reviewed-By: Luis Machado <luis.machado@arm.com>
>> Tested-By: Luis Machado <luis.machado@arm.com>
> 
> Luis, thanks for the review and the testing.
> 
> Pedro, since you reviewed a target-specific patch for the arm PR, any 
> comments?

And just to mention this, I'd like to backport this fix to the 
gdb-15-branch.

Thanks,
- Tom

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

[Luis Machado]

On 6/13/24 10:07, Tom de Vries wrote:
> On 6/7/24 14:05, Tom de Vries wrote:
>> On 6/7/24 12:18, Luis Machado wrote:
>>> On 6/7/24 07:35, Tom de Vries wrote:
>>>> When running test-case gdb.base/watchpoint-running on ppc64le-linux, we get:
>>>> ...
>>>> (gdb) watch global_var^M
>>>> warning: Error when detecting the debug register interface. \
>>>>    Debug registers will be unavailable.^M
>>>> Watchpoint 2: global_var^M
>>>> (gdb) FAIL: $exp: all-stop: hardware: watch global_var
>>>> FAIL: $exp: all-stop: hardware: watchpoint hit (timeout)
>>>> ...
>>>>
>>>> The problem is that ppc_linux_dreg_interface::detect fails to detect the
>>>> hardware watchpoint interface, because the calls to ptrace return with errno
>>>> set to ESRCH.
>>>>
>>>> This is a feature of ptrace: if a call is done while the tracee is not
>>>> ptrace-stopped, it returns ESRCH.
>>>>
>>>> Indeed, in the test-case "watch global_var" is executed while the inferior is
>>>> running, and that triggers the first call to ppc_linux_dreg_interface::detect.
>>>>
>>>> And because the detection failure is cached, subsequent attempts at setting
>>>> hardware watchpoints will also fail, even if the tracee is ptrace-stopped.
>>>>
>>>> Fix this by calling target_can_use_hardware_watchpoint from
>>>> linux_init_ptrace_procfs, which is called from both:
>>>> - linux_nat_target::post_attach, and
>>>> - linux_nat_target::post_startup_inferior.
>>>>
>>>> By fixing this here, we also fix the same problem for arm-linux.
>>>>
>>>> Tested on ppc64le-linux and arm-linux.
>>>>
>>>> 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);
>>>>   }
>>>>   linux_nat_target::~linux_nat_target ()
>>>>
>>>> base-commit: f9478936896ada7786e8d68622f6e6ff78b97b0d
>>>
>>> Looks good from arm-linux's side. Thanks!
>>>
>>> Reviewed-By: Luis Machado <luis.machado@arm.com>
>>> Tested-By: Luis Machado <luis.machado@arm.com>
>>
>> Luis, thanks for the review and the testing.
>>
>> Pedro, since you reviewed a target-specific patch for the arm PR, any comments?
> 
> And just to mention this, I'd like to backport this fix to the gdb-15-branch.
> 
> Thanks,
> - Tom
> 

That's OK by me from arm's side.

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

[Pedro Alves]

Hi!

Sorry for the delay.  I've been super swamped.  :-/

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?

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

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.

Pedro Alves

From e4e232efc509f56ae9d183d69c77d8d3f3024452 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 14 Jun 2024 17:15:43 +0100
Subject: [PATCH] foo

Change-Id: I25bc362da9e9c6ca3ad3a81395b5d442cba5febf
---
 gdb/aarch64-linux-nat.c | 30 +++++------------------
 gdb/arm-linux-nat.c     |  7 ++++++
 gdb/linux-nat.c         |  6 +++++
 gdb/linux-nat.h         |  6 +++++
 gdb/ppc-linux-nat.c     | 54 +++++++++++++----------------------------
 5 files changed, 42 insertions(+), 61 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 4b2a0ba9f7b..c4f9f6c2f48 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -78,12 +78,6 @@ class aarch64_linux_nat_target final
 
   int can_do_single_step () override;
 
-  /* Override the GNU/Linux inferior startup hook.  */
-  void post_startup_inferior (ptid_t) override;
-
-  /* Override the GNU/Linux post attach hook.  */
-  void post_attach (int pid) override;
-
   /* These three defer to common nat/ code.  */
   void low_new_thread (struct lwp_info *lp) override
   { aarch64_linux_new_thread (lp); }
@@ -93,6 +87,7 @@ class aarch64_linux_nat_target final
   { aarch64_linux_prepare_to_resume (lp); }
 
   void low_new_fork (struct lwp_info *parent, pid_t child_pid) override;
+  void low_init_process (ptid_t pid) override;
   void low_forget_process (pid_t pid) override;
 
   /* Add our siginfo layout converter.  */
@@ -844,29 +839,16 @@ ps_get_thread_area (struct ps_prochandle *ph,
 }
 \f
 
-/* Implement the virtual inf_ptrace_target::post_startup_inferior method.  */
-
-void
-aarch64_linux_nat_target::post_startup_inferior (ptid_t ptid)
-{
-  low_forget_process (ptid.pid ());
-  aarch64_linux_get_debug_reg_capacity (ptid.pid ());
-  linux_nat_target::post_startup_inferior (ptid);
-}
-
-/* Implement the "post_attach" target_ops method.  */
+/* Implement the "low_init_process" target_ops method.  */
 
 void
-aarch64_linux_nat_target::post_attach (int pid)
+aarch64_linux_nat_target::low_init_process (int pid)
 {
   low_forget_process (pid);
-  /* Set the hardware debug register capacity.  If
-     aarch64_linux_get_debug_reg_capacity is not called
-     (as it is in aarch64_linux_child_post_startup_inferior) then
-     software watchpoints will be used instead of hardware
-     watchpoints when attaching to a target.  */
+  /* Set the hardware debug register capacity.  If this is not called
+     then software watchpoints will be used instead of hardware
+     watchpoints.  */
   aarch64_linux_get_debug_reg_capacity (pid);
-  linux_nat_target::post_attach (pid);
 }
 
 /* Implement the "read_description" target_ops method.  */
diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index 50c24ecfcd2..531d555474a 100644
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -103,6 +103,7 @@ class arm_linux_nat_target final : public linux_nat_target
 
   /* Handle process creation and exit.  */
   void low_new_fork (struct lwp_info *parent, pid_t child_pid) override;
+  void low_init_process (pid_t pid) override;
   void low_forget_process (pid_t pid) override;
 };
 
@@ -805,6 +806,12 @@ arm_linux_process_info_get (pid_t pid)
   return proc;
 }
 
+void
+arm_linux_nat_target::low_init_process (pid_t pid)
+{
+  arm_linux_get_hwbp_cap ();
+}
+
 /* Called whenever GDB is no longer debugging process PID.  It deletes
    data structures that keep track of debug register state.  */
 
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index c0fe08a2a8b..3f252370c7b 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -454,6 +454,12 @@ linux_init_ptrace_procfs (pid_t pid, int attached)
   linux_ptrace_init_warnings ();
   linux_proc_init_warnings ();
   proc_mem_file_is_writable ();
+
+  /* Let the arch-specific native code do any needed initialization.
+     Some architectures need to call ptrace to check for hardware
+     watchpoints support, etc.  Call it now, when we know the tracee
+     is ptrace-stopped.  */
+  linux_target->low_init_process (pid);
 }
 
 linux_nat_target::~linux_nat_target ()
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index f30a5f90e2a..ee8603743f6 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -164,6 +164,12 @@ class linux_nat_target : public inf_ptrace_target
   virtual void low_new_clone (struct lwp_info *parent, pid_t child_lwp)
   {}
 
+  /* The method to call, if any, when we have a new (from run/attach,
+     not fork) process to debug.  The process is ptrace-stopped when
+     this is called.  */
+  virtual void low_init_process (pid_t pid)
+  {}
+
   /* The method to call, if any, when a process is no longer
      attached.  */
   virtual void low_forget_process (pid_t pid)
diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index c73c7c90b4c..7ff12e19db3 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -370,19 +370,17 @@ class ppc_linux_dreg_interface
     return m_interface.has_value ();
   }
 
-  /* Detect the available interface, if any, if it hasn't been detected
-     before, using PTID for the necessary ptrace calls.  */
+  /* Detect the available interface, if any, if it hasn't been
+     detected before, using PID for the necessary ptrace calls.  */
 
-  void detect (const ptid_t &ptid)
+  void detect (const pid_t pid)
   {
     if (m_interface.has_value ())
       return;
 
-    gdb_assert (ptid.lwp_p ());
-
     bool no_features = false;
 
-    if (ptrace (PPC_PTRACE_GETHWDBGINFO, ptid.lwp (), 0, &m_hwdebug_info)
+    if (ptrace (PPC_PTRACE_GETHWDBGINFO, pid, 0, &m_hwdebug_info)
 	>= 0)
       {
 	/* If there are no advertised features, we don't use the
@@ -429,7 +427,7 @@ class ppc_linux_dreg_interface
       {
 	unsigned long wp;
 
-	if (ptrace (PTRACE_GET_DEBUGREG, ptid.lwp (), 0, &wp) >= 0)
+	if (ptrace (PTRACE_GET_DEBUGREG, pid, 0, &wp) >= 0)
 	  {
 	    m_interface.emplace (DEBUGREG);
 	    return;
@@ -2046,8 +2044,6 @@ ppc_linux_nat_target::can_use_hw_breakpoint (enum bptype type, int cnt,
 {
   int total_hw_wp, total_hw_bp;
 
-  m_dreg_interface.detect (inferior_ptid);
-
   if (m_dreg_interface.unavailable_p ())
     return 0;
 
@@ -2101,8 +2097,6 @@ ppc_linux_nat_target::region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
   if (len <= 0)
     return 0;
 
-  m_dreg_interface.detect (inferior_ptid);
-
   if (m_dreg_interface.unavailable_p ())
     return 0;
 
@@ -2180,8 +2174,6 @@ ppc_linux_nat_target::hwdebug_point_cmp (const struct ppc_hw_breakpoint &a,
 int
 ppc_linux_nat_target::ranged_break_num_registers ()
 {
-  m_dreg_interface.detect (inferior_ptid);
-
   return ((m_dreg_interface.hwdebug_p ()
 	   && (m_dreg_interface.hwdebug_info ().features
 	       & PPC_DEBUG_FEATURE_INSN_BP_RANGE))?
@@ -2199,8 +2191,6 @@ ppc_linux_nat_target::insert_hw_breakpoint (struct gdbarch *gdbarch,
 {
   struct ppc_hw_breakpoint p;
 
-  m_dreg_interface.detect (inferior_ptid);
-
   if (!m_dreg_interface.hwdebug_p ())
     return -1;
 
@@ -2240,8 +2230,6 @@ ppc_linux_nat_target::remove_hw_breakpoint (struct gdbarch *gdbarch,
 {
   struct ppc_hw_breakpoint p;
 
-  m_dreg_interface.detect (inferior_ptid);
-
   if (!m_dreg_interface.hwdebug_p ())
     return -1;
 
@@ -2346,8 +2334,6 @@ ppc_linux_nat_target::remove_mask_watchpoint (CORE_ADDR addr, CORE_ADDR mask,
 bool
 ppc_linux_nat_target::can_use_watchpoint_cond_accel (void)
 {
-  m_dreg_interface.detect (inferior_ptid);
-
   if (!m_dreg_interface.hwdebug_p ())
     return false;
 
@@ -2546,8 +2532,6 @@ ppc_linux_nat_target::can_accel_watchpoint_condition (CORE_ADDR addr,
 {
   CORE_ADDR data_value;
 
-  m_dreg_interface.detect (inferior_ptid);
-
   return (m_dreg_interface.hwdebug_p ()
 	  && (m_dreg_interface.hwdebug_info ().num_condition_regs > 0)
 	  && check_condition (addr, cond, &data_value, &len));
@@ -2618,8 +2602,6 @@ ppc_linux_nat_target::insert_watchpoint (CORE_ADDR addr, int len,
 					 enum target_hw_bp_type type,
 					 struct expression *cond)
 {
-  m_dreg_interface.detect (inferior_ptid);
-
   if (m_dreg_interface.unavailable_p ())
     return -1;
 
@@ -2705,6 +2687,12 @@ ppc_linux_nat_target::remove_watchpoint (CORE_ADDR addr, int len,
   return 0;
 }
 
+void
+ppc_linux_nat_target::low_init_process (pid_t pid)
+{
+  m_dreg_interface.detect (pid);
+}
+
 /* Clean up the per-process info associated with PID.  When using the
    HWDEBUG interface, we also erase the per-thread state of installed
    debug registers for all the threads that belong to the group of PID.
@@ -2716,8 +2704,7 @@ ppc_linux_nat_target::remove_watchpoint (CORE_ADDR addr, int len,
 void
 ppc_linux_nat_target::low_forget_process (pid_t pid)
 {
-  if ((!m_dreg_interface.detected_p ())
-      || (m_dreg_interface.unavailable_p ()))
+  if (m_dreg_interface.unavailable_p ())
     return;
 
   ptid_t pid_ptid (pid, 0, 0);
@@ -2761,8 +2748,7 @@ void
 ppc_linux_nat_target::low_new_fork (struct lwp_info *parent,
 				    pid_t child_pid)
 {
-  if ((!m_dreg_interface.detected_p ())
-      || (m_dreg_interface.unavailable_p ()))
+  if (m_dreg_interface.unavailable_p ())
     return;
 
   auto process_it = m_process_info.find (parent->ptid.pid ());
@@ -2788,8 +2774,7 @@ void
 ppc_linux_nat_target::low_new_clone (struct lwp_info *parent,
 				     pid_t child_lwp)
 {
-  if ((!m_dreg_interface.detected_p ())
-      || (m_dreg_interface.unavailable_p ()))
+  if (m_dreg_interface.unavailable_p ())
     return;
 
   if (m_dreg_interface.hwdebug_p ())
@@ -2821,8 +2806,7 @@ ppc_linux_nat_target::low_delete_thread (struct arch_lwp_info
 {
   if (lp_arch_info != NULL)
     {
-      if (m_dreg_interface.detected_p ()
-	  && m_dreg_interface.hwdebug_p ())
+      if (m_dreg_interface.hwdebug_p ())
 	m_installed_hw_bps.erase (lp_arch_info->lwp_ptid);
 
       xfree (lp_arch_info);
@@ -2835,8 +2819,7 @@ ppc_linux_nat_target::low_delete_thread (struct arch_lwp_info
 void
 ppc_linux_nat_target::low_prepare_to_resume (struct lwp_info *lp)
 {
-  if ((!m_dreg_interface.detected_p ())
-      || (m_dreg_interface.unavailable_p ()))
+  if (m_dreg_interface.unavailable_p ())
     return;
 
   /* We have to re-install or clear the debug registers if we set the
@@ -3030,8 +3013,6 @@ int
 ppc_linux_nat_target::masked_watch_num_registers (CORE_ADDR addr,
 						  CORE_ADDR mask)
 {
-  m_dreg_interface.detect (inferior_ptid);
-
   if (!m_dreg_interface.hwdebug_p ()
       || (m_dreg_interface.hwdebug_info ().features
 	  & PPC_DEBUG_FEATURE_DATA_BP_MASK) == 0)
@@ -3069,8 +3050,7 @@ ppc_linux_nat_target::copy_thread_dreg_state (const ptid_t &parent_ptid,
 void
 ppc_linux_nat_target::mark_thread_stale (struct lwp_info *lp)
 {
-  if ((!m_dreg_interface.detected_p ())
-      || (m_dreg_interface.unavailable_p ()))
+  if (m_dreg_interface.unavailable_p ())
     return;
 
   arch_lwp_info *lp_arch_info = get_arch_lwp_info (lp);

base-commit: 5527eae3f1f28a628f6c73c7b5743cebf7df8a13
-- 
2.45.2

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

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 4406 bytes --]

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


[-- Attachment #2: 0001-gdb-tdep-Fix-gdb.base-watchpoint-running-on-arm-ppc6.patch --]
[-- Type: text/x-patch, Size: 5621 bytes --]

From 51be092155cd24ce1fb648ebaf7f5bd4fbd99a6f Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 14 Jun 2024 17:49:16 +0100
Subject: [PATCH] [gdb/tdep] Fix gdb.base/watchpoint-running on {arm,
 ppc64le}-linux

---
 gdb/aarch64-linux-nat.c | 30 ++++++------------------------
 gdb/arm-linux-nat.c     |  7 +++++++
 gdb/linux-nat.c         |  6 ++++++
 gdb/linux-nat.h         |  6 ++++++
 gdb/ppc-linux-nat.c     |  8 ++++++++
 5 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 4b2a0ba9f7b..8bab594f1b3 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -78,12 +78,6 @@ class aarch64_linux_nat_target final
 
   int can_do_single_step () override;
 
-  /* Override the GNU/Linux inferior startup hook.  */
-  void post_startup_inferior (ptid_t) override;
-
-  /* Override the GNU/Linux post attach hook.  */
-  void post_attach (int pid) override;
-
   /* These three defer to common nat/ code.  */
   void low_new_thread (struct lwp_info *lp) override
   { aarch64_linux_new_thread (lp); }
@@ -93,6 +87,7 @@ class aarch64_linux_nat_target final
   { aarch64_linux_prepare_to_resume (lp); }
 
   void low_new_fork (struct lwp_info *parent, pid_t child_pid) override;
+  void low_init_process (pid_t pid) override;
   void low_forget_process (pid_t pid) override;
 
   /* Add our siginfo layout converter.  */
@@ -844,29 +839,16 @@ ps_get_thread_area (struct ps_prochandle *ph,
 }
 \f
 
-/* Implement the virtual inf_ptrace_target::post_startup_inferior method.  */
-
-void
-aarch64_linux_nat_target::post_startup_inferior (ptid_t ptid)
-{
-  low_forget_process (ptid.pid ());
-  aarch64_linux_get_debug_reg_capacity (ptid.pid ());
-  linux_nat_target::post_startup_inferior (ptid);
-}
-
-/* Implement the "post_attach" target_ops method.  */
+/* Implement the "low_init_process" target_ops method.  */
 
 void
-aarch64_linux_nat_target::post_attach (int pid)
+aarch64_linux_nat_target::low_init_process (pid_t pid)
 {
   low_forget_process (pid);
-  /* Set the hardware debug register capacity.  If
-     aarch64_linux_get_debug_reg_capacity is not called
-     (as it is in aarch64_linux_child_post_startup_inferior) then
-     software watchpoints will be used instead of hardware
-     watchpoints when attaching to a target.  */
+  /* Set the hardware debug register capacity.  If this is not called
+     then software watchpoints will be used instead of hardware
+     watchpoints.  */
   aarch64_linux_get_debug_reg_capacity (pid);
-  linux_nat_target::post_attach (pid);
 }
 
 /* Implement the "read_description" target_ops method.  */
diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index 50c24ecfcd2..531d555474a 100644
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -103,6 +103,7 @@ class arm_linux_nat_target final : public linux_nat_target
 
   /* Handle process creation and exit.  */
   void low_new_fork (struct lwp_info *parent, pid_t child_pid) override;
+  void low_init_process (pid_t pid) override;
   void low_forget_process (pid_t pid) override;
 };
 
@@ -805,6 +806,12 @@ arm_linux_process_info_get (pid_t pid)
   return proc;
 }
 
+void
+arm_linux_nat_target::low_init_process (pid_t pid)
+{
+  arm_linux_get_hwbp_cap ();
+}
+
 /* Called whenever GDB is no longer debugging process PID.  It deletes
    data structures that keep track of debug register state.  */
 
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index c0fe08a2a8b..3f252370c7b 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -454,6 +454,12 @@ linux_init_ptrace_procfs (pid_t pid, int attached)
   linux_ptrace_init_warnings ();
   linux_proc_init_warnings ();
   proc_mem_file_is_writable ();
+
+  /* Let the arch-specific native code do any needed initialization.
+     Some architectures need to call ptrace to check for hardware
+     watchpoints support, etc.  Call it now, when we know the tracee
+     is ptrace-stopped.  */
+  linux_target->low_init_process (pid);
 }
 
 linux_nat_target::~linux_nat_target ()
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index f30a5f90e2a..ee8603743f6 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -164,6 +164,12 @@ class linux_nat_target : public inf_ptrace_target
   virtual void low_new_clone (struct lwp_info *parent, pid_t child_lwp)
   {}
 
+  /* The method to call, if any, when we have a new (from run/attach,
+     not fork) process to debug.  The process is ptrace-stopped when
+     this is called.  */
+  virtual void low_init_process (pid_t pid)
+  {}
+
   /* The method to call, if any, when a process is no longer
      attached.  */
   virtual void low_forget_process (pid_t pid)
diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index c73c7c90b4c..ebd222b896e 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -545,6 +545,8 @@ struct ppc_linux_nat_target final : public linux_nat_target
 
   void low_new_clone (struct lwp_info *, pid_t) override;
 
+  void low_init_process (pid_t pid) override;
+
   void low_forget_process (pid_t pid) override;
 
   void low_prepare_to_resume (struct lwp_info *) override;
@@ -2705,6 +2707,12 @@ ppc_linux_nat_target::remove_watchpoint (CORE_ADDR addr, int len,
   return 0;
 }
 
+void
+ppc_linux_nat_target::low_init_process (pid_t pid)
+{
+  m_dreg_interface.detect (ptid_t (pid, pid));
+}
+
 /* Clean up the per-process info associated with PID.  When using the
    HWDEBUG interface, we also erase the per-thread state of installed
    debug registers for all the threads that belong to the group of PID.

base-commit: c3d23f753daaac52ee6c788090845ff0f359cf27
-- 
2.35.3

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

[Tom de Vries]

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

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

[Pedro Alves]

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.

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

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

Pedro Alves

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

[Tom de Vries]

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

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

[Pedro Alves]

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!

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

[Tom de Vries]

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!