This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]