gdb: refactor the non-printing disassemblers
authorAndrew Burgess <aburgess@redhat.com>
Mon, 4 Apr 2022 14:48:19 +0000 (15:48 +0100)
committerAndrew Burgess <aburgess@redhat.com>
Wed, 15 Jun 2022 08:44:55 +0000 (09:44 +0100)
This commit started from an observation I made while working on some
other disassembler patches, that is, that the function
gdb_buffered_insn_length, is broken ... sort of.

I noticed that the gdb_buffered_insn_length function doesn't set up
the application data field if the disassemble_info structure.

Further, I noticed that some architectures, for example, ARM, require
that the application_data field be set, see gdb_print_insn_arm in
arm-tdep.c.

And so, if we ever use gdb_buffered_insn_length for ARM, then GDB will
likely crash.  Which is why I said only "sort of" broken.  Right now
we don't use gdb_buffered_insn_length with ARM, so maybe it isn't
broken yet?

Anyway to prove to myself that there was a problem here I extended the
disassembler self tests in disasm-selftests.c to include a test of
gdb_buffered_insn_length.  As I run the test for all architectures, I
do indeed see GDB crash for ARM.

To fix this we need gdb_buffered_insn_length to create a disassembler
that inherits from gdb_disassemble_info, but we also need this new
disassembler to not print anything.

And so, I introduce a new gdb_non_printing_disassembler class, this is
a disassembler that doesn't print anything to the output stream.

I then observed that both ARC and S12Z also create non-printing
disassemblers, but these are slightly different.  While the
disassembler in gdb_non_printing_disassembler reads the instruction
from a buffer, the ARC and S12Z disassemblers read from target memory
using target_read_code.

And so, I further split gdb_non_printing_disassembler into two
sub-classes, gdb_non_printing_memory_disassembler and
gdb_non_printing_buffer_disassembler.

The new selftests now pass, but otherwise, there should be no user
visible changes after this commit.

gdb/arc-linux-tdep.c
gdb/arc-tdep.c
gdb/arc-tdep.h
gdb/disasm-selftests.c
gdb/disasm.c
gdb/disasm.h
gdb/s12z-tdep.c

