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

[Luis Machado <lgustavo@codesourcery.com>]

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.