diff mbox

[2/4] Support up to 3 conditional branches in an atomic sequence

Message ID 1395978111-30706-2-git-send-email-anton@samba.org
State Superseded
Headers show

Commit Message

Anton Blanchard March 28, 2014, 3:41 a.m. UTC 
gdb currently supports 1 conditional branch inside a ppc larx/stcx
critical region. Unfortunately there is existing code that contains more
than 1, for example in the ppc64 Linux kernel:

c00000000003ac18 <.__hash_page_4K>:
...
c00000000003ac4c:       ldarx   r31,0,r6
c00000000003ac50:       andc.   r0,r4,r31
c00000000003ac54:       bne-    c00000000003aee8 <htab_wrong_access>
c00000000003ac58:       andi.   r0,r31,2048
c00000000003ac5c:       bne-    c00000000003ae0c <htab_bail_ok>
c00000000003ac60:       rlwinm  r30,r4,30,24,24
c00000000003ac64:       or      r30,r30,r31
c00000000003ac68:       ori     r30,r30,2304
c00000000003ac6c:       oris    r30,r30,4096
c00000000003ac70:       stdcx.  r30,0,r6

If we try to single step through this we get stuck forever because
the reservation is never set when we step over the stdcx.

The following patch bumps the number to 3 conditional branches + 1
terminating branch. With this patch applied I can single step through
the offending function in the ppc64 Linux kernel.

gdb/
2014-03-28  Anton Blanchard  <anton@samba.org>

	* breakpoint.h (MAX_SINGLE_STEP_BREAKPOINTS): New define.
	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
	than two breakpoints.
	* 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.
---
 gdb/breakpoint.c  | 63 ++++++++++++++++++++++++++++---------------------------
 gdb/breakpoint.h  |  6 ++++--
 gdb/rs6000-tdep.c | 46 ++++++++++++++++++++++------------------
 3 files changed, 62 insertions(+), 53 deletions(-)

Comments

Joel Brobecker March 28, 2014, 1:12 p.m. UTC | #1 
On Fri, Mar 28, 2014 at 02:41:49PM +1100, Anton Blanchard wrote:
> gdb currently supports 1 conditional branch inside a ppc larx/stcx
> critical region. Unfortunately there is existing code that contains more
> than 1, for example in the ppc64 Linux kernel:
> 
> c00000000003ac18 <.__hash_page_4K>:
> ...
> c00000000003ac4c:       ldarx   r31,0,r6
> c00000000003ac50:       andc.   r0,r4,r31
> c00000000003ac54:       bne-    c00000000003aee8 <htab_wrong_access>
> c00000000003ac58:       andi.   r0,r31,2048
> c00000000003ac5c:       bne-    c00000000003ae0c <htab_bail_ok>
> c00000000003ac60:       rlwinm  r30,r4,30,24,24
> c00000000003ac64:       or      r30,r30,r31
> c00000000003ac68:       ori     r30,r30,2304
> c00000000003ac6c:       oris    r30,r30,4096
> c00000000003ac70:       stdcx.  r30,0,r6
> 
> If we try to single step through this we get stuck forever because
> the reservation is never set when we step over the stdcx.
> 
> The following patch bumps the number to 3 conditional branches + 1
> terminating branch. With this patch applied I can single step through
> the offending function in the ppc64 Linux kernel.
> 
> gdb/
> 2014-03-28  Anton Blanchard  <anton@samba.org>
> 
> 	* breakpoint.h (MAX_SINGLE_STEP_BREAKPOINTS): New define.
> 	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
> 	than two breakpoints.
> 	* 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.

Overall, this looks like a nice generalization, but Pedro has been
more active in the breakpoint area than I have, so it would be nice
to have his feedback on this patch as well.

IIUC, it looks like MAX_SINGLE_STEP_BREAKPOINTS is actually not
the max, but MAX - 1. I was a little confused by that. Why not
change MAX to 3, and then adjust the array definition and code
accordingly? I think things will be slightly simpler to understand.