index 13595f2e8e9b739d764de0cfca95d15ab3184645..04ca38f13555cf88b0e7a735cd9ef43a0acbd358 100644 (file)
@@ -356,7 +356,7 @@ arc_linux_sw_breakpoint_from_kind (struct gdbarch *gdbarch,
    */
 
 static std::vector<CORE_ADDR>
-handle_atomic_sequence (arc_instruction insn, disassemble_info &di)
+handle_atomic_sequence (arc_instruction insn, disassemble_info *di)
 {
   const int atomic_seq_len = 24;    /* Instruction sequence length.  */
   std::vector<CORE_ADDR> next_pcs;
@@ -374,7 +374,7 @@ handle_atomic_sequence (arc_instruction insn, disassemble_info &di)
   for (int insn_count = 0; insn_count < atomic_seq_len; ++insn_count)
     {
       arc_insn_decode (arc_insn_get_linear_next_pc (insn),
-                      &di, arc_delayed_print_insn, &insn);
+                      di, arc_delayed_print_insn, &insn);
 
       if (insn.insn_class == BRCC)
         {
@@ -412,15 +412,15 @@ arc_linux_software_single_step (struct regcache *regcache)
 {
   struct gdbarch *gdbarch = regcache->arch ();
   arc_gdbarch_tdep *tdep = (arc_gdbarch_tdep *) gdbarch_tdep (gdbarch);
-  struct disassemble_info di = arc_disassemble_info (gdbarch);
+  struct gdb_non_printing_memory_disassembler dis (gdbarch);
 
   /* Read current instruction.  */
   struct arc_instruction curr_insn;
-  arc_insn_decode (regcache_read_pc (regcache), &di, arc_delayed_print_insn,
-                  &curr_insn);
+  arc_insn_decode (regcache_read_pc (regcache), dis.disasm_info (),
+                  arc_delayed_print_insn, &curr_insn);
 
   if (curr_insn.insn_class == LLOCK)
-    return handle_atomic_sequence (curr_insn, di);
+    return handle_atomic_sequence (curr_insn, dis.disasm_info ());
 
   CORE_ADDR next_pc = arc_insn_get_linear_next_pc (curr_insn);
   std::vector<CORE_ADDR> next_pcs;
@@ -431,7 +431,8 @@ arc_linux_software_single_step (struct regcache *regcache)
   if (curr_insn.has_delay_slot)
     {
       struct arc_instruction next_insn;
-      arc_insn_decode (next_pc, &di, arc_delayed_print_insn, &next_insn);
+      arc_insn_decode (next_pc, dis.disasm_info (), arc_delayed_print_insn,
+                      &next_insn);
       next_pcs.push_back (arc_insn_get_linear_next_pc (next_insn));
     }
   else
index 3edfd466f3b90e3003c203e51a83ed54d48eedde..2f96e24a73414589684c41020d3c0cd0d2b1c0ad 100644 (file)
@@ -1306,24 +1306,6 @@ arc_is_in_prologue (struct gdbarch *gdbarch, const struct arc_instruction &insn,
   return false;
 }
 
-/* See arc-tdep.h.  */
-
-struct disassemble_info
-arc_disassemble_info (struct gdbarch *gdbarch)
-{
-  struct disassemble_info di;
-  init_disassemble_info_for_no_printing (&di);
-  di.arch = gdbarch_bfd_arch_info (gdbarch)->arch;
-  di.mach = gdbarch_bfd_arch_info (gdbarch)->mach;
-  di.endian = gdbarch_byte_order (gdbarch);
-  di.read_memory_func = [](bfd_vma memaddr, gdb_byte *myaddr,
-                          unsigned int len, struct disassemble_info *info)
-    {
-      return target_read_code (memaddr, myaddr, len);
-    };
-  return di;
-}
-
 /* Analyze the prologue and update the corresponding frame cache for the frame
    unwinder for unwinding frames that doesn't have debug info.  In such
    situation GDB attempts to parse instructions in the prologue to understand
@@ -1394,9 +1376,10 @@ arc_analyze_prologue (struct gdbarch *gdbarch, const CORE_ADDR entrypoint,
   while (current_prologue_end < limit_pc)
     {
       struct arc_instruction insn;
-      struct disassemble_info di = arc_disassemble_info (gdbarch);
-      arc_insn_decode (current_prologue_end, &di, arc_delayed_print_insn,
-                      &insn);
+
+      struct gdb_non_printing_memory_disassembler dis (gdbarch);
+      arc_insn_decode (current_prologue_end, dis.disasm_info (),
+                      arc_delayed_print_insn, &insn);
 
       if (arc_debug)
        arc_insn_dump (insn);
@@ -2460,8 +2443,8 @@ dump_arc_instruction_command (const char *args, int from_tty)
 
   CORE_ADDR address = value_as_address (val);
   struct arc_instruction insn;
-  struct disassemble_info di = arc_disassemble_info (target_gdbarch ());
-  arc_insn_decode (address, &di, arc_delayed_print_insn, &insn);
+  struct gdb_non_printing_memory_disassembler dis (target_gdbarch ());
+  arc_insn_decode (address, dis.disasm_info (), arc_delayed_print_insn, &insn);
   arc_insn_dump (insn);
 }
 
index ceca003204f4ebca4d01b87880761f62d166b06c..53e5d8476fc2709e9d5fd3b4e9061b9f34251bd3 100644 (file)
@@ -186,11 +186,6 @@ arc_arch_is_em (const struct bfd_arch_info* arch)
    can't be set to an actual NULL value - that would cause a crash.  */
 int arc_delayed_print_insn (bfd_vma addr, struct disassemble_info *info);
 
-/* Return properly initialized disassemble_info for ARC disassembler - it will
-   not print disassembled instructions to stderr.  */
-
-struct disassemble_info arc_disassemble_info (struct gdbarch *gdbarch);
-
 /* Get branch/jump target address for the INSN.  Note that this function
    returns branch target and doesn't evaluate if this branch is taken or not.
    For the indirect jumps value depends in register state, hence can change.
index 4f5667bc4e293eeb07a8cb2204f28479cb8bd527..db2d1e0ac592431a487ddfb3e12c95e8e4035ed7 100644 (file)
 
 namespace selftests {
 
-/* Test disassembly of one instruction.  */
+/* Return a pointer to a buffer containing an instruction that can be
+   disassembled for architecture GDBARCH.  *LEN will be set to the length
+   of the returned buffer.
 
-static void
-print_one_insn_test (struct gdbarch *gdbarch)
+   If there's no known instruction to disassemble for GDBARCH (because we
+   haven't figured on out, not because no instructions exist) then nullptr
+   is returned, and *LEN is set to 0.  */
+
+static const gdb_byte *
+get_test_insn (struct gdbarch *gdbarch, size_t *len)
 {
-  size_t len = 0;
-  const gdb_byte *insn = NULL;
+  *len = 0;
+  const gdb_byte *insn = nullptr;
 
   switch (gdbarch_bfd_arch_info (gdbarch)->arch)
     {
@@ -40,27 +46,27 @@ print_one_insn_test (struct gdbarch *gdbarch)
       static const gdb_byte bfin_insn[] = {0x17, 0xe1, 0xff, 0xff};
 
       insn = bfin_insn;
-      len = sizeof (bfin_insn);
+      *len = sizeof (bfin_insn);
       break;
     case bfd_arch_arm:
       /* mov     r0, #0 */
       static const gdb_byte arm_insn[] = {0x0, 0x0, 0xa0, 0xe3};
 
       insn = arm_insn;
-      len = sizeof (arm_insn);
+      *len = sizeof (arm_insn);
       break;
     case bfd_arch_ia64:
       /* We get:
         internal-error: gdbarch_sw_breakpoint_from_kind:
         Assertion `gdbarch->sw_breakpoint_from_kind != NULL' failed.  */
-      return;
+      return insn;
     case bfd_arch_mep:
       /* Disassembles as '*unknown*' insn, then len self-check fails.  */
-      return;
+      return insn;
     case bfd_arch_mips:
       if (gdbarch_bfd_arch_info (gdbarch)->mach == bfd_mach_mips16)
        /* Disassembles insn, but len self-check fails.  */
-       return;
+       return insn;
       goto generic_case;
     case bfd_arch_tic6x:
       /* Disassembles as '<undefined instruction 0x56454314>' insn, but len
@@ -68,7 +74,7 @@ print_one_insn_test (struct gdbarch *gdbarch)
       goto generic_case;
     case bfd_arch_xtensa:
       /* Disassembles insn, but len self-check fails.  */
-      return;
+      return insn;
     case bfd_arch_or1k:
       /* Disassembles as '*unknown*' insn, but len self-check passes, so let's
         allow it.  */
@@ -78,14 +84,14 @@ print_one_insn_test (struct gdbarch *gdbarch)
       static const gdb_byte s390_insn[] = {0x07, 0x07};
 
       insn = s390_insn;
-      len = sizeof (s390_insn);
+      *len = sizeof (s390_insn);
       break;
     case bfd_arch_xstormy16:
       /* nop */
       static const gdb_byte xstormy16_insn[] = {0x0, 0x0};
 
       insn = xstormy16_insn;
-      len = sizeof (xstormy16_insn);
+      *len = sizeof (xstormy16_insn);
       break;
     case bfd_arch_nios2:
     case bfd_arch_score:
@@ -96,19 +102,19 @@ print_one_insn_test (struct gdbarch *gdbarch)
       {
        int bplen;
        insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 4, &bplen);
-       len = bplen;
+       *len = bplen;
       }
       break;
     case bfd_arch_arc:
       /* PR 21003 */
       if (gdbarch_bfd_arch_info (gdbarch)->mach == bfd_mach_arc_arc601)
-       return;
+       return insn;
       goto generic_case;
     case bfd_arch_z80:
       {
        int bplen;
        insn = gdbarch_sw_breakpoint_from_kind (gdbarch, 0x0008, &bplen);
-       len = bplen;
+       *len = bplen;
       }
       break;
     case bfd_arch_i386:
@@ -118,7 +124,7 @@ print_one_insn_test (struct gdbarch *gdbarch)
           opcodes rejects an attempt to disassemble for an arch with
           a 64-bit address size when bfd_vma is 32-bit.  */
        if (info->bits_per_address > sizeof (bfd_vma) * CHAR_BIT)
-         return;
+         return insn;
       }
       /* fall through */
     default:
@@ -171,11 +177,25 @@ print_one_insn_test (struct gdbarch *gdbarch)
        /* Assert that we have found an instruction to disassemble.  */
        SELF_CHECK (found);
 
-       len = bplen;
+       *len = bplen;
        break;
       }
     }
-  SELF_CHECK (len > 0);
+  SELF_CHECK (*len > 0);
+
+  return insn;
+}
+
+/* Test disassembly of one instruction.  */
+
+static void
+print_one_insn_test (struct gdbarch *gdbarch)
+{
+  size_t len;
+  const gdb_byte *insn = get_test_insn (gdbarch, &len);
+
+  if (insn == nullptr)
+    return;
 
   /* Test gdb_disassembler for a given gdbarch by reading data from a
      pre-allocated buffer.  If you want to see the disassembled
@@ -234,6 +254,32 @@ print_one_insn_test (struct gdbarch *gdbarch)
   SELF_CHECK (di.print_insn (0) == len);
 }
 
+/* Test the gdb_buffered_insn_length function.  */
+
+static void
+buffered_insn_length_test (struct gdbarch *gdbarch)
+{
+  size_t buf_len;
+  const gdb_byte *insn = get_test_insn (gdbarch, &buf_len);
+
+  if (insn == nullptr)
+    return;
+
+  /* The tic6x architecture is VLIW.  Disassembling requires that the
+     entire instruction bundle be available.  However, the buffer we got
+     back from get_test_insn only contains a single instruction, which is
+     just part of an instruction bundle.  As a result, the disassemble will
+     fail.  To avoid this, skip tic6x tests now.  */
+  if (gdbarch_bfd_arch_info (gdbarch)->arch == bfd_arch_tic6x)
+    return;
+
+  CORE_ADDR insn_address = 0;
+  int calculated_len = gdb_buffered_insn_length (gdbarch, insn, buf_len,
+                                                insn_address);
+
+  SELF_CHECK (calculated_len == buf_len);
+}
+
 /* Test disassembly on memory error.  */
 
 static void
@@ -294,4 +340,6 @@ _initialize_disasm_selftests ()
                                         selftests::print_one_insn_test);
   selftests::register_test_foreach_arch ("memory_error",
                                         selftests::memory_error_test);
+  selftests::register_test_foreach_arch ("buffered_insn_length",
+                                        selftests::buffered_insn_length_test);
 }
