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

Pedro Alves
2014-10-15T17:40:22Z

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 <donb@codesourcery.com>
>
> * 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