> ---
>  gdb/breakpoint.c  | 63 ++++++++++++++++++++++++++++---------------------------
>  gdb/breakpoint.h  |  6 ++++--
>  gdb/rs6000-tdep.c | 46 ++++++++++++++++++++++------------------
>  3 files changed, 62 insertions(+), 53 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 19af9df..b12ea94 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -15036,11 +15036,10 @@ deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp)
>    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.  */
>  
> @@ -15049,19 +15048,17 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
>  			       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
> @@ -15082,8 +15079,13 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
>  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.  */
> @@ -15091,22 +15093,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
> @@ -15119,7 +15120,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]);
> @@ -15136,7 +15137,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]);
> @@ -15151,7 +15152,7 @@ single_step_breakpoint_inserted_here_p (struct address_space *aspace,
>  {
>    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
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index d8e88fc..65d3ecb 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -1443,8 +1443,10 @@ extern void add_solib_catchpoint (char *arg, int is_load, int is_temp,
>     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);
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index dbe3aa2..be14e39 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1088,7 +1088,7 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>    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);
> @@ -1097,7 +1097,6 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>    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
> @@ -1111,24 +1110,20 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>        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++;
>          }
>  
> @@ -1147,18 +1142,29 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>    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;
>  }
> -- 
> 1.8.3.2
Pedro Alves March 28, 2014, 5:12 p.m. UTC | #2 
On 03/28/2014 01:12 PM, Joel Brobecker wrote:
> On Fri, Mar 28, 2014 at 02:41:49PM +1100, Anton Blanchard wrote:
>> gdb currently supports 1 conditional branch inside a ppc larx/stcx
>> critical region. Unfortunately there is existing code that contains more
>> than 1, for example in the ppc64 Linux kernel:
>>
>> c00000000003ac18 <.__hash_page_4K>:
>> ...
>> c00000000003ac4c:       ldarx   r31,0,r6
>> c00000000003ac50:       andc.   r0,r4,r31
>> c00000000003ac54:       bne-    c00000000003aee8 <htab_wrong_access>
>> c00000000003ac58:       andi.   r0,r31,2048
>> c00000000003ac5c:       bne-    c00000000003ae0c <htab_bail_ok>
>> c00000000003ac60:       rlwinm  r30,r4,30,24,24
>> c00000000003ac64:       or      r30,r30,r31
>> c00000000003ac68:       ori     r30,r30,2304
>> c00000000003ac6c:       oris    r30,r30,4096
>> c00000000003ac70:       stdcx.  r30,0,r6
>>
>> If we try to single step through this we get stuck forever because
>> the reservation is never set when we step over the stdcx.
>>
>> The following patch bumps the number to 3 conditional branches + 1
>> terminating branch. With this patch applied I can single step through
>> the offending function in the ppc64 Linux kernel.

Is there a hard limit?  Like, is there a limit on the number of instructions
that can appear inside a ppc larx/stcx region?

>>
>> gdb/
>> 2014-03-28  Anton Blanchard  <anton@samba.org>
>>
>> 	* breakpoint.h (MAX_SINGLE_STEP_BREAKPOINTS): New define.
>> 	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
>> 	than two breakpoints.
>> 	* 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.
> 
> Overall, this looks like a nice generalization, but Pedro has been
> more active in the breakpoint area than I have, so it would be nice
> to have his feedback on this patch as well.
> 
> IIUC, it looks like MAX_SINGLE_STEP_BREAKPOINTS is actually not
> the max, but MAX - 1. I was a little confused by that. Why not
> change MAX to 3, and then adjust the array definition and code
> accordingly? I think things will be slightly simpler to understand.

IMO that would be more confusing.  I read MAX_SINGLE_STEP_BREAKPOINTS
as meaning the "maximum number of of single-step breakpoints we
can create simultaneously".  I think you're reading it as
"the highest index possible into the single-step breakpoints
array" ?

Maybe it'd be clearer to just rename the macro, to, say
NUM_SINGLE_STEP_BREAKPOINTS?

>> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
>> +    if (single_step_breakpoints[i] == NULL)
>> +        break;