index 4af40c916b2544ebeb106d3c80d64397ca107b5c..53cd6f5b6bb2c8959ece593252272592ddc11040 100644 (file)
@@ -1003,66 +1003,56 @@ gdb_insn_length (struct gdbarch *gdbarch, CORE_ADDR addr)
   return gdb_print_insn (gdbarch, addr, &null_stream, NULL);
 }
 
-/* An fprintf-function for use by the disassembler when we know we don't
-   want to print anything.  Always returns success.  */
+/* See disasm.h.  */
 
-static int ATTRIBUTE_PRINTF (2, 3)
-gdb_disasm_null_printf (void *stream, const char *format, ...)
+int
+gdb_non_printing_disassembler::null_fprintf_func (void *stream,
+                                                 const char *format, ...)
 {
   return 0;
 }
 
-/* An fprintf-function for use by the disassembler when we know we don't
-   want to print anything, and the disassembler is using style.  Always
-   returns success.  */
+/* See disasm.h.  */
 
-static int ATTRIBUTE_PRINTF (3, 4)
-gdb_disasm_null_styled_printf (void *stream,
-                              enum disassembler_style style,
-                              const char *format, ...)
+int
+gdb_non_printing_disassembler::null_fprintf_styled_func
+  (void *stream, enum disassembler_style style, const char *format, ...)
 {
   return 0;
 }
 
 /* See disasm.h.  */
 
