[RFA] Make ppc-linux-nat insert/remove breakpoint and watchpoint functions return -1.

[RFA] Make ppc-linux-nat insert/remove breakpoint and watchpoint functions return -1.

[Thiago Jung Bauermann]

Hi,

target_{insert,remove}_{hw_breakpoint,watchpoint,mask_watchpoint} are
supposed to return 0, 1 and -1 for success, unsupported and error,
respectively. This patch makes ppc-linux-nat.c comply.

No regressions on ppc-linux and ppc64-linux. Ok?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2011-07-05  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	Make insert/remove breakpoint and watchpoint functions return -1.
	* ppc-linux-nat.c (ppc_linux_remove_watchpoint): Add prototype.
	(booke_insert_point): Return 0 on success and -1 on error.
	(booke_remove_point): Likewise.
	(ppc_linux_insert_hw_breakpoint): Return 0 on success, -1 on error
	and 1 if hardware breakpoints are not supported.  Also, remove the
	breakpoint from all threads if it couldn't be installed in one
	of them.
	(ppc_linux_remove_hw_breakpoint): Likewise.
	(ppc_linux_insert_mask_watchpoint): Likewise.  Also, remove the
	watchpoint from all threads if it couldn't be installed in one
	of them.
	(ppc_linux_remove_mask_watchpoint): Likewise.
	(ppc_linux_insert_watchpoint): Likewise.  Also, remove the
	watchpoint from all threads if it couldn't be installed in one
	of them.
	(ppc_linux_remove_watchpoint): Likewise.

diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 275de78..410755f 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -336,6 +336,9 @@ PT_FPR0 + 56, PT_FPR0 + 58, PT_FPR0 + 60, PT_FPR0 + 62,
 PT_NIP, PT_MSR, PT_CCR, PT_LNK, PT_CTR, PT_XER, PT_MQ */
 /* *INDENT_ON * */
 
+static int ppc_linux_remove_watchpoint (CORE_ADDR addr, int len, int rw,
+					struct expression *cond);
+
 static int
 ppc_register_u_addr (struct gdbarch *gdbarch, int regno)
 {
@@ -1566,8 +1569,10 @@ booke_find_thread_points_by_tid (int tid, int alloc_new)
 
 /* This function is a generic wrapper that is responsible for inserting a
    *point (i.e., calling `ptrace' in order to issue the request to the
-   kernel) and registering it internally in GDB.  */
-static void
+   kernel) and registering it internally in GDB.  Returns 0 for success
+   or -1 for failure.  */
+
+static int
 booke_insert_point (struct ppc_hw_breakpoint *b, int tid)
 {
   int i;
@@ -1580,10 +1585,13 @@ booke_insert_point (struct ppc_hw_breakpoint *b, int tid)
 
   memcpy (p, b, sizeof (struct ppc_hw_breakpoint));
 
-  errno = 0;
   slot = ptrace (PPC_PTRACE_SETHWDEBUG, tid, 0, p);
   if (slot < 0)
-    perror_with_name (_("Unexpected error setting breakpoint or watchpoint"));
+    {
+      do_cleanups (c);
+
+      return -1;
+    }
 
   /* Everything went fine, so we have to register this *point.  */
   t = booke_find_thread_points_by_tid (tid, 1);
@@ -1602,15 +1610,19 @@ booke_insert_point (struct ppc_hw_breakpoint *b, int tid)
   gdb_assert (i != max_slots_number);
 
   discard_cleanups (c);
+
+  return 0;
 }
 
 /* This function is a generic wrapper that is responsible for removing a
    *point (i.e., calling `ptrace' in order to issue the request to the
-   kernel), and unregistering it internally at GDB.  */
-static void
+   kernel), and unregistering it internally at GDB.  Returns 0 for
+   success or -1 for failure.  */
+
+static int
 booke_remove_point (struct ppc_hw_breakpoint *b, int tid)
 {
-  int i;
+  int i, ret = 0;
   struct hw_break_tuple *hw_breaks;
   struct thread_points *t;
 
@@ -1622,7 +1634,9 @@ booke_remove_point (struct ppc_hw_breakpoint *b, int tid)
     if (hw_breaks[i].hw_break && booke_cmp_hw_point (hw_breaks[i].hw_break, b))
       break;
 
-  gdb_assert (i != max_slots_number);
+  /* There's no such breakpoint/watchpoint for this thread.  */
+  if (i == max_slots_number)
+    return -1;
 
   /* We have to ignore ENOENT errors because the kernel implements hardware
      breakpoints/watchpoints as "one-shot", that is, they are automatically
@@ -1630,11 +1644,12 @@ booke_remove_point (struct ppc_hw_breakpoint *b, int tid)
   errno = 0;
   if (ptrace (PPC_PTRACE_DELHWDEBUG, tid, 0, hw_breaks[i].slot) < 0)
     if (errno != ENOENT)
-      perror_with_name (_("Unexpected error deleting "
-			  "breakpoint or watchpoint"));
+      ret = -1;
 
   xfree (hw_breaks[i].hw_break);
   hw_breaks[i].hw_break = NULL;
+
+  return ret;
 }
 
 /* Return the number of registers needed for a ranged breakpoint.  */
@@ -1652,14 +1667,15 @@ ppc_linux_ranged_break_num_registers (struct target_ops *target)
 
 static int
 ppc_linux_insert_hw_breakpoint (struct gdbarch *gdbarch,
-				  struct bp_target_info *bp_tgt)
+				struct bp_target_info *bp_tgt)
 {
+  int ret;
   ptid_t ptid;
   struct lwp_info *lp;
   struct ppc_hw_breakpoint p;
 
   if (!have_ptrace_booke_interface ())
-    return -1;
+    return 1;
 
   p.version = PPC_DEBUG_CURRENT_VERSION;
   p.trigger_type = PPC_BREAKPOINT_TRIGGER_EXECUTE;
@@ -1682,21 +1698,36 @@ ppc_linux_insert_hw_breakpoint (struct gdbarch *gdbarch,
     }
 
   ALL_LWPS (lp, ptid)
-    booke_insert_point (&p, TIDGET (ptid));
+    {
+      ret = booke_insert_point (&p, TIDGET (ptid));
+      if (ret)
+	break;
+    }
 
-  return 0;
+  /* If an error occurred, remove the breakpoint from all threads, to
+     keep consistency (all threads must have the same breakpoints and
+     watchpoints intalled.)  */
+  if (ret)
+    ALL_LWPS (lp, ptid)
+      booke_remove_point (&p, TIDGET (ptid));
+
+  return ret;
 }
 
