This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH/RFC] Refactor gdb.reverse/insn-reverse.c
On 01/25/2017 04:28 PM, Yao Qi wrote:
On 17-01-25 12:11:01, Luis Machado wrote:
That is a reasonable assessment. insn-reverse.[c|exp] is redundant and IMO
would benefit from renaming too.
The support in "insn-support-<arch>.c means support for a set of
instructions for this particular subsystem of gdb, therefore why i went with
that name. Thinking about it further, instruction decoding support is the
basis/foundation of reverse debugging, without which things would not work
properly. But i may be overthinking. :-)
Every test is about testing some sort of support. Breakpoint test is
about breakpoint support, tracepoint test is about tracepoint support.
We don't have to explicitly mention "support" in the test case name,
IMO.
It is easy to relate "insn-reverse-<arch>.c" to "insn-reverse.c".
If you think "reverse" is redundant, "insn.c" and "insn-<arch>.c" is
acceptable to me too.
It is not terribly important. I've reverted to the original name
(insn-reverse-<arch>.c), updated things to mention the new name and
pushed this as 8b00c176168dc7b0d78d0dc1f7d42f915375dc4a.
Patch attached.
Thanks for reviewing,
Luis
Refactor gdb.reverse/insn-reverse.c
Changes in v2:
- Renamed arch-specific files to insn-reverse-<arch>.c.
- Adjusted according to reviews.
This patch prepares things for an upcoming testcase for record/replay support
on x86. As is, gdb.reverse/insn-reverse.c is divided into sections guarded by
a few #if blocks, and right now it only handles arm/aarch64.
If we move forward with requiring more tests for record/replay on different
architectures, i think this has the potential to become cluttered with a lot
of differing arch-specific code in the same file.
I've broken up the main file into other files with arch-specific bits
(insn-reverse-<arch>.c). The main file will hold the generic pieces that will
take care of calling the tests.
The arch-specific c files are then included at the top of the generic c file.
I've also added a generic initialize function since we need to run pre-test
checks on x86 to make sure the rdrand/rdseed instructions are supported,
otherwise we will run into a SIGILL.
The arch-specific files will implement their own initialize function with
whatever makes sense. Right now the aarch64 and arm files have an empty
initialization function.
Does this look reasonable?
gdb/testsuite/ChangeLog:
2017-01-26 Luis Machado <lgustavo@codesourcery.com>
* gdb.reverse/insn-reverse.c: Move arm and aarch64 code to their own
files.
(initialize): New function conditionally defined.
(testcases): Move within conditional block.
(main): Call initialize.
* gdb.reverse/insn-reverse-aarch64.c: New file, based on aarch64 bits
of gdb.reverse/insn-reverse.c.
* gdb.reverse/insn-reverse-arm.c: New file, based on arm bits of
gdb.reverse/insn-reverse.c.
---
gdb/testsuite/gdb.reverse/insn-reverse-aarch64.c | 105 +++++++++++++++++
gdb/testsuite/gdb.reverse/insn-reverse-arm.c | 70 +++++++++++
gdb/testsuite/gdb.reverse/insn-reverse.c | 144 +++--------------------
3 files changed, 194 insertions(+), 125 deletions(-)
create mode 100644 gdb/testsuite/gdb.reverse/insn-reverse-aarch64.c
create mode 100644 gdb/testsuite/gdb.reverse/insn-reverse-arm.c
diff --git a/gdb/testsuite/gdb.reverse/insn-reverse-aarch64.c b/gdb/testsuite/gdb.reverse/insn-reverse-aarch64.c
new file mode 100644
index 0000000..4cb83c4
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/insn-reverse-aarch64.c
@@ -0,0 +1,105 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2015-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/>. */
+
+#include <arm_neon.h>
+
+static void
+load (void)
+{
+ int buf[8];
+
+ asm ("ld1 { v1.8b }, [%[buf]]\n"
+ "ld1 { v2.8b, v3.8b }, [%[buf]]\n"
+ "ld1 { v3.8b, v4.8b, v5.8b }, [%[buf]]\n"
+ :
+ : [buf] "r" (buf)
+ : /* No clobbers */);
+}
+
+static void
+move (void)
+{
+ float32x2_t b1_ = vdup_n_f32(123.0f);
+ float32_t a1_ = 0;
+ float64x1_t b2_ = vdup_n_f64(456.0f);
+ float64_t a2_ = 0;
+
+ asm ("ins %0.s[0], %w1\n"
+ : "=w"(b1_)
+ : "r"(a1_), "0"(b1_)
+ : /* No clobbers */);
+
+ asm ("ins %0.d[1], %x1\n"
+ : "=w"(b2_)
+ : "r"(a2_), "0"(b2_)
+ : /* No clobbers */);
+}
+
+static void
+adv_simd_mod_imm (void)
+{
+ float32x2_t a1 = {2.0, 4.0};
+
+ asm ("bic %0.2s, #1\n"
+ "bic %0.2s, #1, lsl #8\n"
+ : "=w"(a1)
+ : "0"(a1)
+ : /* No clobbers */);
+}
+
+static void
+adv_simd_scalar_index (void)
+{
+ float64x2_t b_ = {0.0, 0.0};
+ float64_t a_ = 1.0;
+ float64_t result;
+
+ asm ("fmla %d0,%d1,%2.d[1]"
+ : "=w"(result)
+ : "w"(a_), "w"(b_)
+ : /* No clobbers */);
+}
+
+static void
+adv_simd_smlal (void)
+{
+ asm ("smlal v13.2d, v8.2s, v0.2s");
+}
+
+static void
+adv_simd_vect_shift (void)
+{
+ asm ("fcvtzs s0, s0, #1");
+}
+
+/* Initialize arch-specific bits. */
+
+static void initialize (void)
+{
+ /* AArch64 doesn't currently use this function. */
+}
+
+/* Functions testing instruction decodings. GDB will test all of these. */
+static testcase_ftype testcases[] =
+{
+ load,
+ move,
+ adv_simd_mod_imm,
+ adv_simd_scalar_index,
+ adv_simd_smlal,
+ adv_simd_vect_shift,
+};
diff --git a/gdb/testsuite/gdb.reverse/insn-reverse-arm.c b/gdb/testsuite/gdb.reverse/insn-reverse-arm.c
new file mode 100644
index 0000000..cd721f6
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/insn-reverse-arm.c
@@ -0,0 +1,70 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2015-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/>. */
+
+static void
+ext_reg_load (void)
+{
+ char in[8];
+
+ asm ("vldr d0, [%0]" : : "r" (in));
+ asm ("vldr s3, [%0]" : : "r" (in));
+
+ asm ("vldm %0, {d3-d4}" : : "r" (in));
+ asm ("vldm %0, {s9-s11}" : : "r" (in));
+}
+
+static void
+ext_reg_mov (void)
+{
+ int i, j;
+ double d;
+
+ i = 1;
+ j = 2;
+
+ asm ("vmov s4, s5, %0, %1" : "=r" (i), "=r" (j): );
+ asm ("vmov s7, s8, %0, %1" : "=r" (i), "=r" (j): );
+ asm ("vmov %0, %1, s10, s11" : : "r" (i), "r" (j));
+ asm ("vmov %0, %1, s1, s2" : : "r" (i), "r" (j));
+
+ asm ("vmov %P2, %0, %1" : "=r" (i), "=r" (j): "w" (d));
+ asm ("vmov %1, %2, %P0" : "=w" (d) : "r" (i), "r" (j));
+}
+
+static void
+ext_reg_push_pop (void)
+{
+ double d;
+
+ asm ("vpush {%P0}" : : "w" (d));
+ asm ("vpop {%P0}" : : "w" (d));
+}
+
+/* Initialize arch-specific bits. */
+
+static void initialize (void)
+{
+ /* ARM doesn't currently use this function. */
+}
+
+/* Functions testing instruction decodings. GDB will test all of these. */
+static testcase_ftype testcases[] =
+{
+ ext_reg_load,
+ ext_reg_mov,
+ ext_reg_push_pop,
+};
diff --git a/gdb/testsuite/gdb.reverse/insn-reverse.c b/gdb/testsuite/gdb.reverse/insn-reverse.c
index f110fcf..662a02b 100644
--- a/gdb/testsuite/gdb.reverse/insn-reverse.c
+++ b/gdb/testsuite/gdb.reverse/insn-reverse.c
@@ -15,141 +15,32 @@
You should have received a copy of the GNU General Public License
along with this program. If not, see <http://www.gnu.org/licenses/>. */
-#if (defined __aarch64__)
-#include <arm_neon.h>
-#endif
-
-#if (defined __aarch64__)
-static void
-load (void)
-{
- int buf[8];
-
- asm ("ld1 { v1.8b }, [%[buf]]\n"
- "ld1 { v2.8b, v3.8b }, [%[buf]]\n"
- "ld1 { v3.8b, v4.8b, v5.8b }, [%[buf]]\n"
- :
- : [buf] "r" (buf)
- : /* No clobbers */);
-}
-
-static void
-move (void)
-{
- float32x2_t b1_ = vdup_n_f32(123.0f);
- float32_t a1_ = 0;
- float64x1_t b2_ = vdup_n_f64(456.0f);
- float64_t a2_ = 0;
-
- asm ("ins %0.s[0], %w1\n"
- : "=w"(b1_)
- : "r"(a1_), "0"(b1_)
- : /* No clobbers */);
-
- asm ("ins %0.d[1], %x1\n"
- : "=w"(b2_)
- : "r"(a2_), "0"(b2_)
- : /* No clobbers */);
-}
-
-static void
-adv_simd_mod_imm (void)
-{
- float32x2_t a1 = {2.0, 4.0};
-
- asm ("bic %0.2s, #1\n"
- "bic %0.2s, #1, lsl #8\n"
- : "=w"(a1)
- : "0"(a1)
- : /* No clobbers */);
-}
-
-static void
-adv_simd_scalar_index (void)
-{
- float64x2_t b_ = {0.0, 0.0};
- float64_t a_ = 1.0;
- float64_t result;
+typedef void (*testcase_ftype) (void);
- asm ("fmla %d0,%d1,%2.d[1]"
- : "=w"(result)
- : "w"(a_), "w"(b_)
- : /* No clobbers */);
-}
+/* The arch-specific files need to implement both the initialize function
+ and define the testcases array. */
-static void
-adv_simd_smlal (void)
-{
- asm ("smlal v13.2d, v8.2s, v0.2s");
-}
-
-static void
-adv_simd_vect_shift (void)
-{
- asm ("fcvtzs s0, s0, #1");
-}
+#if (defined __aarch64__)
+#include "insn-reverse-aarch64.c"
#elif (defined __arm__)
-static void
-ext_reg_load (void)
-{
- char in[8];
-
- asm ("vldr d0, [%0]" : : "r" (in));
- asm ("vldr s3, [%0]" : : "r" (in));
-
- asm ("vldm %0, {d3-d4}" : : "r" (in));
- asm ("vldm %0, {s9-s11}" : : "r" (in));
-}
-
-static void
-ext_reg_mov (void)
-{
- int i, j;
- double d;
+#include "insn-reverse-arm.c"
+#else
+/* We get here if the current architecture being tested doesn't have any
+ record/replay instruction decoding tests implemented. */
+static testcase_ftype testcases[] = {};
- i = 1;
- j = 2;
-
- asm ("vmov s4, s5, %0, %1" : "=r" (i), "=r" (j): );
- asm ("vmov s7, s8, %0, %1" : "=r" (i), "=r" (j): );
- asm ("vmov %0, %1, s10, s11" : : "r" (i), "r" (j));
- asm ("vmov %0, %1, s1, s2" : : "r" (i), "r" (j));
-
- asm ("vmov %P2, %0, %1" : "=r" (i), "=r" (j): "w" (d));
- asm ("vmov %1, %2, %P0" : "=w" (d) : "r" (i), "r" (j));
-}
+/* Dummy implementation in case this target doesn't have any record/replay
+ instruction decoding tests implemented. */
static void
-ext_reg_push_pop (void)
+initialize (void)
{
- double d;
-
- asm ("vpush {%P0}" : : "w" (d));
- asm ("vpop {%P0}" : : "w" (d));
}
#endif
-typedef void (*testcase_ftype) (void);
-
-/* Functions testing instruction decodings. GDB will read n_testcases
- to know how many functions to test. */
-
-static testcase_ftype testcases[] =
-{
-#if (defined __aarch64__)
- load,
- move,
- adv_simd_mod_imm,
- adv_simd_scalar_index,
- adv_simd_smlal,
- adv_simd_vect_shift,
-#elif (defined __arm__)
- ext_reg_load,
- ext_reg_mov,
- ext_reg_push_pop,
-#endif
-};
-
+/* GDB will read n_testcases to know how many functions to test. The
+ functions are implemented in arch-specific files and the testcases
+ array is defined together with them. */
static int n_testcases = (sizeof (testcases) / sizeof (testcase_ftype));
int
@@ -157,6 +48,9 @@ main ()
{
int i = 0;
+ /* Initialize any required arch-specific bits. */
+ initialize ();
+
for (i = 0; i < n_testcases; i++)
testcases[i] ();
--
2.7.4