I notice something odd with the formatting.  E.g., above,
vs:

>> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
>> +    if (single_step_breakpoints[i] != NULL)
>> +      return 1;

Probably tab vs spaces -- please make use to use tabs
for 8 spaces.

>> --- a/gdb/rs6000-tdep.c
>> +++ b/gdb/rs6000-tdep.c
>> @@ -1088,7 +1088,7 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>>    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];

If we ever bump MAX_SINGLE_STEP_BREAKPOINTS further,
you'd still only want 4 here.

I think it'd be better if this was:

  /* 3 conditional branches + 1 terminating branch.  */
  CORE_ADDR breaks[4];

Followed by:

gdb_static_assert (MAX_SINGLE_STEP_BREAKPOINTS >= ARRAY_SIZE (breaks));

This way clearly documents that we need to support 4 sss breakpoints.
As it is, nothing in your patch leaves any indication in the source to
that effect, so the poor soul trying to revamp software single-step
breakpoints could miss this.

>>    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);
>> @@ -1097,7 +1097,6 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>>    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
>> @@ -1111,24 +1110,20 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>>        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

No need for '>=' IFAICS.  Here I'd suggest:

          if (last_breakpoint == ARRAY_SIZE (breaks) - 1)
            {
              /* Too many conditional branches found, fallback
                 to the standard single-step code.  */
              return 0;
            }

Note "Too" should be capitalized.  also, our style nowadays says
to wrap the comment and statement in a {}s if it's multiline,
even though the old code wasn't doing that.

With those changes this looks good to me.
Pedro Alves March 28, 2014, 5:22 p.m. UTC | #3 
On 03/28/2014 05:12 PM, Pedro Alves wrote:

>>> --- a/gdb/rs6000-tdep.c
>>> +++ b/gdb/rs6000-tdep.c
>>> @@ -1088,7 +1088,7 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>>>    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];
> 
> If we ever bump MAX_SINGLE_STEP_BREAKPOINTS further,
> you'd still only want 4 here.
> 
> I think it'd be better if this was:
> 
>   /* 3 conditional branches + 1 terminating branch.  */
>   CORE_ADDR breaks[4];
> 
> Followed by:
> 
> gdb_static_assert (MAX_SINGLE_STEP_BREAKPOINTS >= ARRAY_SIZE (breaks));
> 
> This way clearly documents that we need to support 4 sss breakpoints.
> As it is, nothing in your patch leaves any indication in the source to
> that effect, so the poor soul trying to revamp software single-step
> breakpoints could miss this.

Sorry, I wrote this before realizing that there could even be more
condition branches in the region and writing the "is there a hard limit",
and then pushed send too soon.  So assuming there's no limit and we're
just trying to be practical in what we support, there's really no harm
in leaving this as:

 CORE_ADDR breaks[MAX_SINGLE_STEP_BREAKPOINTS];

but I still think a comment (+ the assert would be nice) is
missing.  Something like:

/* The ppc64 Linux kernel has regions with 3 conditional branches.
   (plus 1 for the terminating branch).  */
gdb_static_assert (MAX_SINGLE_STEP_BREAKPOINTS >= 4);

?

BTW, shouldn't GDB warn or even error out if too many
conditional branches are found?  I think that'd be better
than silently getting stuck forever.
Joel Brobecker March 28, 2014, 5:32 p.m. UTC | #4 
> > IIUC, it looks like MAX_SINGLE_STEP_BREAKPOINTS is actually not
> > the max, but MAX - 1. I was a little confused by that. Why not
> > change MAX to 3, and then adjust the array definition and code
> > accordingly? I think things will be slightly simpler to understand.
> 
> IMO that would be more confusing.  I read MAX_SINGLE_STEP_BREAKPOINTS
> as meaning the "maximum number of of single-step breakpoints we
> can create simultaneously".  I think you're reading it as
> "the highest index possible into the single-step breakpoints
> array" ?

