[PATCH] [ppc64] Add POWER8 atomic sequences single-stepping support

[PATCH] [ppc64] Add POWER8 atomic sequences single-stepping support

[Edjunior Barbosa Machado]

Hi,
this patch aims to add single-stepping support for POWER8 atomic sequences
lbarx/stbcx, lharx/sthcx and lqarx/stqcx. Tested on ppc64 and ppc64le. Ok?

Thanks,
--
Edjunior

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

	* rs6000-tdep.c (LBARX_INSTRUCTION, LHARX_INSTRUCTION,
	LQARX_INSTRUCTION, STBCX_INSTRUCTION, STHCX_INSTRUCTION,
	STQCX_INSTRUCTION): New defines.
	(ppc_displaced_step_copy_insn): Check for lbarx/stbcx, lharx/sthcx and
	lqarx/stqcx.
	(ppc_deal_with_atomic_sequence): Likewise.

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

	* gdb.arch/ppc64-atomic-inst.exp: Add tests for lbarx/stbcx,
	lharx/sthcx and lqarx/stqcx.
	* gdb.arch/ppc64-atomic-inst.S: Likewise.


---
 gdb/rs6000-tdep.c                            | 26 ++++++++++--
 gdb/testsuite/gdb.arch/ppc64-atomic-inst.S   | 59 +++++++++++++++++++++++++++-
 gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp | 47 +++++++++++++++++++++-
 3 files changed, 124 insertions(+), 8 deletions(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 527f643..4cd3b59 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -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
 
 /* 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)
 	{
@@ -1162,7 +1171,10 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
 
   /* 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)
     return NULL;
 
   /* Assume that no atomic sequence is longer than "atomic_sequence_length" 
@@ -1194,13 +1206,19 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
         }
 
       if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
-          || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+          || (insn & STWCX_MASK) == STDCX_INSTRUCTION
+          || (insn & STWCX_MASK) == STBCX_INSTRUCTION
+          || (insn & STWCX_MASK) == STHCX_INSTRUCTION
+          || (insn & STWCX_MASK) == STQCX_INSTRUCTION)
         break;
     }
 
   /* Assume that the atomic sequence ends with a stwcx/stdcx instruction.  */
   if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
-      && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
+      && (insn & STWCX_MASK) != STDCX_INSTRUCTION
+      && (insn & STWCX_MASK) != STBCX_INSTRUCTION
+      && (insn & STWCX_MASK) != STHCX_INSTRUCTION
+      && (insn & STWCX_MASK) != STQCX_INSTRUCTION)
     return NULL;
 
   closing_insn = loc;
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
+	cmpdi	5,0
+	bne	4f
+	addi	5,5,1
+	stbcx.	5,0,4
+	bne	3b
+
+	sth	0,0(4)
+4:	lharx	5,0,4
+	cmpdi	5,0
+	bne	5f
+	addi	5,5,1
+	sthcx.	5,0,4
+	bne	4b
+
+#ifdef	__BIG_ENDIAN__
+	li 10,0
+	li 6,0
+	li 7,1
+	std 10,-16(1)
+	li 10,1
+	std 10,-8(1)
+	addi 4,1,-16
+#else
+	std 9,40(1)
+	li 9,1
+	addi 4,1,32
+	std 9,32(1)
+	mr 8,9
+	ld 3,8(4)
+#endif
+5:	lqarx 10,0,4
+#ifdef	__BIG_ENDIAN__
+	li 8,0
+	li 9,2
+	mr 5,10
+	xor 10,11,7
+	xor 5,5,6
+	or. 4,5,10
+	bne- 6f
+	addi 10,1,-16
+	stqcx. 8,0,10
+#else
+	xor 9,11,8
+	mr 6,11
+	xor 11,10,3
+	or. 0,9,11
+	bne 6f
+	li 14,0
+	li 15,2
+	stqcx. 14,0,4
+#endif
+	bne 5b
 
-3:	li	3,0
+6:	li	3,0
 	blr
 
 #if _CALL_ELF == 2
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]] } {
     return -1
 }
 
