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

[Luis Machado <lgustavo@codesourcery.com>]

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" } {
>