Here is how I read the patch: MAX_SINGLE_STEP_BREAKPOINTS is the size
of the array, and we rely on the last element always being NULL
to determine the number of "live" elements we actually have.
Hence, to me, the maximum number of SS breakpoints we can handle
in practice is not MAX_SINGLE_STEP_BREAKPOINTS but 1 less. For
instance, I think the patch is trying to increase the number of
SS breakpoints to 3, and yet defines MAX_SINGLE_STEP_BREAKPOINTS
to 4.

Perhaps it's time to just use a vec? That way, we don't have
a limit at all anymore...
Pedro Alves March 28, 2014, 5:57 p.m. UTC | #5 
On 03/28/2014 05:32 PM, Joel Brobecker wrote:
>>> IIUC, it looks like MAX_SINGLE_STEP_BREAKPOINTS is actually not
>>> the max, but MAX - 1. I was a little confused by that. Why not
>>> change MAX to 3, and then adjust the array definition and code
>>> accordingly? I think things will be slightly simpler to understand.
>>
>> IMO that would be more confusing.  I read MAX_SINGLE_STEP_BREAKPOINTS
>> as meaning the "maximum number of of single-step breakpoints we
>> can create simultaneously".  I think you're reading it as
>> "the highest index possible into the single-step breakpoints
>> array" ?
> 
> Here is how I read the patch: MAX_SINGLE_STEP_BREAKPOINTS is the size
> of the array, and we rely on the last element always being NULL
> to determine the number of "live" elements we actually have.

But we can fill the whole array, there's no sentinel.  E.g.:

+  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
+    if (single_step_breakpoints[i] == NULL)
+        break;
+
+  gdb_assert (i < MAX_SINGLE_STEP_BREAKPOINTS);
+

This just looks for the first empty slot.

> Hence, to me, the maximum number of SS breakpoints we can handle
> in practice is not MAX_SINGLE_STEP_BREAKPOINTS but 1 less.

Nope.  We can handle MAX_SINGLE_STEP_BREAKPOINTS.

> For
> instance, I think the patch is trying to increase the number of
> SS breakpoints to 3, and yet defines MAX_SINGLE_STEP_BREAKPOINTS
> to 4.

Anton is making the port handle 3 conditional branches + 1
terminating branch, so that's 4.  I guess it's either the
subject that's confusing you, or this, perhaps:

> -          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.  */

This is "- 1" here because that's leaving space for the terminating
branch.  At least, that's what I understood.

I blame lack of comments in the patch.  :-)

> Perhaps it's time to just use a vec? That way, we don't have
> a limit at all anymore...

Yeah...
Joel Brobecker March 28, 2014, 6:09 p.m. UTC | #6 
> But we can fill the whole array, there's no sentinel.  E.g.:

Ah, ok, I was indeed confused. Lack of comments I will blame also :)

Thanks for clarifying it for me.
diff mbox

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 19af9df..b12ea94 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -15036,11 +15036,10 @@  deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp)
   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.  */
 
@@ -15049,19 +15048,17 @@  insert_single_step_breakpoint (struct gdbarch *gdbarch,
 			       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
@@ -15082,8 +15079,13 @@  insert_single_step_breakpoint (struct gdbarch *gdbarch,
 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.  */
@@ -15091,22 +15093,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
@@ -15119,7 +15120,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]);
@@ -15136,7 +15137,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]);
@@ -15151,7 +15152,7 @@  single_step_breakpoint_inserted_here_p (struct address_space *aspace,
 {
   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
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index d8e88fc..65d3ecb 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1443,8 +1443,10 @@  extern void add_solib_catchpoint (char *arg, int is_load, int is_temp,
    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);
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index dbe3aa2..be14e39 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1088,7 +1088,7 @@  ppc_deal_with_atomic_sequence (struct frame_info *frame)
   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);
@@ -1097,7 +1097,6 @@  ppc_deal_with_atomic_sequence (struct frame_info *frame)
   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
@@ -1111,24 +1110,20 @@  ppc_deal_with_atomic_sequence (struct frame_info *frame)
       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++;
         }
 
@@ -1147,18 +1142,29 @@  ppc_deal_with_atomic_sequence (struct frame_info *frame)
   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;
 }