-void
-init_disassemble_info_for_no_printing (struct disassemble_info *dinfo)
+int
+gdb_non_printing_memory_disassembler::dis_asm_read_memory
+  (bfd_vma memaddr, bfd_byte *myaddr, unsigned int length,
+   struct disassemble_info *dinfo)
 {
-  init_disassemble_info (dinfo, nullptr, gdb_disasm_null_printf,
-                        gdb_disasm_null_styled_printf);
+  return target_read_code (memaddr, myaddr, length);
 }
 
-/* Initialize a struct disassemble_info for gdb_buffered_insn_length.
-   Upon return, *DISASSEMBLER_OPTIONS_HOLDER owns the string pointed
-   to by DI.DISASSEMBLER_OPTIONS.  */
+/* A non-printing disassemble_info management class.  The disassemble_info
+   setup by this class will not print anything to the output stream (there
+   is no output stream), and the instruction to be disassembled will be
+   read from a buffer passed to the constructor.  */
 
-static void
-gdb_buffered_insn_length_init_dis (struct gdbarch *gdbarch,
-                                  struct disassemble_info *di,
-                                  const gdb_byte *insn, int max_len,
-                                  CORE_ADDR addr,
-                                  std::string *disassembler_options_holder)
+struct gdb_non_printing_buffer_disassembler
+  : public gdb_non_printing_disassembler
 {
-  init_disassemble_info_for_no_printing (di);
-
-  /* init_disassemble_info installs buffer_read_memory, etc.
-     so we don't need to do that here.
-     The cast is necessary until disassemble_info is const-ified.  */
-  di->buffer = (gdb_byte *) insn;
-  di->buffer_length = max_len;
-  di->buffer_vma = addr;
-
-  di->arch = gdbarch_bfd_arch_info (gdbarch)->arch;
-  di->mach = gdbarch_bfd_arch_info (gdbarch)->mach;
-  di->endian = gdbarch_byte_order (gdbarch);
-  di->endian_code = gdbarch_byte_order_for_code (gdbarch);
-
-  *disassembler_options_holder = get_all_disassembler_options (gdbarch);
-  if (!disassembler_options_holder->empty ())
-    di->disassembler_options = disassembler_options_holder->c_str ();
-  disassemble_init_for_target (di);
-}
+  /* Constructor.  GDBARCH is the architecture to disassemble for, BUFFER
+     contains the instruction to disassemble, and INSN_ADDRESS is the
+     address (in target memory) of the instruction to disassemble.  */
+  gdb_non_printing_buffer_disassembler (struct gdbarch *gdbarch,
+                                       gdb::array_view<const gdb_byte> buffer,
+                                       CORE_ADDR insn_address)
+    : gdb_non_printing_disassembler (gdbarch, nullptr)
+  {
+    /* The cast is necessary until disassemble_info is const-ified.  */
+    m_di.buffer = (gdb_byte *) buffer.data ();
+    m_di.buffer_length = buffer.size ();
+    m_di.buffer_vma = insn_address;
+  }
+};
 
 /* Return the length in bytes of INSN.  MAX_LEN is the size of the
    buffer containing INSN.  */
