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
@@ -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;
new file mode 100644
@@ -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
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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
+ }
+}