This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 05/16 v2] GDBserver clone breakpoint list
- From: "Breazeal, Don" <donb at codesourcery dot com>
- To: Pedro Alves <palves at redhat dot com>, gdb-patches at sourceware dot org
- Date: Fri, 31 Oct 2014 16:44:38 -0700
- Subject: Re: [PATCH 05/16 v2] GDBserver clone breakpoint list
- Authentication-results: sourceware.org; auth=none
- References: <1407434395-19089-1-git-send-email-donb at codesourcery dot com> <1408580964-27916-6-git-send-email-donb at codesourcery dot com> <543EB17E dot 8090404 at redhat dot com>
On 10/15/2014 10:40 AM, Pedro Alves wrote:
> 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.
>> +#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 '{ ... }'.
Done.
>> +static void
>> +clone_agent_expr (struct agent_expr **new_ax, struct agent_expr *src_ax)
>> +{
>
> Like memcpy, please make the src argument of these functions be
> const.
Done.
>
> Is there a reason this doesn't have the more natural prototype that
> returns the new clone?
No reason. I've made this change in the new version.
> 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);
And this change is made as well.
>
> 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. */
I made this change, and also changed the name of SRC to SRC_LIST.
Thanks for the review. I've posted an updated version of this patch
here: https://sourceware.org/ml/gdb-patches/2014-10/msg00870.html.
It's part of a new version of the whole patch series, updated to work
with more recent changes.
Thanks,
--Don