Re: [PATCH 00/13] Split brekapoint_from_pc to breakpoint_kind_from_pc and sw_breakpoint_from_kind

Pedro Alves
2016-10-27T17:55:42Z

On 10/27/2016 04:41 PM, Yao Qi wrote:
> Hi Pedro,
> Thanks for the review,
>
> On Thu, Oct 27, 2016 at 3:58 PM, Pedro Alves <palves@redhat.com> wrote:
>>
>> My only question is what happens to the GDBARCH_BREAKPOINT_MANIPULATION
>> / SET_GDBARCH_BREAKPOINT_MANIPULATION macros? I was hoping they'd
>> disappear in the end, but looks like not? (I find the "manipulation"
>> name to be very opaque here, btw.)
>
> Both are the intermediate structure during the the process of conversion.
> I didn't remove them as I think they two can "simplify" the code, because
> a lot of gdbarch have one breakpoint instruction. Their
> breakpoint_kind_from_pc and sw_breakpoint_from_kind look quite similar.
>

I see. Guess we can proceed with what you have, and then maybe replace
the macros with templates? Here's how it might look like. I confirmed that
i386-tdep.o and rs6000-tdep.o build with this, and confirmed that the
x86 version works at least with a version of the patch that kept
the template inside i386-tdep.c only.

Without C++11, we have to remove the "constexpr", and we can't
make the arrays static nor const -- only extern arrays can be
used as non-type parameters. So with C++03 that'd be a little
regression compared to the macros. C++11 wins here too. :-)

Another option would be to replace the two gdbarch methods with
a single one that returns an object that itself has the two
conversion methods. Those methods in the new object would have
to be virtual, so it'd end up being more hops at run time.

From 51cc27113394b07ad1b3cec429973412d88baa7f Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 27 Oct 2016 18:21:44 +0100
Subject: [PATCH] BREAKPOINT_MANIPULATION: macro -> template

---
gdb/arch-utils.h | 80 ++++++++++++++++++++++++++++---------------------------
gdb/i386-tdep.c | 9 ++++---
gdb/rs6000-tdep.c | 12 +++++----
3 files changed, 54 insertions(+), 47 deletions(-)

diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 9592580..4b0848f 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -26,48 +26,50 @@ struct minimal_symbol;
struct type;
struct gdbarch_info;

-#define GDBARCH_BREAKPOINT_MANIPULATION(ARCH,BREAK_INSN) \
- static int \
- ARCH##_breakpoint_kind_from_pc (struct gdbarch *gdbarch, \
- CORE_ADDR *pcptr) \
- { \
- return sizeof (BREAK_INSN); \
- } \
- static const gdb_byte * \
- ARCH##_sw_breakpoint_from_kind (struct gdbarch *gdbarch, \
- int kind, int *size) \
- { \
- *size = kind; \
- return BREAK_INSN; \
+template <size_t bp_size, const gdb_byte *break_insn>
+struct bp_manipulation
+{
+ static int
+ kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr)
+ {
+ return bp_size;
}

-#define SET_GDBARCH_BREAKPOINT_MANIPULATION(ARCH) \
- set_gdbarch_breakpoint_kind_from_pc (gdbarch, \
- ARCH##_breakpoint_kind_from_pc); \
- set_gdbarch_sw_breakpoint_from_kind (gdbarch, \
- ARCH##_sw_breakpoint_from_kind)
-
-#define GDBARCH_BREAKPOINT_MANIPULATION_ENDIAN(ARCH, \
- LITTLE_BREAK_INSN, \
- BIG_BREAK_INSN) \
- static int \
- ARCH##_breakpoint_kind_from_pc (struct gdbarch *gdbarch, \
- CORE_ADDR *pcptr) \
- { \
- gdb_static_assert (ARRAY_SIZE (LITTLE_BREAK_INSN) \
- == ARRAY_SIZE (BIG_BREAK_INSN)); \
- return sizeof (BIG_BREAK_INSN); \
- } \
- static const gdb_byte * \
- ARCH##_sw_breakpoint_from_kind (struct gdbarch *gdbarch, \
- int kind, int *size) \
- { \
- *size = kind; \
- if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG) \
- return BIG_BREAK_INSN; \
- else \
- return LITTLE_BREAK_INSN; \
+ static const gdb_byte *
+ bp_from_kind (struct gdbarch *gdbarch, int kind, int *size)
+ {
+ *size = kind;
+ return break_insn;
}
+};
+
+template <size_t bp_size,
+ const gdb_byte *break_insn_little,
+ const gdb_byte *break_insn_big>
+struct bp_manipulation_endian
+{
+ static int
+ kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr)
+ {
+ return bp_size;
+ }
+
+ static const gdb_byte *
+ bp_from_kind (struct gdbarch *gdbarch, int kind, int *size)
+ {
+ *size = kind;
+ if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
+ return break_insn_big;
+ else
+ return break_insn_little;
+ }
+};
+
+#define BP_MANIPULATION(BREAK_INSN) \
+ bp_manipulation<sizeof (BREAK_INSN), BREAK_INSN>
+
+#define BP_MANIPULATION_ENDIAN(BREAK_INSN_LITTLE, BREAK_INSN_BIG) \
+ bp_manipulation_endian<sizeof (BREAK_INSN_LITTLE), BREAK_INSN_LITTLE, BREAK_INSN_BIG>

