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

Pedro Alves
2014-03-28T17:13:09Z

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
>