Re: [PATCH 2/3] Support up to 3 conditional branches in an atomic sequence

[Joel Brobecker <brobecker@adacore.com>]

> Here is a new version.
[...]
> 2012-06-05  Anton Blanchard  <anton@samba.org>
> 
> 	* gdb/breakpoint.h: Define MAX_SINGLE_STEP_BREAKPOINTS
> 	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
> 	than two breakpoints.
> 	* gdb/breakpoint.c (insert_single_step_breakpoint): Likewise
> 	(insert_single_step_breakpoint): Likewise
> 	(single_step_breakpoints_inserted): Likewise
> 	(cancel_single_step_breakpoints): Likewise
> 	(detach_single_step_breakpoints): Likewise
> 	(single_step_breakpoint_inserted_here_p): Likewise

Thanks for sending the new version, and sorry for dropping the ball on
this one.

This version of the patch is OK.

> Index: b/gdb/rs6000-tdep.c
> ===================================================================
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1087,7 +1087,7 @@ ppc_deal_with_atomic_sequence (struct fr
>    struct address_space *aspace = get_frame_address_space (frame);
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>    CORE_ADDR pc = get_frame_pc (frame);
> -  CORE_ADDR breaks[2] = {-1, -1};
> +  CORE_ADDR breaks[MAX_SINGLE_STEP_BREAKPOINTS];
>    CORE_ADDR loc = pc;
>    CORE_ADDR closing_insn; /* Instruction that closes the atomic sequence.  */
>    int insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> @@ -1096,7 +1096,6 @@ ppc_deal_with_atomic_sequence (struct fr
>    int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */  
>    const int atomic_sequence_length = 16; /* Instruction sequence length.  */
>    int opcode; /* Branch instruction's OPcode.  */
> -  int bc_insn_count = 0; /* Conditional branch instruction count.  */
>  
>    /* Assume all atomic sequences start with a lwarx/ldarx instruction.  */
>    if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
> @@ -1110,24 +1109,20 @@ ppc_deal_with_atomic_sequence (struct fr
>        loc += PPC_INSN_SIZE;
>        insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
>  
> -      /* Assume that there is at most one conditional branch in the atomic
> -         sequence.  If a conditional branch is found, put a breakpoint in 
> -         its destination address.  */
>        if ((insn & BRANCH_MASK) == BC_INSN)
>          {
>            int immediate = ((insn & 0xfffc) ^ 0x8000) - 0x8000;
>            int absolute = insn & 2;
>  
> -          if (bc_insn_count >= 1)
> -            return 0; /* More than one conditional branch found, fallback 
> +          if (last_breakpoint >= MAX_SINGLE_STEP_BREAKPOINTS - 1)
> +            return 0; /* too many conditional branches found, fallback
>                           to the standard single-step code.  */
>   
>  	  if (absolute)
> -	    breaks[1] = immediate;
> +	    breaks[last_breakpoint] = immediate;
>  	  else
> -	    breaks[1] = loc + immediate;
> +	    breaks[last_breakpoint] = loc + immediate;
>  
> -	  bc_insn_count++;
>  	  last_breakpoint++;
>          }
>  
> @@ -1146,18 +1141,29 @@ ppc_deal_with_atomic_sequence (struct fr
>    insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
>  
>    /* Insert a breakpoint right after the end of the atomic sequence.  */
> -  breaks[0] = loc;
> +  breaks[last_breakpoint] = loc;
>  
> -  /* Check for duplicated breakpoints.  Check also for a breakpoint
> -     placed (branch instruction's destination) anywhere in sequence.  */
> -  if (last_breakpoint
> -      && (breaks[1] == breaks[0]
> -	  || (breaks[1] >= pc && breaks[1] <= closing_insn)))
> -    last_breakpoint = 0;
> -
> -  /* Effectively inserts the breakpoints.  */
>    for (index = 0; index <= last_breakpoint; index++)
> -    insert_single_step_breakpoint (gdbarch, aspace, breaks[index]);
> +    {
> +      int index2;
> +      int insert_bp = 1;
> +
> +      /* Check for a breakpoint placed (branch instruction's destination)
> +	 anywhere in sequence.  */
> +      if (breaks[index] >= pc && breaks[index] <= closing_insn)
> +	continue;
> +
> +      /* Check for duplicated breakpoints.  */
> +      for (index2 = 0; index2 < index; index2++)
> +        {
> +	  if (breaks[index] == breaks[index2])
> +	    insert_bp = 0;
> +	}
> +
> +      /* insert the breakpoint.  */
> +      if (insert_bp)
> +        insert_single_step_breakpoint (gdbarch, aspace, breaks[index]);
> +    }
>  
>    return 1;
>  }
> Index: b/gdb/breakpoint.c
> ===================================================================
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -14741,11 +14741,10 @@ deprecated_remove_raw_breakpoint (struct
>    return ret;
>  }
>  
> -/* One (or perhaps two) breakpoints used for software single
> -   stepping.  */
> +/* Breakpoints used for software single stepping.  */
>  
> -static void *single_step_breakpoints[2];
> -static struct gdbarch *single_step_gdbarch[2];
> +static void *single_step_breakpoints[MAX_SINGLE_STEP_BREAKPOINTS];
> +static struct gdbarch *single_step_gdbarch[MAX_SINGLE_STEP_BREAKPOINTS];
>  
>  /* Create and insert a breakpoint for software single step.  */
>  
> @@ -14754,19 +14753,17 @@ insert_single_step_breakpoint (struct gd
>  			       struct address_space *aspace, 
>  			       CORE_ADDR next_pc)
>  {
> +  int i;
>    void **bpt_p;
>  
> -  if (single_step_breakpoints[0] == NULL)
> -    {
> -      bpt_p = &single_step_breakpoints[0];
> -      single_step_gdbarch[0] = gdbarch;
> -    }
> -  else
> -    {
> -      gdb_assert (single_step_breakpoints[1] == NULL);
> -      bpt_p = &single_step_breakpoints[1];
> -      single_step_gdbarch[1] = gdbarch;
> -    }
> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
> +    if (single_step_breakpoints[i] == NULL)
> +        break;
> +
> +  gdb_assert (i < MAX_SINGLE_STEP_BREAKPOINTS);
> +
> +  bpt_p = &single_step_breakpoints[i];
> +  single_step_gdbarch[i] = gdbarch;
>  
>    /* NOTE drow/2006-04-11: A future improvement to this function would
>       be to only create the breakpoints once, and actually put them on
> @@ -14787,8 +14784,13 @@ insert_single_step_breakpoint (struct gd
>  int
>  single_step_breakpoints_inserted (void)
>  {
> -  return (single_step_breakpoints[0] != NULL
> -          || single_step_breakpoints[1] != NULL);
> +  int i;
> +
> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
> +    if (single_step_breakpoints[i] != NULL)
> +      return 1;
> +
> +  return 0;
>  }
>  
>  /* Remove and delete any breakpoints used for software single step.  */
> @@ -14796,22 +14798,21 @@ single_step_breakpoints_inserted (void)
>  void
>  remove_single_step_breakpoints (void)
>  {
> +  int i;
> +
>    gdb_assert (single_step_breakpoints[0] != NULL);
>  
>    /* See insert_single_step_breakpoint for more about this deprecated
>       call.  */
> -  deprecated_remove_raw_breakpoint (single_step_gdbarch[0],
> -				    single_step_breakpoints[0]);
> -  single_step_gdbarch[0] = NULL;
> -  single_step_breakpoints[0] = NULL;
>  
> -  if (single_step_breakpoints[1] != NULL)
> -    {
> -      deprecated_remove_raw_breakpoint (single_step_gdbarch[1],
> -					single_step_breakpoints[1]);
> -      single_step_gdbarch[1] = NULL;
> -      single_step_breakpoints[1] = NULL;
> -    }
> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
> +    if (single_step_breakpoints[i] != NULL)
> +      {
> +	deprecated_remove_raw_breakpoint (single_step_gdbarch[i],
> +					  single_step_breakpoints[i]);
> +	single_step_gdbarch[i] = NULL;
> +	single_step_breakpoints[i] = NULL;
> +      }
>  }
>  
>  /* Delete software single step breakpoints without removing them from
> @@ -14824,7 +14825,7 @@ cancel_single_step_breakpoints (void)
>  {
>    int i;
>  
> -  for (i = 0; i < 2; i++)
> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
>      if (single_step_breakpoints[i])
>        {
>  	xfree (single_step_breakpoints[i]);
> @@ -14841,7 +14842,7 @@ detach_single_step_breakpoints (void)
>  {
>    int i;
>  
> -  for (i = 0; i < 2; i++)
> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
>      if (single_step_breakpoints[i])
>        target_remove_breakpoint (single_step_gdbarch[i],
>  				single_step_breakpoints[i]);
> @@ -14856,7 +14857,7 @@ single_step_breakpoint_inserted_here_p (
>  {
>    int i;
>  
> -  for (i = 0; i < 2; i++)
> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
>      {
>        struct bp_target_info *bp_tgt = single_step_breakpoints[i];
>        if (bp_tgt
> Index: b/gdb/breakpoint.h
> ===================================================================
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -1408,8 +1408,10 @@ extern int is_catchpoint (struct breakpo
>     deletes all breakpoints.  */
>  extern void delete_command (char *arg, int from_tty);
>  
> -/* Manage a software single step breakpoint (or two).  Insert may be
> -   called twice before remove is called.  */
> +/* Manage software single step breakpoints.  Insert may be
> +   called multiple times before remove is called.  */
> +#define MAX_SINGLE_STEP_BREAKPOINTS 4
> +
>  extern void insert_single_step_breakpoint (struct gdbarch *,
>  					   struct address_space *, 
>  					   CORE_ADDR);

-- 
Joel