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

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

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