+/* Remove the hardware breakpoint described by BP_TGT.  Returns 0 for
+   success, 1 if hardware breakpoints are not supported or -1 for failure.  */
+
 static int
 ppc_linux_remove_hw_breakpoint (struct gdbarch *gdbarch,
-				  struct bp_target_info *bp_tgt)
+				struct bp_target_info *bp_tgt)
 {
+  int ret = 0;
   ptid_t ptid;
   struct lwp_info *lp;
   struct ppc_hw_breakpoint p;
 
   if (!have_ptrace_booke_interface ())
-    return -1;
+    return 1;
 
   p.version = PPC_DEBUG_CURRENT_VERSION;
   p.trigger_type = PPC_BREAKPOINT_TRIGGER_EXECUTE;
@@ -1719,9 +1750,9 @@ ppc_linux_remove_hw_breakpoint (struct gdbarch *gdbarch,
     }
 
   ALL_LWPS (lp, ptid)
-    booke_remove_point (&p, TIDGET (ptid));
+    ret |= booke_remove_point (&p, TIDGET (ptid));
 
-  return 0;
+  return ret;
 }
 
 static int
@@ -1741,13 +1772,14 @@ get_trigger_type (int rw)
 
 /* Insert a new masked watchpoint at ADDR using the mask MASK.
    RW may be hw_read for a read watchpoint, hw_write for a write watchpoint
-   or hw_access for an access watchpoint.  Returns 0 on success and throws
-   an error on failure.  */
+   or hw_access for an access watchpoint.  Returns 0 for success, 1 if
+   masked watchpoints are not supported or -1 for failure.  */
 
 static int
 ppc_linux_insert_mask_watchpoint (struct target_ops *ops, CORE_ADDR addr,
 				  CORE_ADDR mask, int rw)
 {
+  int ret;
   ptid_t ptid;
   struct lwp_info *lp;
   struct ppc_hw_breakpoint p;
@@ -1763,20 +1795,32 @@ ppc_linux_insert_mask_watchpoint (struct target_ops *ops, CORE_ADDR addr,
   p.condition_value = 0;
 
   ALL_LWPS (lp, ptid)
-    booke_insert_point (&p, TIDGET (ptid));
+    {
+      ret = booke_insert_point (&p, TIDGET (ptid));
+      if (ret)
+	break;
+    }
 
-  return 0;
+  /* If an error occurred, remove the breakpoint from all threads, to
+     keep consistency (all threads must have the same breakpoints and
+     watchpoints intalled.)  */
+  if (ret)
+    ALL_LWPS (lp, ptid)
+      booke_remove_point (&p, TIDGET (ptid));
+
+  return ret;
 }
 
 /* Remove a masked watchpoint at ADDR with the mask MASK.
    RW may be hw_read for a read watchpoint, hw_write for a write watchpoint
-   or hw_access for an access watchpoint.  Returns 0 on success and throws
-   an error on failure.  */
+   or hw_access for an access watchpoint.  Returns 0 for success, 1 if
+   masked watchpoints are not supported or -1 for failure.  */
 
 static int
 ppc_linux_remove_mask_watchpoint (struct target_ops *ops, CORE_ADDR addr,
 				  CORE_ADDR mask, int rw)
 {
+  int ret = 0;
   ptid_t ptid;
   struct lwp_info *lp;
   struct ppc_hw_breakpoint p;
@@ -1792,9 +1836,9 @@ ppc_linux_remove_mask_watchpoint (struct target_ops *ops, CORE_ADDR addr,
   p.condition_value = 0;
 
   ALL_LWPS (lp, ptid)
-    booke_remove_point (&p, TIDGET (ptid));
+    ret |= booke_remove_point (&p, TIDGET (ptid));
 
-  return 0;
+  return ret;
 }
 
 /* Check whether we have at least one free DVC register.  */
@@ -2054,13 +2098,18 @@ create_watchpoint_request (struct ppc_hw_breakpoint *p, CORE_ADDR addr,
   p->addr = (uint64_t) addr;
 }
 
+/* Insert a hardware watchpoint at address ADDR, spanning LEN bytes.
+   RW may be hw_read for a read watchpoint, hw_write for a write watchpoint
+   or hw_access for an access watchpoint.  Returns 0 for success, 1 if
+   hardware watchpoints are not supported or -1 for failure.  */
+
 static int
 ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
 			     struct expression *cond)
 {
   struct lwp_info *lp;
   ptid_t ptid;
-  int ret = -1;
+  int ret;
 
   if (have_ptrace_booke_interface ())
     {
@@ -2069,9 +2118,18 @@ ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
       create_watchpoint_request (&p, addr, len, rw, cond, 1);
 
       ALL_LWPS (lp, ptid)
-	booke_insert_point (&p, TIDGET (ptid));
+	{
+	  ret = booke_insert_point (&p, TIDGET (ptid));
+	  if (ret)
+	    break;
+	}
 
-      ret = 0;
+      /* If an error occurred, remove the breakpoint from all threads, to
+	 keep consistency (all threads must have the same breakpoints and
+	 watchpoints intalled.)  */
+      if (ret)
+	ALL_LWPS (lp, ptid)
+	  booke_remove_point (&p, TIDGET (ptid));
     }
   else
     {
@@ -2111,25 +2169,38 @@ ppc_linux_insert_watchpoint (CORE_ADDR addr, int len, int rw,
 	}
 
       saved_dabr_value = dabr_value;
+      ret = 0;
 
       ALL_LWPS (lp, ptid)
 	if (ptrace (PTRACE_SET_DEBUGREG, TIDGET (ptid), 0,
 		    saved_dabr_value) < 0)
-	  return -1;
+	  {
+	    ret = -1;
+	    break;
+	  }
 
-      ret = 0;
+      /* If an error occurred, remove the breakpoint from all threads, to
+	 keep consistency (all threads must have the same breakpoints and
+	 watchpoints intalled.)  */
+      if (ret)
+	ppc_linux_remove_watchpoint (addr, len, rw, cond);
     }
 
   return ret;
 }
 
+/* Remove a hardware watchpoint at address ADDR, spanning LEN bytes.
+   RW may be hw_read for a read watchpoint, hw_write for a write watchpoint
+   or hw_access for an access watchpoint.  Returns 0 for success, 1 if
+   hardware watchpoints are not supported or -1 for failure.  */
+
 static int
 ppc_linux_remove_watchpoint (CORE_ADDR addr, int len, int rw,
 			     struct expression *cond)
 {
   struct lwp_info *lp;
   ptid_t ptid;
-  int ret = -1;
+  int ret = 0;
 
   if (have_ptrace_booke_interface ())
     {
@@ -2138,19 +2209,16 @@ ppc_linux_remove_watchpoint (CORE_ADDR addr, int len, int rw,
       create_watchpoint_request (&p, addr, len, rw, cond, 0);
 
       ALL_LWPS (lp, ptid)
-	booke_remove_point (&p, TIDGET (ptid));
-
-      ret = 0;
+	ret |= booke_remove_point (&p, TIDGET (ptid));
     }
   else
     {
       saved_dabr_value = 0;
+
       ALL_LWPS (lp, ptid)
 	if (ptrace (PTRACE_SET_DEBUGREG, TIDGET (ptid), 0,
 		    saved_dabr_value) < 0)
-	  return -1;
-
-      ret = 0;
+	  ret = -1;
     }
 
   return ret;

Re: [RFA] Make ppc-linux-nat insert/remove breakpoint and watchpoint functions return -1.

[Tom Tromey]

>>>>> "Thiago" == Thiago Jung Bauermann <bauerman@br.ibm.com> writes:

Thiago> target_{insert,remove}_{hw_breakpoint,watchpoint,mask_watchpoint} are
Thiago> supposed to return 0, 1 and -1 for success, unsupported and error,
Thiago> respectively.

I looked around for anything saying this, and couldn't find it.
Well -- I found it for target_insert_mask_watchpoint, but not other
cases.

And, at least in breakpoint.c:insert_bp_location, it seems to me that
the return value is expected to be an errno in one case -- search for
safe_strerror.

Maybe this code is in error?  I really couldn't say; but it seems
inconsistent at best.  Perhaps only parts of your patch could go in
as-is, what do you think?

Tom

Re: [RFA] Make ppc-linux-nat insert/remove breakpoint and watchpoint functions return -1.

[Joel Brobecker]

> >>>>> "Thiago" == Thiago Jung Bauermann <bauerman@br.ibm.com> writes:
> 
> Thiago> target_{insert,remove}_{hw_breakpoint,watchpoint,mask_watchpoint} are
> Thiago> supposed to return 0, 1 and -1 for success, unsupported and error,
> Thiago> respectively.
> 
> I looked around for anything saying this, and couldn't find it.
> Well -- I found it for target_insert_mask_watchpoint, but not other
> cases.
> 
> And, at least in breakpoint.c:insert_bp_location, it seems to me that
> the return value is expected to be an errno in one case -- search for
> safe_strerror.
> 
> Maybe this code is in error?  I really couldn't say; but it seems
> inconsistent at best.  Perhaps only parts of your patch could go in
> as-is, what do you think?

Looking at target.h:

| /* Insert a breakpoint at address BP_TGT->placed_address in the target
|    machine.  Result is 0 for success, or an errno value.  */
| 
| extern int target_insert_breakpoint (struct gdbarch *gdbarch,
|                                      struct bp_target_info *bp_tgt);

It's a little bit confusing because some methods are implemented
inside the target_ops vector, while some others are documented
via the target_... function. That being said, it's ultra important
IMO in that case that we add the documentation wherever missing,
because these are used by all ports. And I remember scratching
my head quite a bit a while ago when I was working on watchpoint
support for VxWorks.

In any case, all this rambling to say that breakpoint.c is correct
in this case.

-- 
Joel

Re: [RFA] Make ppc-linux-nat insert/remove breakpoint and watchpoint functions return -1.

[Joel Brobecker]

> Looking at target.h:
> 
> | /* Insert a breakpoint at address BP_TGT->placed_address in the target
> |    machine.  Result is 0 for success, or an errno value.  */
> | 
> | extern int target_insert_breakpoint (struct gdbarch *gdbarch,
> |                                      struct bp_target_info *bp_tgt);
> 

Sorry, scrap that part - I was looking at the wrong method
(I should have been looking at the `hw' one....).

> That being said, it's ultra important IMO in that case that we add the
> documentation wherever missing, because these are used by all ports.

Makes that comment all the more relevant :).

So I guess we'll have to infer the expected behavior from the core
code that calls it. I am hoping that it's the same as with the non
hw versions! :)

-- 
Joel

Re: [RFA] Make ppc-linux-nat insert/remove breakpoint and watchpoint functions return -1.

[Thiago Jung Bauermann]

On Wed, 2011-07-06 at 09:35 -0700, Joel Brobecker wrote:
> > Looking at target.h:
> > 
> > | /* Insert a breakpoint at address BP_TGT->placed_address in the target
> > |    machine.  Result is 0 for success, or an errno value.  */
> > | 
> > | extern int target_insert_breakpoint (struct gdbarch *gdbarch,
> > |                                      struct bp_target_info *bp_tgt);
> > 
> 
> Sorry, scrap that part - I was looking at the wrong method
> (I should have been looking at the `hw' one....).
> 
> > That being said, it's ultra important IMO in that case that we add the
> > documentation wherever missing, because these are used by all ports.
> 
> Makes that comment all the more relevant :).
> 
> So I guess we'll have to infer the expected behavior from the core
> code that calls it. I am hoping that it's the same as with the non
> hw versions! :)

My ppc476 patches touched a lot of this code and I made everything that
uses the breakpoint_ops methods follow that convention. Things were
indeed confusing and even contradictory before my patches, and I thought
I fixed all that. It seems I missed some parts then. I'll have a look
and post fixes. This patch can wait for now. Thanks for bringing this
up.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center