@@ -36,6 +37,7 @@ if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} {debug quie
 # stepping.
 proc do_test { displaced } {
     global decimal hex
+    global gdb_prompt
 
     if ![runto_main] then {
 	untested "could not run to main"
@@ -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"
+
+    set bp4 [gdb_get_line_number "lharx"]
+    gdb_breakpoint "$bp4" "Breakpoint $decimal at $hex" \
+	"Set the breakpoint at the start of the lharx/sthcx sequence"
+
+    set bp5 [gdb_get_line_number "lqarx"]
+    gdb_breakpoint "$bp5" "Breakpoint $decimal at $hex" \
+	"Set the breakpoint at the start of the lqarx/stqcx sequence"
+
     gdb_test continue "Continuing.*Breakpoint $decimal.*" \
 	"Continue until lwarx/stwcx start breakpoint"
 
@@ -61,8 +75,37 @@ proc do_test { displaced } {
     gdb_test continue "Continuing.*Breakpoint $decimal.*" \
 	"Continue until ldarx/stdcx start breakpoint"
 
-    gdb_test nexti "bne.*1b" \
+    gdb_test nexti "bne.*2b" \
 	"Step through the ldarx/stdcx sequence"
+
+    gdb_test continue "Continuing.*Breakpoint $decimal.*" \
+	"Continue until lbarx/stbcx start breakpoint"
+
+    gdb_test_multiple "nexti" "Check for lbarx instruction support" {
+	-re "Program received signal SIGILL,.*\r\n$gdb_prompt $" {
+	    unsupported "lbarx instruction unsupported"
+	    return
+	}
+	-re "bne.*3b\r\n$gdb_prompt $" {
+	    pass "Step through the lbarx/stbcx sequence"
+	}
+	-re "$gdb_prompt $" {
+	    unsupported "lbarx instruction unsupported (unknown error)"
+	    return
+	}
+    }
+
+    gdb_test continue "Continuing.*Breakpoint $decimal.*" \
+	"Continue until lharx/sthcx start breakpoint"
+
+    gdb_test nexti "bne.*4b" \
+	"Step through the lharx/sthcx sequence"
+
+    gdb_test continue "Continuing.*Breakpoint $decimal.*" \
+	"Continue until ldqrx/stqcx start breakpoint"
+
+    gdb_test nexti "bne.*5b" \
+	"Step through the lqarx/stqcx sequence"
 }
 
 foreach displaced { "off" "on" } {
-- 
2.9.3

Re: [PATCH] [ppc64] Add POWER8 atomic sequences single-stepping support

[Luis Machado]

On 02/05/2017 09:03 PM, Edjunior Barbosa Machado wrote:
> Hi,
> this patch aims to add single-stepping support for POWER8 atomic sequences
> lbarx/stbcx, lharx/sthcx and lqarx/stqcx. Tested on ppc64 and ppc64le. Ok?
>
> Thanks,
> --
> Edjunior
>
> gdb/
> 2017-02-06  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
>
> 	* rs6000-tdep.c (LBARX_INSTRUCTION, LHARX_INSTRUCTION,
> 	LQARX_INSTRUCTION, STBCX_INSTRUCTION, STHCX_INSTRUCTION,
> 	STQCX_INSTRUCTION): New defines.
> 	(ppc_displaced_step_copy_insn): Check for lbarx/stbcx, lharx/sthcx and
> 	lqarx/stqcx.
> 	(ppc_deal_with_atomic_sequence): Likewise.
>
> gdb/testsuite/
> 2017-02-06  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
>
> 	* gdb.arch/ppc64-atomic-inst.exp: Add tests for lbarx/stbcx,
> 	lharx/sthcx and lqarx/stqcx.
> 	* gdb.arch/ppc64-atomic-inst.S: Likewise.
>
>
> ---
>  gdb/rs6000-tdep.c                            | 26 ++++++++++--
>  gdb/testsuite/gdb.arch/ppc64-atomic-inst.S   | 59 +++++++++++++++++++++++++++-
>  gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp | 47 +++++++++++++++++++++-
>  3 files changed, 124 insertions(+), 8 deletions(-)

>
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 527f643..4cd3b59 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -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.

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

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

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

> +
> +    set bp4 [gdb_get_line_number "lharx"]
> +    gdb_breakpoint "$bp4" "Breakpoint $decimal at $hex" \
> +	"Set the breakpoint at the start of the lharx/sthcx sequence"
> +
> +    set bp5 [gdb_get_line_number "lqarx"]
> +    gdb_breakpoint "$bp5" "Breakpoint $decimal at $hex" \
> +	"Set the breakpoint at the start of the lqarx/stqcx sequence"
> +
>      gdb_test continue "Continuing.*Breakpoint $decimal.*" \
>  	"Continue until lwarx/stwcx start breakpoint"
>
> @@ -61,8 +75,37 @@ proc do_test { displaced } {
>      gdb_test continue "Continuing.*Breakpoint $decimal.*" \
>  	"Continue until ldarx/stdcx start breakpoint"
>
> -    gdb_test nexti "bne.*1b" \
> +    gdb_test nexti "bne.*2b" \
>  	"Step through the ldarx/stdcx sequence"
> +
> +    gdb_test continue "Continuing.*Breakpoint $decimal.*" \
> +	"Continue until lbarx/stbcx start breakpoint"
> +
> +    gdb_test_multiple "nexti" "Check for lbarx instruction support" {
> +	-re "Program received signal SIGILL,.*\r\n$gdb_prompt $" {
> +	    unsupported "lbarx instruction unsupported"
> +	    return
> +	}

This works fine when you have an OS to let you know a SIGILL happened, 
but a bare-metal target will likely go belly up. I think this test needs 
to have a runtime check to make sure the instructions are supported or 
be restricted to only the target we know for sure support them.

> +	-re "bne.*3b\r\n$gdb_prompt $" {
> +	    pass "Step through the lbarx/stbcx sequence"
> +	}
> +	-re "$gdb_prompt $" {
> +	    unsupported "lbarx instruction unsupported (unknown error)"
> +	    return
> +	}
> +    }
> +
> +    gdb_test continue "Continuing.*Breakpoint $decimal.*" \
> +	"Continue until lharx/sthcx start breakpoint"
> +
> +    gdb_test nexti "bne.*4b" \
> +	"Step through the lharx/sthcx sequence"
> +
> +    gdb_test continue "Continuing.*Breakpoint $decimal.*" \
> +	"Continue until ldqrx/stqcx start breakpoint"
> +
> +    gdb_test nexti "bne.*5b" \
> +	"Step through the lqarx/stqcx sequence"
>  }
>
>  foreach displaced { "off" "on" } {
>

Re: [PATCH] [ppc64] Add POWER8 atomic sequences single-stepping support

[Peter Bergner]

On 2/6/17 4:03 AM, Luis Machado wrote:
> On 02/05/2017 09:03 PM, Edjunior Barbosa Machado wrote:
> Maybe LOAD_RESERVE_MASK? LR_MASK with a comment?

Please not LR_MASK, since it could be confusing given we have
an LR register.


Peter

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

[Edjunior Barbosa Machado]

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

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 527f643..8bbcaa7 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -983,12 +983,33 @@ typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
 
 /* Instruction masks used during single-stepping of atomic
    sequences.  */
-#define LWARX_MASK 0xfc0007fe
+#define LOAD_AND_RESERVE_MASK 0xfc0007fe
 #define LWARX_INSTRUCTION 0x7c000028
 #define LDARX_INSTRUCTION 0x7c0000A8
-#define STWCX_MASK 0xfc0007ff
+#define LBARX_INSTRUCTION 0x7c000068
+#define LHARX_INSTRUCTION 0x7c0000e8
+#define LQARX_INSTRUCTION 0x7c000228
+#define STORE_CONDITIONAL_MASK 0xfc0007ff
 #define STWCX_INSTRUCTION 0x7c00012d
 #define STDCX_INSTRUCTION 0x7c0001ad
+#define STBCX_INSTRUCTION 0x7c00056d
+#define STHCX_INSTRUCTION 0x7c0005ad
+#define STQCX_INSTRUCTION 0x7c00016d
+
+/* Check if insn is one of the Load And Reserve instructions used for atomic
+   sequences.  */
+#define IS_LOAD_AND_RESERVE_INSN(insn)	((insn & LOAD_AND_RESERVE_MASK) == LWARX_INSTRUCTION \
+					 || (insn & LOAD_AND_RESERVE_MASK) == LDARX_INSTRUCTION \
+					 || (insn & LOAD_AND_RESERVE_MASK) == LBARX_INSTRUCTION \
+					 || (insn & LOAD_AND_RESERVE_MASK) == LHARX_INSTRUCTION \
+					 || (insn & LOAD_AND_RESERVE_MASK) == LQARX_INSTRUCTION)
+/* Check if insn is one of the Store Conditional instructions used for atomic
+   sequences.  */
+#define IS_STORE_CONDITIONAL_INSN(insn)      ((insn & STORE_CONDITIONAL_MASK) == STWCX_INSTRUCTION \
+						 || (insn & STORE_CONDITIONAL_MASK) == STDCX_INSTRUCTION \
+						 || (insn & STORE_CONDITIONAL_MASK) == STBCX_INSTRUCTION \
+						 || (insn & STORE_CONDITIONAL_MASK) == STHCX_INSTRUCTION \
+						 || (insn & STORE_CONDITIONAL_MASK) == STQCX_INSTRUCTION)
 
 /* We can't displaced step atomic sequences.  Otherwise this is just
    like simple_displaced_step_copy_insn.  */
@@ -1008,9 +1029,8 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
 
   insn = extract_signed_integer (buf, PPC_INSN_SIZE, byte_order);
 
-  /* Assume all atomic sequences start with a lwarx/ldarx instruction.  */
-  if ((insn & LWARX_MASK) == LWARX_INSTRUCTION
-      || (insn & LWARX_MASK) == LDARX_INSTRUCTION)
+  /* Assume all atomic sequences start with a Load and Reserve instruction.  */
+  if (IS_LOAD_AND_RESERVE_INSN(insn))
     {
       if (debug_displaced)
 	{
@@ -1138,11 +1158,10 @@ ppc_displaced_step_hw_singlestep (struct gdbarch *gdbarch,
   return 1;
 }
 
-/* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
-   instruction and ending with a STWCX/STDCX instruction.  If such a sequence
-   is found, attempt to step through it.  A breakpoint is placed at the end of 
-   the sequence.  */
-
+/* Checks for an atomic sequence of instructions beginning with a
+   Load And Reserve instruction and ending with a Store Conditional
+   instruction.  If such a sequence is found, attempt to step through it.
+   A breakpoint is placed at the end of the sequence.  */
 VEC (CORE_ADDR) *
 ppc_deal_with_atomic_sequence (struct regcache *regcache)
 {
@@ -1160,9 +1179,8 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
   int bc_insn_count = 0; /* Conditional branch instruction count.  */
   VEC (CORE_ADDR) *next_pcs = NULL;
 
-  /* Assume all atomic sequences start with a lwarx/ldarx instruction.  */
-  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
-      && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
+  /* Assume all atomic sequences start with a Load And Reserve instruction.  */
+  if (!IS_LOAD_AND_RESERVE_INSN(insn))
     return NULL;
 
   /* Assume that no atomic sequence is longer than "atomic_sequence_length" 
@@ -1193,14 +1211,13 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
 	  last_breakpoint++;
         }
 
-      if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
-          || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+      if (IS_STORE_CONDITIONAL_INSN(insn))
         break;
     }
 
-  /* Assume that the atomic sequence ends with a stwcx/stdcx instruction.  */
-  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
-      && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
+  /* Assume that the atomic sequence ends with a Store Conditional
+     instruction.  */
+  if (!IS_STORE_CONDITIONAL_INSN(insn))
     return NULL;
 
   closing_insn = loc;
diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.S b/gdb/testsuite/gdb.arch/power8-atomic-inst.S
new file mode 100644
index 0000000..daa3337
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.S
@@ -0,0 +1,100 @@
+/* This file is part of GDB, the GNU debugger.
+
+   Copyright 2017 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+	.align 2
+	.globl test_atomic_sequences
+#if _CALL_ELF == 2
+	.type test_atomic_sequences,@function
+test_atomic_sequences:
+#else
+	.section ".opd","aw"
+	.align 3
+test_atomic_sequences:
+	.quad .test_atomic_sequences,.TOC.@tocbase,0
+	.size test_atomic_sequences,.-test_atomic_sequences
+	.previous
+	.globl .test_atomic_sequences
+	.type .test_atomic_sequences,@function
+.test_atomic_sequences:
+#endif
+
+	li	0,0
+	addi	4,1,-8
+
+	stb	0,0(4)
+1:	lbarx	5,0,4
+	cmpdi	5,0
+	bne	2f
+	addi	5,5,1
+	stbcx.	5,0,4
+	bne	1b
+
+	sth	0,0(4)
+2:	lharx	5,0,4
+	cmpdi	5,0
+	bne	3f
+	addi	5,5,1
+	sthcx.	5,0,4
+	bne	2b
+
+#ifdef	__BIG_ENDIAN__
+	li 10,0
+	li 6,0
+	li 7,1
+	std 10,-16(1)
+	li 10,1
+	std 10,-8(1)
+	addi 4,1,-16
+#else
+	std 9,40(1)
+	li 9,1
+	addi 4,1,32
+	std 9,32(1)
+	mr 8,9
+	ld 3,8(4)
+#endif
+3:	lqarx 10,0,4
+#ifdef	__BIG_ENDIAN__
+	li 8,0
+	li 9,2
+	mr 5,10
+	xor 10,11,7
+	xor 5,5,6
+	or. 4,5,10
+	bne 4f
+	addi 10,1,-16
+	stqcx. 8,0,10
+#else
+	xor 9,11,8
+	mr 6,11
+	xor 11,10,3
+	or. 0,9,11
+	bne 4f
+	li 14,0
+	li 15,2
+	stqcx. 14,0,4
+#endif
+	bne 3b
+
+4:	li	3,0
+	blr
+
+#if _CALL_ELF == 2
+	.size test_atomic_sequences,.-test_atomic_sequences
+#else
+	.size .test_atomic_sequences,.-.test_atomic_sequences
+#endif
diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.c b/gdb/testsuite/gdb.arch/power8-atomic-inst.c
new file mode 100644
index 0000000..535e057
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.c
@@ -0,0 +1,42 @@
+/* Copyright 2017 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <elf.h>
+
+typedef Elf64_auxv_t auxv_t;
+
+#ifndef PPC_FEATURE2_ARCH_2_07
+#define PPC_FEATURE2_ARCH_2_07	0x80000000
+#endif
+
+extern void test_atomic_sequences (void);
+
+int
+main (int argc, char *argv[], char *envp[], auxv_t auxv[])
+{
+  int i;
+
+  for (i = 0; auxv[i].a_type != AT_NULL; i++)
+    if (auxv[i].a_type == AT_HWCAP2) {
+      if (!(auxv[i].a_un.a_val & PPC_FEATURE2_ARCH_2_07))
+        return 1;
+      break;
+    }
+
+  test_atomic_sequences ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.exp b/gdb/testsuite/gdb.arch/power8-atomic-inst.exp
new file mode 100644
index 0000000..9e13310
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.exp
@@ -0,0 +1,97 @@
+# Copyright 2017 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+#
+# This file is part of the gdb testsuite.
+
+# Test single stepping through atomic sequences beginning with
+# a lbarx/lharx/lqarx instruction and ending with a stbcx/sthcx/stqxc
+# instruction.
+
+
+if {![istarget "powerpc*"] || ![is_lp64_target]} {
+    verbose "Skipping testing of powerpc64 single stepping over atomic sequences."
+    return
+}
+
+standard_testfile  .c .S
+
+if { [prepare_for_testing "failed to prepare" $testfile "$srcfile $srcfile2" \
+      {debug quiet}] } {
+    return -1
+}
+
+# The test proper.  DISPLACED is true if we should try with displaced
+# stepping.
+proc do_test { displaced } {
+    global decimal hex
+    global gdb_prompt inferior_exited_re srcfile srcfile2
+
+    if ![runto_main] then {
+	untested "could not run to main"
+	return -1
+    }
+
+    gdb_test_no_output "set displaced-stepping $displaced"
+
+    gdb_breakpoint "test_atomic_sequences" "Breakpoint $decimal at $hex" \
+	"set the breakpoint at the start of the test function"
+
+    gdb_test_multiple "continue" "Continue until lbarx/stbcx start breakpoint" {
+      -re "$inferior_exited_re with code 01.\[\r\n\]+$gdb_prompt $" {
+	unsupported "POWER8 atomic instructions not supported."
+	return -1
+      }
+      -re "Continuing.*Breakpoint $decimal.*$gdb_prompt $" {
+	pass "continue until test_atomic_sequences function"
+      }
+    }
+
+    set bp1 [gdb_get_line_number "lbarx" "$srcfile2"]
+    gdb_breakpoint "$bp1" "Breakpoint $decimal at $hex" \
+	"set the breakpoint at the start of the lbarx/stbcx sequence"
+
+    set bp2 [gdb_get_line_number "lharx" "$srcfile2"]
+    gdb_breakpoint "$bp2" "Breakpoint $decimal at $hex" \
+	"set the breakpoint at the start of the lharx/sthcx sequence"
+
+    set bp3 [gdb_get_line_number "lqarx" "$srcfile2"]
+    gdb_breakpoint "$bp3" "Breakpoint $decimal at $hex" \
+	"set the breakpoint at the start of the lqarx/stqcx sequence"
+
+    gdb_test continue "Continuing.*Breakpoint $decimal.*" \
+	"continue until lbarx/stbcx start breakpoint"
+
+    gdb_test nexti "bne.*1b" \
+	"step through the lbarx/stbcx sequence"
+
+    gdb_test continue "Continuing.*Breakpoint $decimal.*" \
+	"continue until lharx/sthcx start breakpoint"
+
+    gdb_test nexti "bne.*2b" \
+	"step through the lharx/sthcx sequence"
+
+    gdb_test continue "Continuing.*Breakpoint $decimal.*" \
+	"continue until ldqrx/stqcx start breakpoint"
+
+    gdb_test nexti "bne.*3b" \
+	"step through the lqarx/stqcx sequence"
+}
+
+foreach displaced { "off" "on" } {
+    with_test_prefix "displaced=$displaced" {
+	do_test $displaced
+    }
+}
-- 
2.9.3

[PATCH] [trivial] Fix test names starting with uppercase in gdb.arch/ppc64-atomic-inst.exp

[Edjunior Barbosa Machado]

On 02/06/2017 08:03 AM, Luis Machado wrote:
>> @@ -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.
> 

Hi,
this trivial patch fixes the test names starting with uppercase using gdb_test
in gdb.arch/ppc64-atomic-inst.exp.

Thanks,
--
Edjunior

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

	* gdb.arch/ppc64-atomic-inst.exp: Fix test names starting with
	uppercase.

---
 gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
index d1b3a7d..7bfff6c 100644
--- a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
+++ b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
@@ -46,23 +46,23 @@ proc do_test { displaced } {
 
     set bp1 [gdb_get_line_number "lwarx"]
     gdb_breakpoint "$bp1" "Breakpoint $decimal at $hex" \
-	"Set the breakpoint at the start of the lwarx/stwcx sequence"
+	"set the breakpoint at the start of the lwarx/stwcx sequence"
 
     set bp2 [gdb_get_line_number "ldarx"]
     gdb_breakpoint "$bp2" "Breakpoint $decimal at $hex" \
-	"Set the breakpoint at the start of the ldarx/stdcx sequence"
+	"set the breakpoint at the start of the ldarx/stdcx sequence"
 
     gdb_test continue "Continuing.*Breakpoint $decimal.*" \
-	"Continue until lwarx/stwcx start breakpoint"
+	"continue until lwarx/stwcx start breakpoint"
 
     gdb_test nexti "bne.*1b" \
-	"Step through the lwarx/stwcx sequence"
+	"step through the lwarx/stwcx sequence"
 
     gdb_test continue "Continuing.*Breakpoint $decimal.*" \
-	"Continue until ldarx/stdcx start breakpoint"
+	"continue until ldarx/stdcx start breakpoint"
 
     gdb_test nexti "bne.*1b" \
-	"Step through the ldarx/stdcx sequence"
+	"step through the ldarx/stdcx sequence"
 }
 
 foreach displaced { "off" "on" } {
-- 
1.8.3.1

Re: [PATCH] [trivial] Fix test names starting with uppercase in gdb.arch/ppc64-atomic-inst.exp

[Luis Machado]

On 02/13/2017 09:35 PM, Edjunior Barbosa Machado wrote:
> On 02/06/2017 08:03 AM, Luis Machado wrote:
>>> @@ -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.
>>
>
> Hi,
> this trivial patch fixes the test names starting with uppercase using gdb_test
> in gdb.arch/ppc64-atomic-inst.exp.
>
> Thanks,
> --
> Edjunior
>
> gdb/testsuite/
> 2017-02-14  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
>
> 	* gdb.arch/ppc64-atomic-inst.exp: Fix test names starting with
> 	uppercase.
>
> ---
>  gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
> index d1b3a7d..7bfff6c 100644
> --- a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
> +++ b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
> @@ -46,23 +46,23 @@ proc do_test { displaced } {
>
>      set bp1 [gdb_get_line_number "lwarx"]
>      gdb_breakpoint "$bp1" "Breakpoint $decimal at $hex" \
> -	"Set the breakpoint at the start of the lwarx/stwcx sequence"
> +	"set the breakpoint at the start of the lwarx/stwcx sequence"
>
>      set bp2 [gdb_get_line_number "ldarx"]
>      gdb_breakpoint "$bp2" "Breakpoint $decimal at $hex" \
> -	"Set the breakpoint at the start of the ldarx/stdcx sequence"
> +	"set the breakpoint at the start of the ldarx/stdcx sequence"
>
>      gdb_test continue "Continuing.*Breakpoint $decimal.*" \
> -	"Continue until lwarx/stwcx start breakpoint"
> +	"continue until lwarx/stwcx start breakpoint"
>
>      gdb_test nexti "bne.*1b" \
> -	"Step through the lwarx/stwcx sequence"
> +	"step through the lwarx/stwcx sequence"
>
>      gdb_test continue "Continuing.*Breakpoint $decimal.*" \
> -	"Continue until ldarx/stdcx start breakpoint"
> +	"continue until ldarx/stdcx start breakpoint"
>
>      gdb_test nexti "bne.*1b" \
> -	"Step through the ldarx/stdcx sequence"
> +	"step through the ldarx/stdcx sequence"
>  }
>
>  foreach displaced { "off" "on" } {
>

LGTM.

Re: [PATCH v2] [ppc64] Add POWER8 atomic sequences single-stepping support

[Luis Machado]

On 02/13/2017 06:47 PM, Edjunior Barbosa Machado wrote:
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 527f643..8bbcaa7 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
...
> @@ -1160,9 +1179,8 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
>    int bc_insn_count = 0; /* Conditional branch instruction count.  */
>    VEC (CORE_ADDR) *next_pcs = NULL;
>
> -  /* Assume all atomic sequences start with a lwarx/ldarx instruction.  */
> -  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
> -      && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
> +  /* Assume all atomic sequences start with a Load And Reserve instruction.  */
> +  if (!IS_LOAD_AND_RESERVE_INSN(insn))

Space before (. More occurrences of this.

>      return NULL;
>
>    /* Assume that no atomic sequence is longer than "atomic_sequence_length"
> @@ -1193,14 +1211,13 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
>  	  last_breakpoint++;
>          }
>
> -      if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
> -          || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
> +      if (IS_STORE_CONDITIONAL_INSN(insn))

Here.

>          break;
>      }
>
> -  /* Assume that the atomic sequence ends with a stwcx/stdcx instruction.  */
> -  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
> -      && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
> +  /* Assume that the atomic sequence ends with a Store Conditional
> +     instruction.  */
> +  if (!IS_STORE_CONDITIONAL_INSN(insn))

Here.

> diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.S b/gdb/testsuite/gdb.arch/power8-atomic-inst.S
> new file mode 100644
> index 0000000..daa3337
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.S

I don't know if there are other powerpc initiatives out there other than 
IBM's power 8/9 that are using these instructions. If there are, 
renaming power8 to something generic would be best. Otherwise i don't 
see a problem with leaving this and fixing it in the future if some 
other manufacturer shows up using ISA 2.06/2.07.

I thought i'd mention it though.

> diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.c b/gdb/testsuite/gdb.arch/power8-atomic-inst.c
> new file mode 100644
> index 0000000..535e057
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.c

Same as above about mentioning power8 in the filename.

> @@ -0,0 +1,42 @@
> +/* Copyright 2017 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <elf.h>
> +
> +typedef Elf64_auxv_t auxv_t;
> +
> +#ifndef PPC_FEATURE2_ARCH_2_07
> +#define PPC_FEATURE2_ARCH_2_07	0x80000000
> +#endif
> +
> +extern void test_atomic_sequences (void);
> +
> +int
> +main (int argc, char *argv[], char *envp[], auxv_t auxv[])
> +{
> +  int i;
> +
> +  for (i = 0; auxv[i].a_type != AT_NULL; i++)
> +    if (auxv[i].a_type == AT_HWCAP2) {
> +      if (!(auxv[i].a_un.a_val & PPC_FEATURE2_ARCH_2_07))
> +        return 1;
> +      break;
> +    }
> +
> +  test_atomic_sequences ();
> +  return 0;
> +}

Since we've separated testing of these new instructions from the older 
ones, dropped the power8 compiler switch and are not expecting SIGILL 
anymore, do we still need a runtime check here?

Checking the auxv is also Linux-specific and won't work for bare-metal.

I think letting the test give a compilation error if the compiler 
doesn't support the instructions is fine and also an indication the test 
shouldn't run.

If the compiler does support generating such instructions and the target 
itself doesn't support them, we will have a problem. But it would be up 
to whoever is building the program to pass the correct switches to the 
compiler. In any case, this can be handled in the future if this 
situation arises, right?

So dropping this runtime check if fine with me.

> diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.exp b/gdb/testsuite/gdb.arch/power8-atomic-inst.exp
> new file mode 100644
> index 0000000..9e13310
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.exp

Same thing about the naming.

> @@ -0,0 +1,97 @@
> +# Copyright 2017 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> +#
> +# This file is part of the gdb testsuite.
> +
> +# Test single stepping through atomic sequences beginning with
> +# a lbarx/lharx/lqarx instruction and ending with a stbcx/sthcx/stqxc
> +# instruction.
> +
> +
> +if {![istarget "powerpc*"] || ![is_lp64_target]} {
> +    verbose "Skipping testing of powerpc64 single stepping over atomic sequences."
> +    return
> +}
> +
> +standard_testfile  .c .S
> +
> +if { [prepare_for_testing "failed to prepare" $testfile "$srcfile $srcfile2" \
> +      {debug quiet}] } {
> +    return -1
> +}
> +
> +# The test proper.  DISPLACED is true if we should try with displaced
> +# stepping.

Missing newline between comment and proc.

> +proc do_test { displaced } {
> +    global decimal hex
> +    global gdb_prompt inferior_exited_re srcfile srcfile2
> +
> +    if ![runto_main] then {
> +	untested "could not run to main"
> +	return -1
> +    }
> +
> +    gdb_test_no_output "set displaced-stepping $displaced"
> +
> +    gdb_breakpoint "test_atomic_sequences" "Breakpoint $decimal at $hex" \
> +	"set the breakpoint at the start of the test function"
> +
> +    gdb_test_multiple "continue" "Continue until lbarx/stbcx start breakpoint" {
> +      -re "$inferior_exited_re with code 01.\[\r\n\]+$gdb_prompt $" {
> +	unsupported "POWER8 atomic instructions not supported."

POWER8 or ISA 2.07 instructions? This goes back to the naming suggesting 
only POWER8 supports them.

Re: [PATCH v2] [ppc64] Add POWER8 atomic sequences single-stepping support

[Edjunior Barbosa Machado]

Hi Luis,
thanks for the review once again. Just few doubts below.

On 02/15/2017 08:00 AM, Luis Machado wrote:
> On 02/13/2017 06:47 PM, Edjunior Barbosa Machado wrote:
>> diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.S
>> b/gdb/testsuite/gdb.arch/power8-atomic-inst.S
>> new file mode 100644
>> index 0000000..daa3337
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.S
> 
> I don't know if there are other powerpc initiatives out there other than
> IBM's power 8/9 that are using these instructions. If there are,
> renaming power8 to something generic would be best. Otherwise i don't
> see a problem with leaving this and fixing it in the future if some
> other manufacturer shows up using ISA 2.06/2.07.
> 
> I thought i'd mention it though.


I'm also not aware of other initiatives that implement these
instructions. This name was more inspired on others testcases from gas
focused on these POWER8/ISA 2.07 instructions like
gas/testsuite/gas/ppc/power8.*.  Any suggestion about what would be a
better name here?

> 
>> diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.c
>> b/gdb/testsuite/gdb.arch/power8-atomic-inst.c
>> new file mode 100644
>> index 0000000..535e057
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.c
> 
> Same as above about mentioning power8 in the filename.
> 
>> @@ -0,0 +1,42 @@
>> +/* Copyright 2017 Free Software Foundation, Inc.
>> +
>> +   This file is part of GDB.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see
>> <http://www.gnu.org/licenses/>.  */
>> +
>> +#include <elf.h>
>> +
>> +typedef Elf64_auxv_t auxv_t;
>> +
>> +#ifndef PPC_FEATURE2_ARCH_2_07
>> +#define PPC_FEATURE2_ARCH_2_07    0x80000000
>> +#endif
>> +
>> +extern void test_atomic_sequences (void);
>> +
>> +int
>> +main (int argc, char *argv[], char *envp[], auxv_t auxv[])
>> +{
>> +  int i;
>> +
>> +  for (i = 0; auxv[i].a_type != AT_NULL; i++)
>> +    if (auxv[i].a_type == AT_HWCAP2) {
>> +      if (!(auxv[i].a_un.a_val & PPC_FEATURE2_ARCH_2_07))
>> +        return 1;
>> +      break;
>> +    }
>> +
>> +  test_atomic_sequences ();
>> +  return 0;
>> +}
> 
> Since we've separated testing of these new instructions from the older
> ones, dropped the power8 compiler switch and are not expecting SIGILL
> anymore, do we still need a runtime check here?
> 
> Checking the auxv is also Linux-specific and won't work for bare-metal.
> 
> I think letting the test give a compilation error if the compiler
> doesn't support the instructions is fine and also an indication the test
> shouldn't run.
> 
> If the compiler does support generating such instructions and the target
> itself doesn't support them, we will have a problem. But it would be up
> to whoever is building the program to pass the correct switches to the
> compiler. In any case, this can be handled in the future if this
> situation arises, right?
> 


Actually this is a problem I'm already facing when testing more recent
compilers on POWER7 machines for example. It builds OK but fails with
SIGILL when running (that's why I initially tried expecting for SIGILL),
then switched to this runtime check. Do you have any suggestion about
what would be the best strategy that would work for ppc64 bare-metal too?


Thanks,
--
Edjunior

Re: [PATCH v2] [ppc64] Add POWER8 atomic sequences single-stepping support

[Luis Machado]

On 02/15/2017 06:00 AM, Edjunior Barbosa Machado wrote:
> Hi Luis,
> thanks for the review once again. Just few doubts below.
>
> On 02/15/2017 08:00 AM, Luis Machado wrote:
>> On 02/13/2017 06:47 PM, Edjunior Barbosa Machado wrote:
>>> diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.S
>>> b/gdb/testsuite/gdb.arch/power8-atomic-inst.S
>>> new file mode 100644
>>> index 0000000..daa3337
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.S
>>
>> I don't know if there are other powerpc initiatives out there other than
>> IBM's power 8/9 that are using these instructions. If there are,
>> renaming power8 to something generic would be best. Otherwise i don't
>> see a problem with leaving this and fixing it in the future if some
>> other manufacturer shows up using ISA 2.06/2.07.
>>
>> I thought i'd mention it though.
>
>
> I'm also not aware of other initiatives that implement these
> instructions. This name was more inspired on others testcases from gas
> focused on these POWER8/ISA 2.07 instructions like
> gas/testsuite/gas/ppc/power8.*.  Any suggestion about what would be a
> better name here?
>

I don't have anything off the top of my head. Only ppc-atomic-inst2, 
which is probably not a great name either.

>>
>>> diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.c
>>> b/gdb/testsuite/gdb.arch/power8-atomic-inst.c
>>> new file mode 100644
>>> index 0000000..535e057
>>> --- /dev/null
>>> +++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.c
>>
>> Same as above about mentioning power8 in the filename.
>>
>>> @@ -0,0 +1,42 @@
>>> +/* Copyright 2017 Free Software Foundation, Inc.
>>> +
>>> +   This file is part of GDB.
>>> +
>>> +   This program is free software; you can redistribute it and/or modify
>>> +   it under the terms of the GNU General Public License as published by
>>> +   the Free Software Foundation; either version 3 of the License, or
>>> +   (at your option) any later version.
>>> +
>>> +   This program is distributed in the hope that it will be useful,
>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +   GNU General Public License for more details.
>>> +
>>> +   You should have received a copy of the GNU General Public License
>>> +   along with this program.  If not, see
>>> <http://www.gnu.org/licenses/>.  */
>>> +
>>> +#include <elf.h>
>>> +
>>> +typedef Elf64_auxv_t auxv_t;
>>> +
>>> +#ifndef PPC_FEATURE2_ARCH_2_07
>>> +#define PPC_FEATURE2_ARCH_2_07    0x80000000
>>> +#endif
>>> +
>>> +extern void test_atomic_sequences (void);
>>> +
>>> +int
>>> +main (int argc, char *argv[], char *envp[], auxv_t auxv[])
>>> +{
>>> +  int i;
>>> +
>>> +  for (i = 0; auxv[i].a_type != AT_NULL; i++)
>>> +    if (auxv[i].a_type == AT_HWCAP2) {
>>> +      if (!(auxv[i].a_un.a_val & PPC_FEATURE2_ARCH_2_07))
>>> +        return 1;
>>> +      break;
>>> +    }
>>> +
>>> +  test_atomic_sequences ();
>>> +  return 0;
>>> +}
>>
>> Since we've separated testing of these new instructions from the older
>> ones, dropped the power8 compiler switch and are not expecting SIGILL
>> anymore, do we still need a runtime check here?
>>
>> Checking the auxv is also Linux-specific and won't work for bare-metal.
>>
>> I think letting the test give a compilation error if the compiler
>> doesn't support the instructions is fine and also an indication the test
>> shouldn't run.
>>
>> If the compiler does support generating such instructions and the target
>> itself doesn't support them, we will have a problem. But it would be up
>> to whoever is building the program to pass the correct switches to the
>> compiler. In any case, this can be handled in the future if this
>> situation arises, right?
>>
>
>
> Actually this is a problem I'm already facing when testing more recent
> compilers on POWER7 machines for example. It builds OK but fails with
> SIGILL when running (that's why I initially tried expecting for SIGILL),
> then switched to this runtime check. Do you have any suggestion about
> what would be the best strategy that would work for ppc64 bare-metal too?

Oh, i see. Well, i think we need the runtime check for now then. And we 
can handle bare-metal (if such a target is available in the future) later?

Re: [PATCH] [trivial] Fix test names starting with uppercase in gdb.arch/ppc64-atomic-inst.exp

[Ulrich Weigand]

Luis Machado wrote:
> On 02/13/2017 09:35 PM, Edjunior Barbosa Machado wrote:
> > Hi,
> > this trivial patch fixes the test names starting with uppercase using gdb_test
> > in gdb.arch/ppc64-atomic-inst.exp.
> >
> > Thanks,
> > --
> > Edjunior
> >
> > gdb/testsuite/
> > 2017-02-14  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
> >
> > 	* gdb.arch/ppc64-atomic-inst.exp: Fix test names starting with
> > 	uppercase.
> 
> LGTM.
> 

Thanks for the review, Luis!   I agree, this patch is OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

[PATCH v3] [ppc64] Add POWER8/ISA 2.07 atomic sequences single-stepping support

[Edjunior Barbosa Machado]

On 02/15/2017 10:13 AM, Luis Machado wrote:
> On 02/15/2017 06:00 AM, Edjunior Barbosa Machado wrote:
>> Hi Luis,
>> thanks for the review once again. Just few doubts below.
>>
>> On 02/15/2017 08:00 AM, Luis Machado wrote:
>>> On 02/13/2017 06:47 PM, Edjunior Barbosa Machado wrote:
>>>> diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.S
>>>> b/gdb/testsuite/gdb.arch/power8-atomic-inst.S
>>>> new file mode 100644
>>>> index 0000000..daa3337
>>>> --- /dev/null
>>>> +++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.S
>>>
>>> I don't know if there are other powerpc initiatives out there other than
>>> IBM's power 8/9 that are using these instructions. If there are,
>>> renaming power8 to something generic would be best. Otherwise i don't
>>> see a problem with leaving this and fixing it in the future if some
>>> other manufacturer shows up using ISA 2.06/2.07.
>>>
>>> I thought i'd mention it though.
>>
>>
>> I'm also not aware of other initiatives that implement these
>> instructions. This name was more inspired on others testcases from gas
>> focused on these POWER8/ISA 2.07 instructions like
>> gas/testsuite/gas/ppc/power8.*.  Any suggestion about what would be a
>> better name here?
>>
> 
> I don't have anything off the top of my head. Only ppc-atomic-inst2,
> which is probably not a great name either.

What about gdb.arch/ppc64-isa207-atomic-inst.*?

> 
>>>
>>>> diff --git a/gdb/testsuite/gdb.arch/power8-atomic-inst.c
>>>> b/gdb/testsuite/gdb.arch/power8-atomic-inst.c
>>>> new file mode 100644
>>>> index 0000000..535e057
>>>> --- /dev/null
>>>> +++ b/gdb/testsuite/gdb.arch/power8-atomic-inst.c
>>>
>>> Same as above about mentioning power8 in the filename.
>>>
>>>> @@ -0,0 +1,42 @@
>>>> +/* Copyright 2017 Free Software Foundation, Inc.
>>>> +
>>>> +   This file is part of GDB.
>>>> +
>>>> +   This program is free software; you can redistribute it and/or
>>>> modify
>>>> +   it under the terms of the GNU General Public License as
>>>> published by
>>>> +   the Free Software Foundation; either version 3 of the License, or
>>>> +   (at your option) any later version.
>>>> +
>>>> +   This program is distributed in the hope that it will be useful,
>>>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> +   GNU General Public License for more details.
>>>> +
>>>> +   You should have received a copy of the GNU General Public License
>>>> +   along with this program.  If not, see
>>>> <http://www.gnu.org/licenses/>.  */
>>>> +
>>>> +#include <elf.h>
>>>> +
>>>> +typedef Elf64_auxv_t auxv_t;
>>>> +
>>>> +#ifndef PPC_FEATURE2_ARCH_2_07
>>>> +#define PPC_FEATURE2_ARCH_2_07    0x80000000
>>>> +#endif
>>>> +
>>>> +extern void test_atomic_sequences (void);
>>>> +
>>>> +int
>>>> +main (int argc, char *argv[], char *envp[], auxv_t auxv[])
>>>> +{
>>>> +  int i;
>>>> +
>>>> +  for (i = 0; auxv[i].a_type != AT_NULL; i++)
>>>> +    if (auxv[i].a_type == AT_HWCAP2) {
>>>> +      if (!(auxv[i].a_un.a_val & PPC_FEATURE2_ARCH_2_07))
>>>> +        return 1;
>>>> +      break;
>>>> +    }
>>>> +
>>>> +  test_atomic_sequences ();
>>>> +  return 0;
>>>> +}
>>>
>>> Since we've separated testing of these new instructions from the older
>>> ones, dropped the power8 compiler switch and are not expecting SIGILL
>>> anymore, do we still need a runtime check here?
>>>
>>> Checking the auxv is also Linux-specific and won't work for bare-metal.
>>>
>>> I think letting the test give a compilation error if the compiler
>>> doesn't support the instructions is fine and also an indication the test
>>> shouldn't run.
>>>
>>> If the compiler does support generating such instructions and the target
>>> itself doesn't support them, we will have a problem. But it would be up
>>> to whoever is building the program to pass the correct switches to the
>>> compiler. In any case, this can be handled in the future if this
>>> situation arises, right?
>>>
>>
>>
>> Actually this is a problem I'm already facing when testing more recent
>> compilers on POWER7 machines for example. It builds OK but fails with
>> SIGILL when running (that's why I initially tried expecting for SIGILL),
>> then switched to this runtime check. Do you have any suggestion about
>> what would be the best strategy that would work for ppc64 bare-metal too?
> 
> Oh, i see. Well, i think we need the runtime check for now then. And we
> can handle bare-metal (if such a target is available in the future) later?
> 

Thus, I'm keeping the runtime check using HWCAP2 bits for now.

Please let me know if there are any additional fixes to do.

Thanks,
--
Edjunior

gdb/
2017-02-16  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-16  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>

	* gdb.arch/ppc64-isa207-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/ppc64-isa207-atomic-inst.S: New file.
	* gdb.arch/ppc64-isa207-atomic-inst.c: Likewise.

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

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 527f643..72ee05d 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -983,12 +983,33 @@ typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
 
 /* Instruction masks used during single-stepping of atomic
    sequences.  */
-#define LWARX_MASK 0xfc0007fe
+#define LOAD_AND_RESERVE_MASK 0xfc0007fe
 #define LWARX_INSTRUCTION 0x7c000028
 #define LDARX_INSTRUCTION 0x7c0000A8
-#define STWCX_MASK 0xfc0007ff
+#define LBARX_INSTRUCTION 0x7c000068
+#define LHARX_INSTRUCTION 0x7c0000e8
+#define LQARX_INSTRUCTION 0x7c000228
+#define STORE_CONDITIONAL_MASK 0xfc0007ff
 #define STWCX_INSTRUCTION 0x7c00012d
 #define STDCX_INSTRUCTION 0x7c0001ad
+#define STBCX_INSTRUCTION 0x7c00056d
+#define STHCX_INSTRUCTION 0x7c0005ad
+#define STQCX_INSTRUCTION 0x7c00016d
+
+/* Check if insn is one of the Load And Reserve instructions used for atomic
+   sequences.  */
+#define IS_LOAD_AND_RESERVE_INSN(insn)	((insn & LOAD_AND_RESERVE_MASK) == LWARX_INSTRUCTION \
+					 || (insn & LOAD_AND_RESERVE_MASK) == LDARX_INSTRUCTION \
+					 || (insn & LOAD_AND_RESERVE_MASK) == LBARX_INSTRUCTION \
+					 || (insn & LOAD_AND_RESERVE_MASK) == LHARX_INSTRUCTION \
+					 || (insn & LOAD_AND_RESERVE_MASK) == LQARX_INSTRUCTION)
+/* Check if insn is one of the Store Conditional instructions used for atomic
+   sequences.  */
+#define IS_STORE_CONDITIONAL_INSN(insn)	((insn & STORE_CONDITIONAL_MASK) == STWCX_INSTRUCTION \
+					 || (insn & STORE_CONDITIONAL_MASK) == STDCX_INSTRUCTION \
+					 || (insn & STORE_CONDITIONAL_MASK) == STBCX_INSTRUCTION \
+					 || (insn & STORE_CONDITIONAL_MASK) == STHCX_INSTRUCTION \
+					 || (insn & STORE_CONDITIONAL_MASK) == STQCX_INSTRUCTION)
 
 /* We can't displaced step atomic sequences.  Otherwise this is just
    like simple_displaced_step_copy_insn.  */
@@ -1008,9 +1029,8 @@ ppc_displaced_step_copy_insn (struct gdbarch *gdbarch,
 
   insn = extract_signed_integer (buf, PPC_INSN_SIZE, byte_order);
 
-  /* Assume all atomic sequences start with a lwarx/ldarx instruction.  */
-  if ((insn & LWARX_MASK) == LWARX_INSTRUCTION
-      || (insn & LWARX_MASK) == LDARX_INSTRUCTION)
+  /* Assume all atomic sequences start with a Load and Reserve instruction.  */
+  if (IS_LOAD_AND_RESERVE_INSN (insn))
     {
       if (debug_displaced)
 	{
@@ -1138,11 +1158,10 @@ ppc_displaced_step_hw_singlestep (struct gdbarch *gdbarch,
   return 1;
 }
 
-/* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
-   instruction and ending with a STWCX/STDCX instruction.  If such a sequence
-   is found, attempt to step through it.  A breakpoint is placed at the end of 
-   the sequence.  */
-
+/* Checks for an atomic sequence of instructions beginning with a
+   Load And Reserve instruction and ending with a Store Conditional
+   instruction.  If such a sequence is found, attempt to step through it.
+   A breakpoint is placed at the end of the sequence.  */
 VEC (CORE_ADDR) *
 ppc_deal_with_atomic_sequence (struct regcache *regcache)
 {
@@ -1160,9 +1179,8 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
   int bc_insn_count = 0; /* Conditional branch instruction count.  */
   VEC (CORE_ADDR) *next_pcs = NULL;
 
-  /* Assume all atomic sequences start with a lwarx/ldarx instruction.  */
-  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
-      && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
+  /* Assume all atomic sequences start with a Load And Reserve instruction.  */
+  if (!IS_LOAD_AND_RESERVE_INSN (insn))
     return NULL;
 
   /* Assume that no atomic sequence is longer than "atomic_sequence_length" 
@@ -1193,14 +1211,13 @@ ppc_deal_with_atomic_sequence (struct regcache *regcache)
 	  last_breakpoint++;
         }
 
-      if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
-          || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+      if (IS_STORE_CONDITIONAL_INSN (insn))
         break;
     }
 
-  /* Assume that the atomic sequence ends with a stwcx/stdcx instruction.  */
-  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
-      && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
+  /* Assume that the atomic sequence ends with a Store Conditional
+     instruction.  */
+  if (!IS_STORE_CONDITIONAL_INSN (insn))
     return NULL;
 
   closing_insn = loc;
diff --git a/gdb/testsuite/gdb.arch/ppc64-isa207-atomic-inst.S b/gdb/testsuite/gdb.arch/ppc64-isa207-atomic-inst.S
new file mode 100644
index 0000000..daa3337
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/ppc64-isa207-atomic-inst.S
@@ -0,0 +1,100 @@
+/* This file is part of GDB, the GNU debugger.
+
+   Copyright 2017 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+	.align 2
+	.globl test_atomic_sequences
+#if _CALL_ELF == 2
+	.type test_atomic_sequences,@function
+test_atomic_sequences:
+#else
+	.section ".opd","aw"
+	.align 3
+test_atomic_sequences:
+	.quad .test_atomic_sequences,.TOC.@tocbase,0
+	.size test_atomic_sequences,.-test_atomic_sequences
+	.previous
+	.globl .test_atomic_sequences
+	.type .test_atomic_sequences,@function
+.test_atomic_sequences:
+#endif
+
+	li	0,0
+	addi	4,1,-8
+
+	stb	0,0(4)
+1:	lbarx	5,0,4
+	cmpdi	5,0
+	bne	2f
+	addi	5,5,1
+	stbcx.	5,0,4
+	bne	1b
+
+	sth	0,0(4)
+2:	lharx	5,0,4
+	cmpdi	5,0
+	bne	3f
+	addi	5,5,1
+	sthcx.	5,0,4
+	bne	2b
+
+#ifdef	__BIG_ENDIAN__
+	li 10,0
+	li 6,0
+	li 7,1
+	std 10,-16(1)
+	li 10,1
+	std 10,-8(1)
+	addi 4,1,-16
+#else
+	std 9,40(1)
+	li 9,1
+	addi 4,1,32
+	std 9,32(1)
+	mr 8,9
+	ld 3,8(4)
+#endif
+3:	lqarx 10,0,4
+#ifdef	__BIG_ENDIAN__
+	li 8,0
+	li 9,2
+	mr 5,10
+	xor 10,11,7
+	xor 5,5,6
+	or. 4,5,10
+	bne 4f
+	addi 10,1,-16
+	stqcx. 8,0,10
+#else
+	xor 9,11,8
+	mr 6,11
+	xor 11,10,3
+	or. 0,9,11
+	bne 4f
+	li 14,0
+	li 15,2
+	stqcx. 14,0,4
+#endif
+	bne 3b
+
+4:	li	3,0
+	blr
+
+#if _CALL_ELF == 2
+	.size test_atomic_sequences,.-test_atomic_sequences
+#else
+	.size .test_atomic_sequences,.-.test_atomic_sequences
+#endif
diff --git a/gdb/testsuite/gdb.arch/ppc64-isa207-atomic-inst.c b/gdb/testsuite/gdb.arch/ppc64-isa207-atomic-inst.c
new file mode 100644
index 0000000..535e057
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/ppc64-isa207-atomic-inst.c
@@ -0,0 +1,42 @@
+/* Copyright 2017 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <elf.h>
+
+typedef Elf64_auxv_t auxv_t;
+
+#ifndef PPC_FEATURE2_ARCH_2_07
+#define PPC_FEATURE2_ARCH_2_07	0x80000000
+#endif
+
+extern void test_atomic_sequences (void);
+
+int
+main (int argc, char *argv[], char *envp[], auxv_t auxv[])
+{
+  int i;
+
+  for (i = 0; auxv[i].a_type != AT_NULL; i++)
+    if (auxv[i].a_type == AT_HWCAP2) {
+      if (!(auxv[i].a_un.a_val & PPC_FEATURE2_ARCH_2_07))
+        return 1;
+      break;
+    }
+
+  test_atomic_sequences ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/ppc64-isa207-atomic-inst.exp b/gdb/testsuite/gdb.arch/ppc64-isa207-atomic-inst.exp
new file mode 100644
index 0000000..2b4c8ad
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/ppc64-isa207-atomic-inst.exp
@@ -0,0 +1,99 @@
+# Copyright 2017 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+#
+# This file is part of the gdb testsuite.
+
+# Test single stepping through POWER8/ISA 2.07 atomic sequences beginning with
+# a lbarx/lharx/lqarx instruction and ending with a stbcx/sthcx/stqxc
+# instruction.  Note that although lbarx, lharx, stbcx and sthcx instructions
+# were introduced in ISA 2.06, they were implemented only in POWER8 (ISA 2.07).
+
+
+if {![istarget "powerpc*"] || ![is_lp64_target]} {
+    verbose "Skipping testing of powerpc64 single stepping over atomic sequences."
+    return
+}
+
+standard_testfile  .c .S
+
+if { [prepare_for_testing "failed to prepare" $testfile "$srcfile $srcfile2" \
+      {debug quiet}] } {
+    return -1
+}
+
+# The test proper.  DISPLACED is true if we should try with displaced
+# stepping.
+
+proc do_test { displaced } {
+    global decimal hex
+    global gdb_prompt inferior_exited_re srcfile srcfile2
+
+    if ![runto_main] then {
+	untested "could not run to main"
+	return -1
+    }
+
+    gdb_test_no_output "set displaced-stepping $displaced"
+
+    gdb_breakpoint "test_atomic_sequences" "Breakpoint $decimal at $hex" \
+	"set the breakpoint at the start of the test function"
+
+    gdb_test_multiple "continue" "Continue until lbarx/stbcx start breakpoint" {
+      -re "$inferior_exited_re with code 01.\[\r\n\]+$gdb_prompt $" {
+	unsupported "POWER8/ISA 2.07 atomic instructions not supported."
+	return -1
+      }
+      -re "Continuing.*Breakpoint $decimal.*$gdb_prompt $" {
+	pass "continue until test_atomic_sequences function"
+      }
+    }
+
+    set bp1 [gdb_get_line_number "lbarx" "$srcfile2"]
+    gdb_breakpoint "$bp1" "Breakpoint $decimal at $hex" \
+	"set the breakpoint at the start of the lbarx/stbcx sequence"
+
+    set bp2 [gdb_get_line_number "lharx" "$srcfile2"]
+    gdb_breakpoint "$bp2" "Breakpoint $decimal at $hex" \
+	"set the breakpoint at the start of the lharx/sthcx sequence"
+
+    set bp3 [gdb_get_line_number "lqarx" "$srcfile2"]
+    gdb_breakpoint "$bp3" "Breakpoint $decimal at $hex" \
+	"set the breakpoint at the start of the lqarx/stqcx sequence"
+
+    gdb_test continue "Continuing.*Breakpoint $decimal.*" \
+	"continue until lbarx/stbcx start breakpoint"
+
+    gdb_test nexti "bne.*1b" \
+	"step through the lbarx/stbcx sequence"
+
+    gdb_test continue "Continuing.*Breakpoint $decimal.*" \
+	"continue until lharx/sthcx start breakpoint"
+
+    gdb_test nexti "bne.*2b" \
+	"step through the lharx/sthcx sequence"
+
+    gdb_test continue "Continuing.*Breakpoint $decimal.*" \
+	"continue until ldqrx/stqcx start breakpoint"
+
+    gdb_test nexti "bne.*3b" \
+	"step through the lqarx/stqcx sequence"
+}
+
+foreach displaced { "off" "on" } {
+    with_test_prefix "displaced=$displaced" {
+	do_test $displaced
+    }
+}
-- 
2.9.3

Re: [PATCH v3] [ppc64] Add POWER8/ISA 2.07 atomic sequences single-stepping support

[Luis Machado]

On 02/16/2017 05:42 PM, Edjunior Barbosa Machado wrote:
> What about gdb.arch/ppc64-isa207-atomic-inst.*?
>

Could be. I don't have a better suggestion. Maybe Ulrich has.

> Thus, I'm keeping the runtime check using HWCAP2 bits for now.
>
> Please let me know if there are any additional fixes to do.

Just a nit.

> diff --git a/gdb/testsuite/gdb.arch/ppc64-isa207-atomic-inst.exp b/gdb/testsuite/gdb.arch/ppc64-isa207-atomic-inst.exp
> new file mode 100644
> index 0000000..2b4c8ad
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/ppc64-isa207-atomic-inst.exp

...

> +if {![istarget "powerpc*"] || ![is_lp64_target]} {
> +    verbose "Skipping testing of powerpc64 single stepping over atomic sequences."

untested "skipping powerpc isa 207 atomic sequences test"?

Otherwise i have no further comments.

Thanks,
Luis

Re: [PATCH v3] [ppc64] Add POWER8/ISA 2.07 atomic sequences single-stepping support

[Ulrich Weigand]

Luis Machado wrote:
> On 02/16/2017 05:42 PM, Edjunior Barbosa Machado wrote:
> > What about gdb.arch/ppc64-isa207-atomic-inst.*?
> >
> 
> Could be. I don't have a better suggestion. Maybe Ulrich has.

Not really ... the name looks good to me.

> > +if {![istarget "powerpc*"] || ![is_lp64_target]} {
> > +    verbose "Skipping testing of powerpc64 single stepping over atomic sequences."
> 
> untested "skipping powerpc isa 207 atomic sequences test"?

Agreed, makes sense.

> Otherwise i have no further comments.

Thanks for the review, Luis!

Edjunior, the patch is OK with the above change.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] [trivial] Fix test names starting with uppercase in gdb.arch/ppc64-atomic-inst.exp

[Edjunior Barbosa Machado]

On 02/15/2017 10:59 AM, Ulrich Weigand wrote:
> Luis Machado wrote:
>> On 02/13/2017 09:35 PM, Edjunior Barbosa Machado wrote:
>>> gdb/testsuite/
>>> 2017-02-14  Edjunior Barbosa Machado  <emachado@linux.vnet.ibm.com>
>>>
>>> 	* gdb.arch/ppc64-atomic-inst.exp: Fix test names starting with
>>> 	uppercase.
>>
>> LGTM.
>>
> 
> Thanks for the review, Luis!   I agree, this patch is OK.
> 
> Thanks,
> Ulrich
> 

Just checked in:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a51d7ecf3ddd64e0aec68e3c30913faba680b2cb

Thanks,
Edjunior

Re: [PATCH v3] [ppc64] Add POWER8/ISA 2.07 atomic sequences single-stepping support

[Edjunior Barbosa Machado]

On 02/21/2017 07:55 AM, Ulrich Weigand wrote:
> Luis Machado wrote:
>> On 02/16/2017 05:42 PM, Edjunior Barbosa Machado wrote:
>>> What about gdb.arch/ppc64-isa207-atomic-inst.*?
>>>
>>
>> Could be. I don't have a better suggestion. Maybe Ulrich has.
> 
> Not really ... the name looks good to me.
> 
>>> +if {![istarget "powerpc*"] || ![is_lp64_target]} {
>>> +    verbose "Skipping testing of powerpc64 single stepping over atomic sequences."
>>
>> untested "skipping powerpc isa 207 atomic sequences test"?
> 
> Agreed, makes sense.
> 
>> Otherwise i have no further comments.
> 
> Thanks for the review, Luis!
> 
> Edjunior, the patch is OK with the above change.
> 
> Thanks,
> Ulrich
> 

Thank you folks for the review! Just checked in with the change Luis suggested:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=2039d74e780db6659c87cd3c426d526615cfe703

--
Edjunior