Re: [PATCH 2/5] Make breakpoint and breakpoint_len local variables in GDBServer.

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

On 09/28/2015 05:54 AM, Yao Qi wrote:
> Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
>
> > @@ -774,8 +800,9 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) (CORE_ADDR))
> >   {
> >     int err_ignored;
> >
> > +  /* default breakpoint_len will be initialized downstream.  */
> >     return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
> > -			 where, breakpoint_len, handler,
> > +			 where, 0, handler,
> >   			 &err_ignored);
> >   }
>
> Why do you set breakpoint length to zero?  I know you'll set the length
> downstream properly, but we should compute the breakpoint length here.
That's what I thought too initially but somehow that intention got lost 
a long the way.

> After thinking about it a bit more, I think this reveals some design
> issues in GDBserver brekapoint, nowadays, GDBserver inserts its own
> breakpoints and breakpoints requested by GDB.  After this patch series,
> GDBserver should be able to:
>
>    1) choose the right breakpoint instruction for its own breakpoints,
>    according to the breakpoint address, status register flag, etc,
>    through API set_breakpoint_at,
>    2) choose the right breakpoint instruction for breakpoints requested
>    by GDB, according to the information in Z packets, through API
>    set_gdb_breakpoint

Indeed, you need to consider also the case of tracepoints as they will 
set a third case where you set a GDBServer breakpoint with a z0 like 
kind field for tracepoints. But it could be done with another call like 
set_breakpoint_at_with_length or with_kind.

Also the HW breakpoint case is already is using where and size as pcfull 
and kind in insert_point so we need to keep the meaning of those 
arguments from set_breakpoint_at and gdb_set_breakpoint.

> there should be two paths for them, and each path needs different target
> hook to choose breakpoint instructions.  breakpoint_from_pc is needed for
> #1, and breakpoint_from_length is needed for #2.  In your current patch
> set (in patch 4/5), two things are mixed together, which doesn't look
> good to me.

I do agree that seems weird.

> breakpoint is like
>
>    set_breakpoint_at
>    set_gdb_breakpoint_1
>       |
>       `--> set_breakpoint
>               |
>               `-->set_raw_breakpoint_at
>                      |
>                      `--> the_target->insert_point
>
> we are handling the breakpoint length at the lowest level, in
> insert_memory_breakpoint, and we use breakpoint_from_pc and
> breakpoint_from_length together there, which looks unclean.  Ideally, we
> can move these code handling breakpoint length (breakpoint_from_pc and
> breakpoint_from_length) to upper levels, like this,
>
>    set_breakpoint_at (call breakpoint_from_pc)
>    set_gdb_breakpoint_1 (call breakpoint_from_length)
>       |
>       `--> set_breakpoint
>               |
>               `-->set_raw_breakpoint_at
>                      |
>                      `--> the_target->insert_point
>
> all needed information is saved in struct breakpoint or struct
> raw_breakpoint, and function set_breakpoint and it callees can use
> breakpoint or raw_breakpoint directly.  It'll be cleaner in this way,
> let me know what do you think?
Indeed this is a very good point, seems obvious now.

I've redone the whole patch sets up to tracepoints with this with no 
issues..

So look forward to this change in v2 ..

Here's what this patch will look like with the change :

