[v2,ppc64] Add POWER8 atomic sequences single-stepping support

  Hi Luis,
thanks for the feedback, I made changes following your suggestions.

On 02/06/2017 08:03 AM, Luis Machado wrote:
> On 02/05/2017 09:03 PM, Edjunior Barbosa Machado wrote:
>> @@ -986,9 +986,15 @@ typedef BP_MANIPULATION_ENDIAN
>> (little_breakpoint, big_breakpoint)
>>  #define LWARX_MASK 0xfc0007fe
>>  #define LWARX_INSTRUCTION 0x7c000028
>>  #define LDARX_INSTRUCTION 0x7c0000A8
>> +#define LBARX_INSTRUCTION 0x7c000068
>> +#define LHARX_INSTRUCTION 0x7c0000e8
>> +#define LQARX_INSTRUCTION 0x7c000228
>>  #define STWCX_MASK 0xfc0007ff
>>  #define STWCX_INSTRUCTION 0x7c00012d
>>  #define STDCX_INSTRUCTION 0x7c0001ad
>> +#define STBCX_INSTRUCTION 0x7c00056d
>> +#define STHCX_INSTRUCTION 0x7c0005ad
>> +#define STQCX_INSTRUCTION 0x7c00016d
> 
> I'm looking at the above and it is starting to get slightly confusing.
> Maybe we should rename LWARX_MASK to something more suitable, since it
> really means "mask for any Load Reserve instruction with basic opcode 31".
> 
> Maybe LOAD_RESERVE_MASK? LR_MASK with a comment?
> 
> Same problem with STWCX_MASK, which is used for the same purpose.
> 

Renamed to LOAD_AND_RESERVE_MASK and STORE_CONDITIONAL_MASK.

>>  /* We can't displaced step atomic sequences.  Otherwise this is just
>>     like simple_displaced_step_copy_insn.  */
>> @@ -1010,7 +1016,10 @@ ppc_displaced_step_copy_insn (struct gdbarch
>> *gdbarch,
>>
>>    /* Assume all atomic sequences start with a lwarx/ldarx
>> instruction.  */
>>    if ((insn & LWARX_MASK) == LWARX_INSTRUCTION
>> -      || (insn & LWARX_MASK) == LDARX_INSTRUCTION)
>> +      || (insn & LWARX_MASK) == LDARX_INSTRUCTION
>> +      || (insn & LWARX_MASK) == LBARX_INSTRUCTION
>> +      || (insn & LWARX_MASK) == LHARX_INSTRUCTION
>> +      || (insn & LWARX_MASK) == LQARX_INSTRUCTION)
>>      {
>>        if (debug_displaced)
>>      {
> 
> These conditionals are getting a bit cluttered. How about moving them to
> a function that checks for a match instead?

Created macros IS_LOAD_AND_RESERVE_INSN and IS_STORE_CONDITIONAL_INSN.

> 
>> diff --git a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.S
>> b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.S
>> index 52c887f..6c84fd8 100644
>> --- a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.S
>> +++ b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.S
>> @@ -49,9 +49,64 @@ main:
>>      bne    3f
>>      addi    5,5,1
>>      stdcx.    5,0,4
>> -    bne    1b
>> +    bne    2b
>> +
>> +    stb    0,0(4)
>> +3:    lbarx    5,0,4
> 
> I think lbarx/lharx/lqarx requires a new ISA level, right? If so, i
> think we need to test this in a separate testcase that is specific to
> that ISA level and newer or at least have guards in place so compilers
> not targeting such ISA (and newer ISA's) can do the right thing. More
> comments about this below.
> 

Althougth lbarx/stbcx and lharx/sthcx were introduced on PowerISA 2.06,
they were implemented only on 2.07 (POWER8) along with lqarx/stqcx.
I created a separate c file to check for the ISA level via HWCAP2 bits
before test the new atomic sequences.

>> diff --git a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
>> b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
>> index d1b3a7d..678a4a7 100644
>> --- a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
>> +++ b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
>> @@ -28,7 +28,8 @@ if {![istarget "powerpc*"] || ![is_lp64_target]} {
>>
>>  standard_testfile .S
>>
>> -if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}
>> {debug quiet}] } {
>> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
>> +    [list debug quiet additional_flags=-mcpu=power8]] } {
> 
> This seems to be restricting testing to compilers that only support
> -mcpu=power8, which is not the case for older compilers or compilers
> that only target embedded.
> 
>> @@ -52,6 +54,18 @@ proc do_test { displaced } {
>>      gdb_breakpoint "$bp2" "Breakpoint $decimal at $hex" \
>>      "Set the breakpoint at the start of the ldarx/stdcx sequence"
>>
>> +    set bp3 [gdb_get_line_number "lbarx"]
>> +    gdb_breakpoint "$bp3" "Breakpoint $decimal at $hex" \
>> +    "Set the breakpoint at the start of the lbarx/stbcx sequence"
> 
> I think my previous set of testsuite fixups failed to catch these test
> names starting with uppercase. But we're requiring test names to start
> with lowercase now. I can address the rest of the offenders in a future
> patch, but new code should have the right format.

I'll send this fix in a separate patch then.

Please let me know if you have any additional comments.

Thanks,
--
Edjunior

gdb/
2017-02-13  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>

	* rs6000-tdep.c (LOAD_AND_RESERVE_MASK): Rename from LWARX_MASK.
	(STORE_CONDITIONAL_MASK): Rename from STWCX_MASK.
	(LBARX_INSTRUCTION, LHARX_INSTRUCTION, LQARX_INSTRUCTION,
	STBCX_INSTRUCTION, STHCX_INSTRUCTION, STQCX_INSTRUCTION): New defines.
	(IS_LOAD_AND_RESERVE_INSN, IS_STORE_CONDITIONAL_INSN): New macros.
	(ppc_displaced_step_copy_insn): Use IS_LOAD_AND_RESERVE_INSN.
	(ppc_deal_with_atomic_sequence): Use IS_LOAD_AND_RESERVE_INSN and
	IS_STORE_CONDITIONAL_INSN.

gdb/testsuite/
2017-02-13  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>

	* gdb.arch/power8-atomic-inst.exp: New testcase based on
	gdb.arch/ppc64-atomic-inst.exp.  Add tests for lbarx/stbcx,
	lharx/sthcx and lqarx/stqcx.
	* gdb.arch/power8-atomic-inst.S: New file.
	* gdb.arch/power8-atomic-inst.c: Likewise.

---
 gdb/rs6000-tdep.c                             |  53 +++++++++-----
 gdb/testsuite/gdb.arch/power8-atomic-inst.S   | 100 ++++++++++++++++++++++++++
 gdb/testsuite/gdb.arch/power8-atomic-inst.c   |  42 +++++++++++
 gdb/testsuite/gdb.arch/power8-atomic-inst.exp |  97 +++++++++++++++++++++++++
 4 files changed, 274 insertions(+), 18 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/power8-atomic-inst.S
 create mode 100644 gdb/testsuite/gdb.arch/power8-atomic-inst.c
 create mode 100644 gdb/testsuite/gdb.arch/power8-atomic-inst.exp

[v2,ppc64] Add POWER8 atomic sequences single-stepping support

Commit Message

Comments

Patch