@@ -1071,14 +1061,10 @@ int
 gdb_buffered_insn_length (struct gdbarch *gdbarch,
                          const gdb_byte *insn, int max_len, CORE_ADDR addr)
 {
-  struct disassemble_info di;
-  std::string disassembler_options_holder;
-
-  gdb_buffered_insn_length_init_dis (gdbarch, &di, insn, max_len, addr,
-                                    &disassembler_options_holder);
-
-  int result = gdb_print_insn_1 (gdbarch, addr, &di);
-  disassemble_free_target (&di);
+  gdb::array_view<const gdb_byte> buffer
+    = gdb::make_array_view (insn, max_len);
+  gdb_non_printing_buffer_disassembler dis (gdbarch, buffer, addr);
+  int result = gdb_print_insn_1 (gdbarch, addr, dis.disasm_info ());
   return result;
 }
 
index f31ca92b038042981b60efc256bd23608ecb4df6..ec5120351a1877a27f1dc59a9e4cd5c8d42df3a7 100644 (file)
@@ -136,6 +136,56 @@ protected:
     ATTRIBUTE_PRINTF(3,4);
 };
 
+/* A basic disassembler that doesn't actually print anything.  */
+
+struct gdb_non_printing_disassembler : public gdb_disassemble_info
+{
+  gdb_non_printing_disassembler (struct gdbarch *gdbarch,
+                                read_memory_ftype read_memory_func)
+    : gdb_disassemble_info (gdbarch, nullptr /* stream */,
+                           read_memory_func,
+                           nullptr /* memory_error_func */,
+                           nullptr /* print_address_func */,
+                           null_fprintf_func,
+                           null_fprintf_styled_func)
+  { /* Nothing.  */ }
+
+private:
+
+  /* Callback used as the disassemble_info's fprintf_func callback, this
+     doesn't write anything to STREAM, but just returns 0.  */
+  static int null_fprintf_func (void *stream, const char *format, ...)
+    ATTRIBUTE_PRINTF(2,3);
+
+  /* Callback used as the disassemble_info's fprintf_styled_func callback,
+     , this doesn't write anything to STREAM, but just returns 0.  */
+  static int null_fprintf_styled_func (void *stream,
+                                      enum disassembler_style style,
+                                      const char *format, ...)
+    ATTRIBUTE_PRINTF(3,4);
+};
+
+/* A non-printing disassemble_info management class.  The disassemble_info
+   setup by this class will not print anything to the output stream (there
+   is no output stream), and the instruction to be disassembled will be
+   read from target memory.  */
+
+struct gdb_non_printing_memory_disassembler
+  : public gdb_non_printing_disassembler
+{
+  /* Constructor.  GDBARCH is the architecture to disassemble for.  */
+  gdb_non_printing_memory_disassembler (struct gdbarch *gdbarch)
+    :gdb_non_printing_disassembler (gdbarch, dis_asm_read_memory)
+  { /* Nothing.  */ }
+
+private:
+
+  /* Implements the read_memory_func disassemble_info callback.  */
+  static int dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr,
+                                 unsigned int len,
+                                 struct disassemble_info *info);
+};
+
 /* A dissassembler class that provides 'print_insn', a method for
    disassembling a single instruction to the output stream.  */
 
