From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6493 invoked by alias); 15 Oct 2015 18:02:41 -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 6466 invoked by uid 89); 15 Oct 2015 18:02:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 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; Thu, 15 Oct 2015 18:02:38 +0000 Received: from EUSAAHC002.ericsson.se (Unknown_Domain [147.117.188.78]) by usevmg20.ericsson.net (Symantec Mail Security) with SMTP id 49.B3.32596.FE98F165; Thu, 15 Oct 2015 13:11:43 +0200 (CEST) Received: from [142.133.110.95] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.80) with Microsoft SMTP Server id 14.3.248.2; Thu, 15 Oct 2015 14:02:34 -0400 Subject: Re: [PATCH v2 4/7] Support breakpoint kinds for software breakpoints in GDBServer. To: Pedro Alves , References: <1444063455-31558-1-git-send-email-antoine.tremblay@ericsson.com> <1444063455-31558-5-git-send-email-antoine.tremblay@ericsson.com> <561FCB85.4020500@redhat.com> From: Antoine Tremblay Message-ID: <561FEA3A.5020801@ericsson.com> Date: Thu, 15 Oct 2015 18:02:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <561FCB85.4020500@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg00269.txt.bz2 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.