Re: [PATCH 05/16 v2] GDBserver clone breakpoint list

Breazeal, Don
2014-10-31T23:44:44Z

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