From patchwork Mon Sep 28 20:59:38 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Antoine Tremblay X-Patchwork-Id: 8874 Received: (qmail 65851 invoked by alias); 28 Sep 2015 20:59:44 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 65840 invoked by uid 89); 28 Sep 2015 20:59:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 X-HELO: usevmg20.ericsson.net Received: from usevmg20.ericsson.net (HELO usevmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 28 Sep 2015 20:59:41 +0000 Received: from EUSAAHC005.ericsson.se (Unknown_Domain [147.117.188.87]) by usevmg20.ericsson.net (Symantec Mail Security) with SMTP id 37.D6.32596.DCA49065; Mon, 28 Sep 2015 16:12:30 +0200 (CEST) Received: from [142.133.110.95] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.89) with Microsoft SMTP Server id 14.3.248.2; Mon, 28 Sep 2015 16:59:38 -0400 From: Antoine Tremblay Subject: Re: [PATCH 2/5] Make breakpoint and breakpoint_len local variables in GDBServer. To: Yao Qi References: <1442577749-6650-1-git-send-email-antoine.tremblay@ericsson.com> <1442577749-6650-3-git-send-email-antoine.tremblay@ericsson.com> <867fna98d3.fsf@gmail.com> CC: Message-ID: <5609AA3A.8070706@ericsson.com> Date: Mon, 28 Sep 2015 16:59:38 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <867fna98d3.fsf@gmail.com> X-IsSubscribed: yes On 09/28/2015 05:54 AM, Yao Qi wrote: > Antoine Tremblay 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 ? @@ -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; 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;