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

Message ID 9a226055-7485-eceb-482d-f1b9e7391ecb@linux.vnet.ibm.com
State New, archived
Headers

Commit Message

Edjunior Barbosa Machado Feb. 16, 2017, 11:42 p.m. UTC
  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
  

Comments

Luis Machado Feb. 20, 2017, 7:52 p.m. UTC | #1
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
  

Patch

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