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

[Pedro Alves <pedro@palves.net>]

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