Does that look better ?

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 200ea94..cc42f36 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7067,16 +7067,10 @@ void
 initialize_low (void)
 {
   struct sigaction sigchld_action;
-  int breakpoint_len = 0;
-  const unsigned char *breakpoint = NULL;

   memset (&sigchld_action, 0, sizeof (sigchld_action));
   set_target_ops (&linux_target_ops);

-  breakpoint = the_target->breakpoint_from_pc (NULL, &breakpoint_len);
-
-  set_breakpoint_data (breakpoint,
-		       breakpoint_len);
   linux_init_signals ();
   linux_ptrace_init_warnings ();

diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 9356741..5e5128e 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -21,8 +21,6 @@
 #include "server.h"
 #include "regcache.h"
 #include "ax.h"
-const unsigned char *breakpoint_data;
-int breakpoint_len;

 #define MAX_BREAKPOINT_LEN 8

@@ -100,6 +98,16 @@ struct raw_breakpoint
      breakpoint for a given PC.  */
   CORE_ADDR pc;

+  /* The breakpoint's insertion address, possibly with flags encoded in 
the pc
+     (e.g. the instruction mode on ARM).  */
+  CORE_ADDR pcfull;
+
+  /* The breakpoint's data */
+  const unsigned char *data;
+
+  /* The breakpoint's kind.  */
+  int kind;
+
   /* The breakpoint's size.  */
   int size;

@@ -301,24 +309,17 @@ insert_memory_breakpoint (struct raw_breakpoint *bp)
   unsigned char buf[MAX_BREAKPOINT_LEN];
   int err;

-  if (breakpoint_data == NULL)
-    return 1;
-
-  /* If the architecture treats the size field of Z packets as a
-     'kind' field, then we'll need to be able to know which is the
-     breakpoint instruction too.  */
-  if (bp->size != breakpoint_len)
+  if (bp->data == NULL)
     {
       if (debug_threads)
-	debug_printf ("Don't know how to insert breakpoints of size %d.\n",
-		      bp->size);
+	debug_printf ("No breakpoint data present");
       return -1;
     }

   /* Note that there can be fast tracepoint jumps installed in the
      same memory range, so to get at the original memory, we need to
      use read_inferior_memory, which masks those out.  */
-  err = read_inferior_memory (bp->pc, buf, breakpoint_len);
+  err = read_inferior_memory (bp->pc, buf, bp->size);
   if (err != 0)
     {
       if (debug_threads)
@@ -328,10 +329,9 @@ insert_memory_breakpoint (struct raw_breakpoint *bp)
     }
   else
     {
-      memcpy (bp->old_data, buf, breakpoint_len);
+      memcpy (bp->old_data, buf, bp->size);

-      err = (*the_target->write_memory) (bp->pc, breakpoint_data,
-					 breakpoint_len);
+      err = (*the_target->write_memory) (bp->pc, bp->data, bp->size);
       if (err != 0)
 	{
 	  if (debug_threads)
@@ -358,8 +358,8 @@ remove_memory_breakpoint (struct raw_breakpoint *bp)
      note that we need to pass the current shadow contents, because
      write_inferior_memory updates any shadow memory with what we pass
      here, and we want that to be a nop.  */
-  memcpy (buf, bp->old_data, breakpoint_len);
-  err = write_inferior_memory (bp->pc, buf, breakpoint_len);
+  memcpy (buf, bp->old_data, bp->size);
+  err = write_inferior_memory (bp->pc, buf, bp->size);
   if (err != 0)
     {
       if (debug_threads)
@@ -375,15 +375,16 @@ remove_memory_breakpoint (struct raw_breakpoint *bp)
    returns NULL and writes the error code to *ERR.  */

 static struct raw_breakpoint *
-set_raw_breakpoint_at (enum raw_bkpt_type type, CORE_ADDR where, int size,
-		       int *err)
+set_raw_breakpoint_at (enum raw_bkpt_type type, const CORE_ADDR where,
+		       const CORE_ADDR pc, const unsigned char* data, int kind,
+		       int size, int *err)
 {
   struct process_info *proc = current_process ();
   struct raw_breakpoint *bp;

   if (type == raw_bkpt_type_sw || type == raw_bkpt_type_hw)
     {
-      bp = find_enabled_raw_code_breakpoint_at (where, type);
+      bp = find_enabled_raw_code_breakpoint_at (pc, type);
       if (bp != NULL && bp->size != size)
 	{
 	  /* A different size than previously seen.  The previous
@@ -396,7 +397,7 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, 
CORE_ADDR where, int size,
 	}
     }
   else
-    bp = find_raw_breakpoint_at (where, type, size);
+    bp = find_raw_breakpoint_at (pc, type, size);

   if (bp != NULL)
     {
@@ -405,12 +406,15 @@ set_raw_breakpoint_at (enum raw_bkpt_type type, 
CORE_ADDR where, int size,
     }

   bp = XCNEW (struct raw_breakpoint);
-  bp->pc = where;
+  bp->pcfull = where;
+  bp->pc = pc;
+  bp->data = data;
+  bp->kind = kind;
   bp->size = size;
   bp->refcount = 1;
   bp->raw_type = type;

-  *err = the_target->insert_point (bp->raw_type, bp->pc, bp->size, bp);
+  *err = the_target->insert_point (bp->raw_type, bp->pcfull, kind, bp);
   if (*err != 0)
     {
       if (debug_threads)
@@ -740,14 +744,14 @@ reinsert_fast_tracepoint_jumps_at (CORE_ADDR where)

 static struct breakpoint *
 set_breakpoint (enum bkpt_type type, enum raw_bkpt_type raw_type,
-		CORE_ADDR where, int size,
-		int (*handler) (CORE_ADDR), int *err)
+		CORE_ADDR where, CORE_ADDR pc, const unsigned char *data,
+		int kind, int size, int (*handler) (CORE_ADDR), int *err)
 {
   struct process_info *proc = current_process ();
   struct breakpoint *bp;
   struct raw_breakpoint *raw;

-  raw = set_raw_breakpoint_at (raw_type, where, size, err);
+  raw = set_raw_breakpoint_at (raw_type, where, pc, data, kind, size, err);

   if (raw == NULL)
     {
@@ -774,9 +778,16 @@ set_breakpoint_at (CORE_ADDR where, int (*handler) 
(CORE_ADDR))
 {
   int err_ignored;

+  const unsigned char *breakpoint_data;
+  int breakpoint_len;
+  CORE_ADDR pc = where;
+
+  breakpoint_data = the_target->breakpoint_from_pc (&pc, &breakpoint_len);
+
+  /* default breakpoint_len will be initialized downstream.  */
   return set_breakpoint (other_breakpoint, raw_bkpt_type_sw,
-			 where, breakpoint_len, handler,
-			 &err_ignored);
+			 where, pc, breakpoint_data, breakpoint_len,
+			 breakpoint_len, handler, &err_ignored);
 }


@@ -929,6 +940,9 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, 
int size, int *err)
   struct breakpoint *bp;
   enum bkpt_type type;
   enum raw_bkpt_type raw_type;
+  const unsigned char *breakpoint_data;
+  int breakpoint_len = size;
+  CORE_ADDR pc = addr;

   /* If we see GDB inserting a second code breakpoint at the same
      address, then either: GDB is updating the breakpoint's conditions
@@ -993,7 +1007,22 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, 
int size, int *err)

   raw_type = Z_packet_to_raw_bkpt_type (z_type);
   type = Z_packet_to_bkpt_type (z_type);
-  return set_breakpoint (type, raw_type, addr, size, NULL, err);
+
+  if (z_type == Z_PACKET_SW_BP)
+    {
+      breakpoint_data = the_target->breakpoint_from_pc (&pc, 
&breakpoint_len);
+
+      if (breakpoint_len != size)
+	{
+	  if (debug_threads)
+	    debug_printf ("Don't know how to insert breakpoint of size %d.\n",
+		      size);
+	  return NULL;
+	}
+    }
+
+  return set_breakpoint (type, raw_type, addr, pc, breakpoint_data, size,
+			 breakpoint_len, NULL, err);
 }

 static int
@@ -1588,13 +1617,6 @@ check_breakpoints (CORE_ADDR stop_pc)
     }
 }

-void
-set_breakpoint_data (const unsigned char *bp_data, int bp_len)
-{
-  breakpoint_data = bp_data;
-  breakpoint_len = bp_len;
-}
-
 int
 breakpoint_here (CORE_ADDR addr)
 {
@@ -1669,9 +1691,9 @@ validate_inserted_breakpoint (struct 
raw_breakpoint *bp)
   gdb_assert (bp->inserted);
   gdb_assert (bp->raw_type == raw_bkpt_type_sw);

-  buf = alloca (breakpoint_len);
-  err = (*the_target->read_memory) (bp->pc, buf, breakpoint_len);
-  if (err || memcmp (buf, breakpoint_data, breakpoint_len) != 0)
+  buf = alloca (bp->size);
+  err = (*the_target->read_memory) (bp->pc, buf, bp->size);
+  if (err || memcmp (buf, bp->data, bp->size) != 0)
     {
       /* Tag it as gone.  */
       bp->inserted = -1;
@@ -1762,10 +1784,11 @@ check_mem_read (CORE_ADDR mem_addr, unsigned 
char *buf, int mem_len)

   for (; bp != NULL; bp = bp->next)
     {
-      CORE_ADDR bp_end = bp->pc + breakpoint_len;
-      CORE_ADDR start, end;
+      CORE_ADDR bp_end, start, end;
       int copy_offset, copy_len, buf_offset;

+      bp_end = bp->pc + bp->size;
+
       if (bp->raw_type != raw_bkpt_type_sw)
 	continue;

@@ -1851,10 +1874,11 @@ check_mem_write (CORE_ADDR mem_addr, unsigned 
char *buf,

   for (; bp != NULL; bp = bp->next)
     {
-      CORE_ADDR bp_end = bp->pc + breakpoint_len;
-      CORE_ADDR start, end;
+      CORE_ADDR bp_end, start, end;
       int copy_offset, copy_len, buf_offset;

+      bp_end = bp->pc + bp->size;
+
       if (bp->raw_type != raw_bkpt_type_sw)
 	continue;

@@ -1882,7 +1906,7 @@ check_mem_write (CORE_ADDR mem_addr, unsigned char 
*buf,
       if (bp->inserted > 0)
 	{
 	  if (validate_inserted_breakpoint (bp))
-	    memcpy (buf + buf_offset, breakpoint_data + copy_offset, copy_len);
+	    memcpy (buf + buf_offset, bp->data + copy_offset, copy_len);
 	  else
 	    disabled_one = 1;
 	}
@@ -1963,6 +1987,9 @@ clone_one_breakpoint (const struct breakpoint *src)
   dest_raw->raw_type = src->raw->raw_type;
   dest_raw->refcount = src->raw->refcount;
   dest_raw->pc = src->raw->pc;
+  dest_raw->pcfull = src->raw->pcfull;
+  dest_raw->data = src->raw->data;
+  dest_raw->kind = src->raw->kind;
   dest_raw->size = src->raw->size;
   memcpy (dest_raw->old_data, src->raw->old_data, MAX_BREAKPOINT_LEN);
   dest_raw->inserted = src->raw->inserted;