@@ -278,10 +328,4 @@ extern char *get_disassembler_options (struct gdbarch *gdbarch);
 
 extern void set_disassembler_options (const char *options);
 
-/* Setup DINFO with its output function and output stream setup so that
-   nothing is printed while disassembling.  */
-
-extern void init_disassemble_info_for_no_printing
-  (struct disassemble_info *dinfo);
-
 #endif
index 5394c1bbf5ea457acb999a08a66ebde0a5372fce..4e33faaea9a5163e369166035ac55e50853662f8 100644 (file)
@@ -141,27 +141,6 @@ s12z_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int num)
 
 /* Support functions for frame handling.  */
 
-
-/* Return a disassemble_info initialized for s12z disassembly, however,
-   the disassembler will not actually print anything.  */
-
-static struct disassemble_info
-s12z_disassemble_info (struct gdbarch *gdbarch)
-{
-  struct disassemble_info di;
-  init_disassemble_info_for_no_printing (&di);
-  di.arch = gdbarch_bfd_arch_info (gdbarch)->arch;
-  di.mach = gdbarch_bfd_arch_info (gdbarch)->mach;
-  di.endian = gdbarch_byte_order (gdbarch);
-  di.read_memory_func = [](bfd_vma memaddr, gdb_byte *myaddr,
-                          unsigned int len, struct disassemble_info *info)
-    {
-      return target_read_code (memaddr, myaddr, len);
-    };
-  return di;
-}
-
-
 /* A struct (based on mem_read_abstraction_base) to read memory
    through the disassemble_info API.  */
 struct mem_read_abstraction
@@ -332,15 +311,14 @@ s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache)
   int frame_size = 0;
   int saved_frame_size = 0;
 
-  struct disassemble_info di = s12z_disassemble_info (gdbarch);
-
+  struct gdb_non_printing_memory_disassembler dis (gdbarch);
 
   struct mem_read_abstraction mra;
   mra.base.read = (int (*)(mem_read_abstraction_base*,
                           int, size_t, bfd_byte*)) abstract_read_memory;
   mra.base.advance = advance ;
   mra.base.posn = posn;
-  mra.info = &di;
+  mra.info = dis.disasm_info ();
 
   while (this_pc > addr)
     {