From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24676 invoked by alias); 15 Oct 2014 17:40:22 -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 24664 invoked by uid 89); 15 Oct 2014 17:40:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 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; Wed, 15 Oct 2014 17:40:20 +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 (8.14.4/8.14.4) with ESMTP id s9FHeGvd009904 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 15 Oct 2014 13:40:17 -0400 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 s9FHeEgO008442; Wed, 15 Oct 2014 13:40:15 -0400 Message-ID: <543EB17E.8090404@redhat.com> Date: Wed, 15 Oct 2014 17:40:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Don Breazeal , gdb-patches@sourceware.org Subject: Re: [PATCH 05/16 v2] GDBserver clone breakpoint list References: <1407434395-19089-1-git-send-email-donb@codesourcery.com> <1408580964-27916-6-git-send-email-donb@codesourcery.com> In-Reply-To: <1408580964-27916-6-git-send-email-donb@codesourcery.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-10/txt/msg00404.txt.bz2 On 08/21/2014 01:29 AM, Don Breazeal wrote: > This patch implements gdbserver routines to clone the breakpoint lists of a > process, duplicating them for another process. In gdbserver, each process > maintains its own independent breakpoint list. When a fork call creates a > child, all of the breakpoints currently inserted in the parent process are > also inserted in the child process, but there is nothing to describe them > in the data structures related to the child. The child must have a > breakpoint list describing them so that they can be removed (if detaching) > or recognized (if following). Implementation is a mechanical process of > just cloning the lists in several new functions in gdbserver/mem-break.c. > > Tested by building, since none of the new functions are called yet. This > was tested with the subsequent patch 6, the follow-fork patch. > Generally looks good. A few nits below. > > gdb/gdbserver/ > 2014-08-20 Don Breazeal > > * mem-break.c (APPEND_TO_LIST): Define macro. > (clone_agent_expr): New function. > (clone_one_breakpoint): New function. > (clone_all_breakpoints): New function. > * mem-break.h: Declare new functions. > > --- > gdb/gdbserver/mem-break.c | 104 +++++++++++++++++++++++++++++++++++++++++++++ > gdb/gdbserver/mem-break.h | 6 +++ > 2 files changed, 110 insertions(+), 0 deletions(-) > > diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c > index 2ce3ab2..7a6062c 100644 > --- a/gdb/gdbserver/mem-break.c > +++ b/gdb/gdbserver/mem-break.c > @@ -28,6 +28,22 @@ int breakpoint_len; > > #define MAX_BREAKPOINT_LEN 8 > > +/* Helper macro used in loops that append multiple items to a singly-linked > + list instead of inserting items at the head of the list, as, say, in the > + breakpoint lists. LISTPP is a pointer to the pointer that is the head of > + the new list. ITEMP is a pointer to the item to be added to the list. > + TAILP must be defined to be the same type as ITEMP, and initialized to > + NULL. */ > + > +#define APPEND_TO_LIST(listpp, itemp, tailp) \ > + { \ > + if (tailp == NULL) \ > + *(listpp) = itemp; \ > + else \ > + tailp->next = itemp; \ > + tailp = itemp; \ > + } Please put ()'s around uses of the parameters. Also, use 'do { ... } while (0)' instead of '{ ... }'. > + > /* GDB will never try to install multiple breakpoints at the same > address. However, we can see GDB requesting to insert a breakpoint > at an address is had already inserted one previously in a few > @@ -1878,3 +1894,91 @@ free_all_breakpoints (struct process_info *proc) > while (proc->breakpoints) > delete_breakpoint_1 (proc, proc->breakpoints); > } > + > +/* Clone an agent expression. */ > + > +static void > +clone_agent_expr (struct agent_expr **new_ax, struct agent_expr *src_ax) > +{ > + struct agent_expr *ax; > + > + ax = xcalloc (1, sizeof (*ax)); > + ax->length = src_ax->length; > + ax->bytes = xcalloc (ax->length, 1); > + memcpy (ax->bytes, src_ax->bytes, ax->length); > + *new_ax = ax; > +} Like memcpy, please make the src argument of these functions be const. Is there a reason this doesn't have the more natural prototype that returns the new clone? static struct agent_expr * clone_agent_expr (const struct agent_expr *src_ax) { struct agent_expr *ax; ax = xcalloc (1, sizeof (*ax)); ax->length = src_ax->length; ax->bytes = xcalloc (ax->length, 1); memcpy (ax->bytes, src_ax->bytes, ax->length); return ax; } > + > +/* Create a new breakpoint list NEW_LIST that is a copy of SRC. Create > + the corresponding new raw_breakpoint list NEW_RAW_LIST as well. */ > + > +void > +clone_all_breakpoints (struct breakpoint **new_list, > + struct raw_breakpoint **new_raw_list, > + struct breakpoint *src) > +{ > + struct breakpoint *bp; > + struct breakpoint *new_bkpt; > + struct raw_breakpoint *new_raw_bkpt; > + struct breakpoint *bkpt_tail = NULL; > + struct raw_breakpoint *raw_bkpt_tail = NULL; > + > + for (bp = src; bp != NULL; bp = bp->next) > + { > + clone_one_breakpoint (&new_bkpt, &new_raw_bkpt, bp); > + APPEND_TO_LIST (new_list, new_bkpt, bkpt_tail); > + APPEND_TO_LIST (new_raw_list, new_raw_bkpt, raw_bkpt_tail); Here this could then be: new_bkpt = clone_one_breakpoint (bp); APPEND_TO_LIST (new_list, new_bkpt, bkpt_tail); APPEND_TO_LIST (new_raw_list, new_bkpt->raw, raw_bkpt_tail); > +/* Create a new breakpoint list NEW_BKPT_LIST that is a copy of SRC. */ > + > +void clone_all_breakpoints (struct breakpoint **new_bkpt_list, > + struct raw_breakpoint **new_raw_bkpt_list, > + struct breakpoint *src); It took me a second to realize that SRC is a list here rather than a single breakpoint. Could you make that more explicit, like e.g.,: /* Create a new breakpoint list NEW_BKPT_LIST that is a copy of the list that starts at SRC. */ Thanks, Pedro Alves