diff mbox

[PATCH/RFC] Refactor gdb.reverse/insn-reverse.c

Message ID 0f3b832e-3255-9c02-2aeb-90014865f557@codesourcery.com
State New
Headers show

Commit Message

Luis Machado Jan. 26, 2017, 4:39 p.m. UTC
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

Comments

Luis Machado Jan. 26, 2017, 4:54 p.m. UTC | #1
On 01/26/2017 10:39 AM, Luis Machado wrote:
> 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

Forgot to effectively add the files. I'll fix this in a bit.
diff mbox

Patch

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