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

[Pedro Alves <palves at redhat dot com>]

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

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