This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2 4/7] Support breakpoint kinds for software breakpoints in GDBServer.




On 10/15/2015 11:51 AM, Pedro Alves wrote:
On 10/05/2015 05:44 PM, Antoine Tremblay wrote:
This patch teaches GDBServer to:

  - choose the right breakpoint instruction for its own breakpoints, through API
    set_breakpoint_at.

  - choose the right breakpoint instruction for breakpoints requested by GDB,
   according to the information in Z packets, through API set_gdb_breakpoint.

New fields are introduced in struct raw_breakpoint:

pcfull: The PC including possible arch specific flags encoded in it.

"full" as opposed to "empty"?  Can we find a clearer term?


full as opposed to incomplete, meaning it includes all it could include. Other then that I would see :

pcencoded ?

pcflaged ?

pcwithflags ?

Not an easy one..

@@ -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;

Can't we always find the size from pcfull and kind ?


We could but then we would have to call breakpoint_from_kind in a lot of places basically everywhere bp->size is referenced like :

check_mem_read
check_mem_write
insert_memory_breakpoint
remove_memory_breakpoint
set_raw_breakpoint_at
validate_inserted_breakpoint
delete_raw_breakpoint
uninsert_raw_breakpoint
reinsert_raw_breakpoint
find_raw_breakpoint_at

Also since these functions can be called in a stack one would have to be careful to call breakpoint_from_kind at the right level and pass it down.. and then size/kind becomes confusing.

Also, this is a bit what I did in v1 but changed based on discussions with Yao see :

https://sourceware.org/ml/gdb-patches/2015-09/msg00597.html

I think it's more clear to call the function once and set the variable.


@@ -293,6 +301,30 @@ find_raw_breakpoint_at (CORE_ADDR addr, enum raw_bkpt_type type, int size)
    return NULL;
  }

+/* Try to resolve the real breakpoint size from the breakpoint kind  */
+
+static int
+breakpoint_from_kind (int kind,
+		      const unsigned char **breakpoint_data,
+		      int *breakpoint_len)
+{
+  /* Get the arch dependent breakpoint.  */
+  if (*the_target->breakpoint_from_kind != NULL)
+    {
+      /* Update magic coded size to the right size if needed.  */
+      *breakpoint_data =
+       (*the_target->breakpoint_from_kind) (&kind);
+      *breakpoint_len = kind;
+    }
+  else {

Formatting.


Done.

@@ -375,15 +399,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)

Which is which: "where" vs "pc" | "pc" vs "pcfull" ?  I think the terminology
should be consistent throughout.  Also remember to update intro comments.


Yes indeed this is confusing but I hesitated to change it since across gdb "where" is used for a location, even before this change where was translated to pc in the breakpoint struct.

It felt a bit weird to call set_breakpoint_at(pcfull) compared to like find_breakpoint_at (where).

But in this case we have where and pc I think it's necessary indeed.

Done.

@@ -405,12 +430,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;

Why do we need to store "data" per breakpoint?  Can't we just call
the_target->breakpoint_from_pc when necessary?

For the same reasons as expressed before for ->size I think it's better not to call breakpoint_from_pc at the lowest level.

@@ -918,17 +952,24 @@ z_type_supported (char z_type)
  	  && the_target->supports_z_point_type (z_type));
  }

-/* Create a new GDB breakpoint of type Z_TYPE at ADDR with size SIZE.
+/* Create a new GDB breakpoint of type Z_TYPE at ADDR with kind KIND.
     Returns a pointer to the newly created breakpoint on success.  On
     failure returns NULL and sets *ERR to either -1 for error, or 1 if
     Z_TYPE breakpoints are not supported on this target.  */

  static struct breakpoint *
-set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err)
+set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind, int *err)
  {
    struct breakpoint *bp;
    enum bkpt_type type;
    enum raw_bkpt_type raw_type;
+  const unsigned char *breakpoint_data = NULL;
+  int breakpoint_len = kind;
+
+  if (z_type == Z_PACKET_SW_BP)
+    {
+      breakpoint_from_kind (kind, &breakpoint_data, &breakpoint_len);
+    }

Unnecessary braces.


Done.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]