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

[Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>]

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