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

[Luis Machado <lgustavo@codesourcery.com>]

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.

>>
>>> 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?