/* An implementation of gdbarch_displaced_step_copy_insn for
processors that don't need to modify the instruction before
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 697edc6..a1e078b 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -588,9 +588,10 @@ static const char *disassembly_flavor = att_flavor;

This function is 64-bit safe. */

-static const gdb_byte break_insn[] = { 0xcc }; /* int 3 */
+constexpr gdb_byte i386_break_insn[] = { 0xcc }; /* int 3 */
+
+typedef BP_MANIPULATION (i386_break_insn) i386_breakpoint;

-GDBARCH_BREAKPOINT_MANIPULATION (i386, break_insn)
\f
/* Displaced instruction handling. */

@@ -8453,7 +8454,9 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
/* Stack grows downward. */
set_gdbarch_inner_than (gdbarch, core_addr_lessthan);

- SET_GDBARCH_BREAKPOINT_MANIPULATION (i386);
+ set_gdbarch_breakpoint_kind_from_pc (gdbarch, i386_breakpoint::kind_from_pc);
+ set_gdbarch_sw_breakpoint_from_kind (gdbarch, i386_breakpoint::bp_from_kind);
+
set_gdbarch_decr_pc_after_break (gdbarch, 1);
set_gdbarch_max_insn_length (gdbarch, I386_MAX_INSN_LEN);

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 7cb7a9e..5578b33 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -967,11 +967,11 @@ rs6000_fetch_pointer_argument (struct frame_info *frame, int argi,

/* Sequence of bytes for breakpoint instruction. */

-static unsigned char big_breakpoint[] = { 0x7d, 0x82, 0x10, 0x08 };
-static unsigned char little_breakpoint[] = { 0x08, 0x10, 0x82, 0x7d };
+constexpr gdb_byte big_breakpoint[] = { 0x7d, 0x82, 0x10, 0x08 };
+constexpr gdb_byte little_breakpoint[] = { 0x08, 0x10, 0x82, 0x7d };

-GDBARCH_BREAKPOINT_MANIPULATION_ENDIAN (rs6000, little_breakpoint,
- big_breakpoint)
+typedef BP_MANIPULATION_ENDIAN (little_breakpoint, big_breakpoint)
+ rs6000_breakpoint;

/* Instruction masks for displaced stepping. */
#define BRANCH_MASK 0xfc000000
@@ -6479,7 +6479,9 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
set_gdbarch_skip_main_prologue (gdbarch, rs6000_skip_main_prologue);

set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
- SET_GDBARCH_BREAKPOINT_MANIPULATION (rs6000);
+
+ set_gdbarch_breakpoint_kind_from_pc (gdbarch, rs6000_breakpoint::kind_from_pc);
+ set_gdbarch_sw_breakpoint_from_kind (gdbarch, rs6000_breakpoint::bp_from_kind);

/* The value of symbols of type N_SO and N_FUN maybe null when
it shouldn't be. */
--
2.5.5