From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25263 invoked by alias); 7 Jun 2013 20:40:48 -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 25253 invoked by uid 89); 7 Jun 2013 20:40:47 -0000 X-Spam-SWARE-Status: No, score=-7.8 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 07 Jun 2013 20:40:46 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r57Kedl8007247 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 7 Jun 2013 16:40:39 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r57KebTG007728; Fri, 7 Jun 2013 16:40:38 -0400 Message-ID: <51B24545.5010509@redhat.com> Date: Fri, 07 Jun 2013 20:50:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH 1/5] Remove global variable tracepoint_list and stepping_list. References: <1370610493-26468-1-git-send-email-yao@codesourcery.com> <1370610493-26468-2-git-send-email-yao@codesourcery.com> In-Reply-To: <1370610493-26468-2-git-send-email-yao@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-06/txt/msg00182.txt.bz2 On 06/07/2013 02:08 PM, Yao Qi wrote: > This patch is a refactor patch. Simply, it removes two global > variables tracepoint_list and stepping_list. > > gdb: > > 2013-06-07 Pedro Alves > Yao Qi > > * tracepoint.c (tracepoint_list, stepping_list): Remove. > (do_clear_collection_list, init_collection_list): New. > (encode_actions): Change the type of the second and third > parameter from 'char ***' to 'struct collection_list *'. > (encode_actions): Rename to encode_actions_rsp, and rewrite on top > of the new encode_actions implementation. > (encode_actions): New function. Also, "Make static.". > (_initialize_tracepoint): Delete references to 'tracepoint_list' > and 'stepping_list'. > * tracepoint.h (encode_actions): Remove declaration. > (encode_actions_rsp): Add declaration. > * remote.c (remote_download_tracepoint): Caller update. > > /* MEMRANGE functions: */ > > @@ -1239,6 +1238,35 @@ clear_collection_list (struct collection_list *list) > list->next_aexpr_elt = 0; > memset (list->regs_mask, 0, sizeof (list->regs_mask)); > list->strace_data = 0; > + > + xfree (list->aexpr_list); > + xfree (list->list); > +} This change is not mentioned in the ChangeLog. > + > +/* Wrapper to function clear_collection_list. */ "Wrapper for". "to" would be used to introduce a rationale for the wrapping, like e.g., "wrapper to allow frobing ...". I suggest mentioning cleanups, like e.g.,: /* A cleanup wrapper for clear_collection_list. */ > /* Reduce a collection list to string form (for gdb protocol). */ > @@ -1606,37 +1634,50 @@ encode_actions_1 (struct command_line *action, > } /* for */ > } > > -/* Render all actions into gdb protocol. */ > - > -void > -encode_actions (struct bp_location *tloc, char ***tdp_actions, > - char ***stepping_actions) > +static void > +encode_actions (struct bp_location *tloc, > + struct collection_list *tracepoint_list, > + struct collection_list *stepping_list) Missing docu. Ah, found it in patch 4. (would have been best merged here). > { > - char *default_collect_line = NULL; > - struct command_line *actions; > - struct command_line *default_collect_action = NULL; > int frame_reg; > LONGEST frame_offset; > - struct cleanup *back_to; > - > - back_to = make_cleanup (null_cleanup, NULL); > + struct command_line *actions; > + struct cleanup *old_chain; > > - clear_collection_list (&tracepoint_list); > - clear_collection_list (&stepping_list); > + old_chain = make_cleanup (null_cleanup, NULL); > > - *tdp_actions = NULL; > - *stepping_actions = NULL; > + actions = all_tracepoint_actions_and_cleanup (tloc->owner); > > gdbarch_virtual_frame_pointer (tloc->gdbarch, > tloc->address, &frame_reg, &frame_offset); > > - actions = all_tracepoint_actions_and_cleanup (tloc->owner); > - > encode_actions_1 (actions, tloc, frame_reg, frame_offset, > - &tracepoint_list, &stepping_list); > + tracepoint_list, stepping_list); > + > + do_cleanups (old_chain); > + > + memrange_sortmerge (tracepoint_list); > + memrange_sortmerge (stepping_list); > +} > + > +/* Render all actions into gdb protocol. */ > + > +void > +encode_actions_rsp (struct bp_location *tloc, > + char ***tdp_actions, char ***stepping_actions) > +{ > + struct cleanup *back_to; > + struct collection_list tracepoint_list, stepping_list; > + > + back_to = make_cleanup (null_cleanup, NULL); > > - memrange_sortmerge (&tracepoint_list); > - memrange_sortmerge (&stepping_list); > + init_collection_list (&tracepoint_list); > + init_collection_list (&stepping_list); > + > + make_cleanup (do_clear_collection_list, &tracepoint_list); > + make_cleanup (do_clear_collection_list, &stepping_list); Thanks for splitting all this up into pieces. Much appreciated. However, there's a weird back and forth dance going on between this patch and patch 4, making the reader dizzy. :-) Patch 4 moves these back to encode_actions... There's only one caller of encode_actions in this patch, so it'd be better to leaving encode_actions as a single function in this patch, and leave the split for one of the follow ups (possible patch 4), explaining that the split allows reusing the encode bits without generating the RSP, necessary for the MI command's implementation. > + > + encode_actions (tloc, &tracepoint_list, &stepping_list); -- Pedro Alves