From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 111673 invoked by alias); 15 Oct 2015 15:51:40 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 111663 invoked by uid 89); 15 Oct 2015 15:51:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 15 Oct 2015 15:51:36 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 4CB408F290; Thu, 15 Oct 2015 15:51:35 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t9FFpXJJ011894; Thu, 15 Oct 2015 11:51:34 -0400 Message-ID: <561FCB85.4020500@redhat.com> Date: Thu, 15 Oct 2015 15:51:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Antoine Tremblay , gdb-patches@sourceware.org Subject: Re: [PATCH v2 4/7] Support breakpoint kinds for software breakpoints in GDBServer. References: <1444063455-31558-1-git-send-email-antoine.tremblay@ericsson.com> <1444063455-31558-5-git-send-email-antoine.tremblay@ericsson.com> In-Reply-To: <1444063455-31558-5-git-send-email-antoine.tremblay@ericsson.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-10/txt/msg00250.txt.bz2 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? > data: This is the breakpoint's raw data. > kind: As meant in a z0 packet this is the kind of breakpoint to be set. > > Note this also clarifies existing fields usage : > > pc: This is PC without any flags as a valid memory address. > size: This is the breakpoint's size in memory. > > Function signatures, and variables names have also been updated to make the > distinction between size and kind clear. > > Note also that an unimplemented breakpoint_from_kind operation is not an error > as this would break targets like QNX Neutrino (nto). > > To check for this kind of error a check for the breakpoint_data is done in > insert_memory_breakpoint. > > No regressions on Ubuntu 14.04 on ARMv7 and x86. > With gdbserver-{native,extended} / { -marm -mthumb } > > gdb/gdbserver/ChangeLog: > * linux-low.c (initialize_low): Remove breakpoint_data initialization. > * mem-break.c (struct raw_breakpoint) : New field. > (struct raw_breakpoint) : New field. > (struct raw_breakpoint) : New field. > (breakpoint_from_kind): New function. > (insert_memory_breakpoint): Adjust to use bp->size and bp->data. > (remove_memory_breakpoint): Adjust to use bp->size. > (set_raw_breakpoint_at): Add pc, data, kind fields arguments and > (set_breakpoint): Likewise. > (set_breakpoint_at): Call breakpoint_from_pc. > (delete_breakpoint): Rename size for kind. > (find_gdb_breakpoint): Use kind rather than size. > (set_gdb_breakpoint_1): Rename size for kind, call breakpoint_from_kind. > (set_gdb_breakpoint): Rename size for kind. > (delete_gdb_breakpoint_1): Rename size for kind. > (delete_gdb_breakpoint_1): Likewise. > (set_breakpoint_data): Remove. > (validate_inserted_breakpoint): Adjust to use bp->size and bp->data. > (check_mem_read): Adjust to use bp->size. > (check_mem_write): Adjust to use bp->size and bp->data. > (clone_one_breakpoint): Clone new fields, pcfull, data, kind. > * server.c (process_serial_event): Rename len for kind. > --- > gdb/gdbserver/linux-low.c | 6 -- > gdb/gdbserver/mem-break.c | 154 +++++++++++++++++++++++++++++----------------- > gdb/gdbserver/server.c | 8 +-- > 3 files changed, 100 insertions(+), 68 deletions(-) > > diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c > index 5aa2b1d..c410deb 100644 > --- a/gdb/gdbserver/linux-low.c > +++ b/gdb/gdbserver/linux-low.c > @@ -7077,16 +7077,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 f077497..4299cfa 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; Can't we always find the size from pcfull and kind ? > > @@ -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. > + if (debug_threads) > + debug_printf ("Don't know breakpoints of size %d.\n", > + kind); > + return -1; > + } > + return 0; > +} > + > /* See mem-break.h. */ > > int > @@ -301,24 +333,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 +353,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 +382,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 +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. > { > 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 +421,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 +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? > + 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 +768,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 +802,15 @@ 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); > + > 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); > } > > > @@ -891,12 +925,12 @@ delete_breakpoint (struct breakpoint *todel) > return delete_breakpoint_1 (proc, todel); > } > > -/* Locate a GDB breakpoint of type Z_TYPE and size SIZE placed at > - address ADDR and return a pointer to its structure. If SIZE is -1, > +/* Locate a GDB breakpoint of type Z_TYPE and kind KIND placed at > + address ADDR and return a pointer to its structure. If KIND is -1, > the breakpoints' sizes are ignored. */ > > static struct breakpoint * > -find_gdb_breakpoint (char z_type, CORE_ADDR addr, int size) > +find_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind) > { > struct process_info *proc = current_process (); > struct breakpoint *bp; > @@ -904,7 +938,7 @@ find_gdb_breakpoint (char z_type, CORE_ADDR addr, int size) > > for (bp = proc->breakpoints; bp != NULL; bp = bp->next) > if (bp->type == type && bp->raw->pc == addr > - && (size == -1 || bp->raw->size == size)) > + && (kind == -1 || bp->raw->kind == kind)) > return bp; > > return NULL; > @@ -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. > > /* If we see GDB inserting a second code breakpoint at the same > address, then either: GDB is updating the breakpoint's conditions > @@ -952,9 +993,9 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err) > > if (bp != NULL) > { > - if (bp->raw->size != size) > + if (bp->raw->kind != kind) > { > - /* A different size than previously seen. The previous > + /* A different kind than previously seen. The previous > breakpoint must be gone then. */ > bp->raw->inserted = -1; > delete_breakpoint (bp); > @@ -978,7 +1019,7 @@ set_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size, int *err) > /* Data breakpoints for the same address but different size are > expected. GDB doesn't merge these. The backend gets to do > that if it wants/can. */ > - bp = find_gdb_breakpoint (z_type, addr, size); > + bp = find_gdb_breakpoint (z_type, addr, kind); > } > > if (bp != NULL) > @@ -993,7 +1034,8 @@ 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); > + return set_breakpoint (type, raw_type, addr, addr, breakpoint_data, kind, > + breakpoint_len, NULL, err); > } > > static int > @@ -1024,7 +1066,7 @@ check_gdb_bp_preconditions (char z_type, int *err) > knows to prepare to access memory for Z0 breakpoints. */ > > struct breakpoint * > -set_gdb_breakpoint (char z_type, CORE_ADDR addr, int size, int *err) > +set_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind, int *err) > { > struct breakpoint *bp; > > @@ -1040,7 +1082,7 @@ set_gdb_breakpoint (char z_type, CORE_ADDR addr, int size, int *err) > return NULL; > } > > - bp = set_gdb_breakpoint_1 (z_type, addr, size, err); > + bp = set_gdb_breakpoint_1 (z_type, addr, kind, err); > > if (z_type == Z_PACKET_SW_BP) > done_accessing_memory (); > @@ -1048,18 +1090,18 @@ set_gdb_breakpoint (char z_type, CORE_ADDR addr, int size, int *err) > return bp; > } > > -/* Delete a GDB breakpoint of type Z_TYPE and size SIZE previously > +/* Delete a GDB breakpoint of type Z_TYPE and kind KIND previously > inserted at ADDR with set_gdb_breakpoint_at. Returns 0 on success, > -1 on error, and 1 if Z_TYPE breakpoints are not supported on this > target. */ > > static int > -delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size) > +delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int kind) > { > struct breakpoint *bp; > int err; > > - bp = find_gdb_breakpoint (z_type, addr, size); > + bp = find_gdb_breakpoint (z_type, addr, kind); > if (bp == NULL) > return -1; > > @@ -1077,7 +1119,7 @@ delete_gdb_breakpoint_1 (char z_type, CORE_ADDR addr, int size) > knows to prepare to access memory for Z0 breakpoints. */ > > int > -delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int size) > +delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int kind) > { > int ret; > > @@ -1095,7 +1137,7 @@ delete_gdb_breakpoint (char z_type, CORE_ADDR addr, int size) > return -1; > } > > - ret = delete_gdb_breakpoint_1 (z_type, addr, size); > + ret = delete_gdb_breakpoint_1 (z_type, addr, kind); > > if (z_type == Z_PACKET_SW_BP) > done_accessing_memory (); > @@ -1588,13 +1630,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 +1704,9 @@ validate_inserted_breakpoint (struct raw_breakpoint *bp) > gdb_assert (bp->inserted); > gdb_assert (bp->raw_type == raw_bkpt_type_sw); > > - buf = (unsigned char *) alloca (breakpoint_len); > - err = (*the_target->read_memory) (bp->pc, buf, breakpoint_len); > - if (err || memcmp (buf, breakpoint_data, breakpoint_len) != 0) > + buf = (unsigned char *) 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,7 +1797,7 @@ 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 bp_end = bp->pc + bp->size; > CORE_ADDR start, end; > int copy_offset, copy_len, buf_offset; > > @@ -1851,7 +1886,7 @@ 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 bp_end = bp->pc + bp->size; > CORE_ADDR start, end; > int copy_offset, copy_len, buf_offset; > > @@ -1882,7 +1917,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 +1998,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; > diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c > index e25b7c7..ad6626e 100644 > --- a/gdb/gdbserver/server.c > +++ b/gdb/gdbserver/server.c > @@ -4069,20 +4069,20 @@ process_serial_event (void) > { > char *dataptr; > ULONGEST addr; > - int len; > + int kind; > char type = own_buf[1]; > int res; > const int insert = ch == 'Z'; > char *p = &own_buf[3]; > > p = unpack_varlen_hex (p, &addr); > - len = strtol (p + 1, &dataptr, 16); > + kind = strtol (p + 1, &dataptr, 16); > > if (insert) > { > struct breakpoint *bp; > > - bp = set_gdb_breakpoint (type, addr, len, &res); > + bp = set_gdb_breakpoint (type, addr, kind, &res); > if (bp != NULL) > { > res = 0; > @@ -4097,7 +4097,7 @@ process_serial_event (void) > } > } > else > - res = delete_gdb_breakpoint (type, addr, len); > + res = delete_gdb_breakpoint (type, addr, kind); > > if (res == 0) > write_ok (own_buf); > Thanks, Pedro Alves