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

[Pedro Alves <palves@redhat.com>]

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