[05/16,v2] GDBserver clone breakpoint list

  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.

Thanks
--Don

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(-)
  

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
  

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