public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [RFC] PR 20939: Handle error in disassembly
@ 2016-12-12 10:48 Yao Qi
  2016-12-14 17:31 ` Pedro Alves
  0 siblings, 1 reply; 4+ messages in thread
From: Yao Qi @ 2016-12-12 10:48 UTC (permalink / raw)
  To: gdb-patches

Hi,
GDB calls some APIs from opcodes to do disassembly and provide some
call backs.  This model makes troubles on C++ exception unwinding,
because GDB is a C++ program, and opcodes is still compiled as C.
As we can see, frame #10 and #12 are C++, while #frame 11 is C,

 #10 0x0000000000544228 in memory_error (err=TARGET_XFER_E_IO, memaddr=<optimized out>) at ../../binutils-gdb/gdb/corefile.c:237
 #11 0x00000000006b0a54 in print_insn_aarch64 (pc=0, info=0xffffffffeeb0) at ../../binutils-gdb/opcodes/aarch64-dis.c:3185
 #12 0x0000000000553590 in gdb_pretty_print_insn (gdbarch=gdbarch@entry=0xbbceb0, uiout=uiout@entry=0xbc73d0, di=di@entry=0xffffffffeeb0, 
    insn=0xffffffffed40, insn@entry=0xffffffffed90, flags=flags@entry=0, 

C++ exception unwinder can't go across frame #11 unless it has
unwind table.  However, C program on many architectures doesn't
have it in default.  As a result, GDB aborts, which is described
in PR 20939.

This is not the first time we see this kind of problem.  We've
had a commit 89525768cd086a0798a504c81fdf7ebcd4c904e1
"Propagate GDB/C++ exceptions across readline using sj/lj-based TRY/CATCH".
We can fix the disassembly bug in a similar way, this is the option one.

Alternatively, we can do more changes in opcodes, because opcodes is
built together with gdb.  Don't throw exception in dis_asm_memory_error,
and only throw exception if the return value of print_insn_$ARCH is -1
in GDB.  This is the option two, which is demonstrated by the patch
below.  This requires every print_insn_$ARCH function return -1 on
memory error, but msp430 and m68k don't follow this convention yet.

Which option do you prefer?  If we prefer option one, the change is
only within the GDB scope.  If we prefer option two, it goes out to
opcodes, and I'll bring the discussion to binutils.  I prefer this
one.

Note that, no matter which option do we take, the fix should be
backported to 7.12 branch, in which GDB can still be built as a C
program.

-- 
Yao (齐尧)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 0175630..4f5f056 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1771,8 +1771,15 @@ aarch64_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
 static int
 aarch64_gdb_print_insn (bfd_vma memaddr, disassemble_info *info)
 {
+  int ret;
+
   info->symbols = NULL;
-  return print_insn_aarch64 (memaddr, info);
+  ret = print_insn_aarch64 (memaddr, info);
+
+  if (ret == -1)
+    memory_error (TARGET_XFER_E_IO, memaddr);
+
+  return ret;
 }
 
 /* AArch64 BRK software debug mode instruction.
diff --git a/gdb/disasm.c b/gdb/disasm.c
index 6f9f5f9..437b64c 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -137,7 +137,7 @@ static void
 dis_asm_memory_error (int err, bfd_vma memaddr,
                      struct disassemble_info *info)
 {
-  memory_error (TARGET_XFER_E_IO, memaddr);
+  /*memory_error (TARGET_XFER_E_IO, memaddr);*/
 }
 
 /* Like print_address with slightly different parameters.  */

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC] PR 20939: Handle error in disassembly
  2016-12-12 10:48 [RFC] PR 20939: Handle error in disassembly Yao Qi
@ 2016-12-14 17:31 ` Pedro Alves
  2016-12-14 22:05   ` Yao Qi
  0 siblings, 1 reply; 4+ messages in thread
From: Pedro Alves @ 2016-12-14 17:31 UTC (permalink / raw)
  To: Yao Qi, gdb-patches

On 12/12/2016 10:48 AM, Yao Qi wrote:
> Hi,
> GDB calls some APIs from opcodes to do disassembly and provide some
> call backs.  This model makes troubles on C++ exception unwinding,
> because GDB is a C++ program, and opcodes is still compiled as C.
> As we can see, frame #10 and #12 are C++, while #frame 11 is C,
> 
>  #10 0x0000000000544228 in memory_error (err=TARGET_XFER_E_IO, memaddr=<optimized out>) at ../../binutils-gdb/gdb/corefile.c:237
>  #11 0x00000000006b0a54 in print_insn_aarch64 (pc=0, info=0xffffffffeeb0) at ../../binutils-gdb/opcodes/aarch64-dis.c:3185
>  #12 0x0000000000553590 in gdb_pretty_print_insn (gdbarch=gdbarch@entry=0xbbceb0, uiout=uiout@entry=0xbc73d0, di=di@entry=0xffffffffeeb0, 
>     insn=0xffffffffed40, insn@entry=0xffffffffed90, flags=flags@entry=0, 
> 
> C++ exception unwinder can't go across frame #11 unless it has
> unwind table.  However, C program on many architectures doesn't
> have it in default.  As a result, GDB aborts, which is described
> in PR 20939.
> 
> This is not the first time we see this kind of problem.  We've
> had a commit 89525768cd086a0798a504c81fdf7ebcd4c904e1
> "Propagate GDB/C++ exceptions across readline using sj/lj-based TRY/CATCH".
> We can fix the disassembly bug in a similar way, this is the option one.
> 
> Alternatively, we can do more changes in opcodes, because opcodes is
> built together with gdb.  Don't throw exception in dis_asm_memory_error,
> and only throw exception if the return value of print_insn_$ARCH is -1
> in GDB.  This is the option two, which is demonstrated by the patch
> below.  This requires every print_insn_$ARCH function return -1 on
> memory error, but msp430 and m68k don't follow this convention yet.
> 
> Which option do you prefer?  If we prefer option one, the change is
> only within the GDB scope.  If we prefer option two, it goes out to
> opcodes, and I'll bring the discussion to binutils.  I prefer this
> one.

Did you try to find the discussions around when the current
interface based on throwing (using longjmp at the time) was added?
Maybe the "return -1" option was considered back then, but
discarded for some reason?

E.g., looks like simply "return -1" would lose the actual
address that failed to be read, in case opcodes does several
reads in sequence and its not the first that fails.  We could
add some other means to get at that, of course.

> 
> Note that, no matter which option do we take, the fix should be
> backported to 7.12 branch, in which GDB can still be built as a C
> program.
> 

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC] PR 20939: Handle error in disassembly
  2016-12-14 17:31 ` Pedro Alves
@ 2016-12-14 22:05   ` Yao Qi
  2016-12-16 16:40     ` Pedro Alves
  0 siblings, 1 reply; 4+ messages in thread
From: Yao Qi @ 2016-12-14 22:05 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On 16-12-14 17:31:18, Pedro Alves wrote:
>
> Did you try to find the discussions around when the current
> interface based on throwing (using longjmp at the time) was added?
> Maybe the "return -1" option was considered back then, but
> discarded for some reason?

Such interface was added in 1993,
5d0734a7d74cf01b73303aeb884b719b4b220035 there wasn't any
discussions.

>
> E.g., looks like simply "return -1" would lose the actual
> address that failed to be read, in case opcodes does several
> reads in sequence and its not the first that fails.  We could
> add some other means to get at that, of course.
>

I thought about this beofre.  We can't get the actual address that
failed to be read today, because we read a piece of memory in
dis_asm_read_memory, but only report error from the starting
address.  So I think it isn't very important to report the error with
the accurate memory address failed to be read.

-- 
Yao

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC] PR 20939: Handle error in disassembly
  2016-12-14 22:05   ` Yao Qi
@ 2016-12-16 16:40     ` Pedro Alves
  0 siblings, 0 replies; 4+ messages in thread
From: Pedro Alves @ 2016-12-16 16:40 UTC (permalink / raw)
  To: Yao Qi; +Cc: gdb-patches

On 12/14/2016 10:05 PM, Yao Qi wrote:
> On 16-12-14 17:31:18, Pedro Alves wrote:
>>
>> Did you try to find the discussions around when the current
>> interface based on throwing (using longjmp at the time) was added?
>> Maybe the "return -1" option was considered back then, but
>> discarded for some reason?
> 
> Such interface was added in 1993,
> 5d0734a7d74cf01b73303aeb884b719b4b220035 there wasn't any
> discussions.
> 
>>
>> E.g., looks like simply "return -1" would lose the actual
>> address that failed to be read, in case opcodes does several
>> reads in sequence and its not the first that fails.  We could
>> add some other means to get at that, of course.
>>
> 
> I thought about this beofre.  We can't get the actual address that
> failed to be read today, because we read a piece of memory in
> dis_asm_read_memory, but only report error from the starting
> address.  So I think it isn't very important to report the error with
> the accurate memory address failed to be read.
> 

Not sure about that.  At least the i386 disassembler seems to
already read memory 1 byte at a time?  (and if dcache fails
a line read, it'll try the range specified by the caller.)

There's also the "err" parameter to memory_error_func.
We don't currently do anything with it, but we could use
it to distinguish between the different memory errors.

An alternative is to change gdb's disasm.c:dis_asm_memory_error
to save the error info, instead of throwing an exception
immediately with memory_error.

On 12/12/2016 10:48 AM, Yao Qi wrote:
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -1771,8 +1771,15 @@ aarch64_dwarf_reg_to_regnum (struct gdbarch *gdbarch, int reg)
>  static int
>  aarch64_gdb_print_insn (bfd_vma memaddr, disassemble_info *info)
>  {
> +  int ret;
> +
>    info->symbols = NULL;
> -  return print_insn_aarch64 (memaddr, info);
> +  ret = print_insn_aarch64 (memaddr, info);
> +
> +  if (ret == -1)
> +    memory_error (TARGET_XFER_E_IO, memaddr);
> +
> +  return ret;
>  }

If we take this route, we should really do this as a
wrapper around gdbarch_print_insn, instead of hacking all
implementations, and leave gdbarch_print_insn just as a run-time
selection of the appropriate print_insn_$arch function.
There are only a few calls to gdbarch_print_insn, and they're all in
generic disassembling code:

$ grep gdbarch_print_insn -rn | grep -v set_gdbarch_print_insn | grep -v "gdbarch\.[h|c]"
guile/scm-disasm.c:153:   Call gdbarch_print_insn using a port for input.
guile/scm-disasm.c:180:  length = gdbarch_print_insn (gdbarch, memaddr, &di);
disasm.c:257:      size = gdbarch_print_insn (gdbarch, pc, di);
disasm.c:276:    size = gdbarch_print_insn (gdbarch, pc, di);
disasm.c:837:  length = gdbarch_print_insn (gdbarch, memaddr, &di);
disasm.c:918:  return gdbarch_print_insn (gdbarch, addr, &di);

I went ahead and wrote a POC patch wrapping gdbarch_print_insn,
and storing the error address.  This actually adds a new class
that wraps a bit more than just the function.  See below.
Passes regtesting for me on x86_64.

For 7.12, since we can't assume C++ there, we'd need to convert
this to plain C, but that should be simple, given there's no
inheritance involved.

From ec4fb6902b7b2c23e07b0985270d7be41ed05afa Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 16 Dec 2016 13:19:14 +0000
Subject: [PATCH] gdb_disassembler

---
 gdb/disasm.c           | 167 ++++++++++++++++++++++++++++---------------------
 gdb/disasm.h           |  52 +++++++++++----
 gdb/guile/scm-disasm.c |  78 +++++++----------------
 gdb/record-btrace.c    |   5 +-
 4 files changed, 159 insertions(+), 143 deletions(-)

diff --git a/gdb/disasm.c b/gdb/disasm.c
index 07c3abe..5450bbc 100644
--- a/gdb/disasm.c
+++ b/gdb/disasm.c
@@ -120,28 +120,37 @@ line_has_code_p (htab_t table, struct symtab *symtab, int line)
 }
 
 /* Like target_read_memory, but slightly different parameters.  */
-static int
-dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len,
-		     struct disassemble_info *info)
+
+int
+gdb_disassembler::dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr,
+				       unsigned int len,
+				       struct disassemble_info *info)
 {
   return target_read_code (memaddr, myaddr, len);
 }
 
 /* Like memory_error with slightly different parameters.  */
-static void
-dis_asm_memory_error (int err, bfd_vma memaddr,
-		      struct disassemble_info *info)
+
+void
+gdb_disassembler::dis_asm_memory_error (int err, bfd_vma memaddr,
+					struct disassemble_info *info)
 {
-  memory_error (TARGET_XFER_E_IO, memaddr);
+  gdb_disassembler *self
+    = static_cast<gdb_disassembler *>(info->application_data);
+
+  self->m_err_memaddr = memaddr;
 }
 
 /* Like print_address with slightly different parameters.  */
-static void
-dis_asm_print_address (bfd_vma addr, struct disassemble_info *info)
+
+void
+gdb_disassembler::dis_asm_print_address (bfd_vma addr,
+					 struct disassemble_info *info)
 {
-  struct gdbarch *gdbarch = (struct gdbarch *) info->application_data;
+  gdb_disassembler *self
+    = static_cast<gdb_disassembler *>(info->application_data);
 
-  print_address (gdbarch, addr, (struct ui_file *) info->stream);
+  print_address (self->arch (), addr, self->stream ());
 }
 
 static int
@@ -173,10 +182,9 @@ compare_lines (const void *mle1p, const void *mle2p)
 /* See disasm.h.  */
 
 int
-gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
-		       struct disassemble_info * di,
-		       const struct disasm_insn *insn, int flags,
-		       struct ui_file *stb)
+gdb_disassembler::pretty_print_insn (struct ui_out *uiout,
+				     const struct disasm_insn *insn,
+				     int flags)
 {
   /* parts of the symbolic representation of the address */
   int unmapped;
@@ -187,6 +195,8 @@ gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
   char *filename = NULL;
   char *name = NULL;
   CORE_ADDR pc;
+  struct ui_file *stb = stream ();
+  struct gdbarch *gdbarch = arch ();
 
   ui_out_chain = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
   pc = insn->addr;
@@ -254,14 +264,14 @@ gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
       struct cleanup *cleanups =
 	make_cleanup_ui_file_delete (opcode_stream);
 
-      size = gdbarch_print_insn (gdbarch, pc, di);
+      size = print_insn (pc);
       end_pc = pc + size;
 
       for (;pc < end_pc; ++pc)
 	{
-	  err = (*di->read_memory_func) (pc, &data, 1, di);
+	  err = m_di.read_memory_func (pc, &data, 1, &m_di);
 	  if (err != 0)
-	    (*di->memory_error_func) (err, pc, di);
+	    m_di.memory_error_func (err, pc, &m_di);
 	  fprintf_filtered (opcode_stream, "%s%02x",
 			    spacer, (unsigned) data);
 	  spacer = " ";
@@ -273,7 +283,7 @@ gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
       do_cleanups (cleanups);
     }
   else
-    size = gdbarch_print_insn (gdbarch, pc, di);
+    size = print_insn (pc);
 
   ui_out_field_stream (uiout, "inst", stb);
   ui_file_rewind (stb);
@@ -284,10 +294,9 @@ gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
 }
 
 static int
-dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
-	    struct disassemble_info * di,
+dump_insns (struct ui_out *uiout, gdb_disassembler *di,
 	    CORE_ADDR low, CORE_ADDR high,
-	    int how_many, int flags, struct ui_file *stb,
+	    int how_many, int flags,
 	    CORE_ADDR *end_pc)
 {
   struct disasm_insn insn;
@@ -300,7 +309,7 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
     {
       int size;
 
-      size = gdb_pretty_print_insn (gdbarch, uiout, di, &insn, flags, stb);
+      size = di->pretty_print_insn (uiout, &insn, flags);
       if (size <= 0)
 	break;
 
@@ -326,10 +335,10 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
 
 static void
 do_mixed_source_and_assembly_deprecated
-  (struct gdbarch *gdbarch, struct ui_out *uiout,
-   struct disassemble_info *di, struct symtab *symtab,
+  (struct ui_out *uiout,
+   gdb_disassembler *di, struct symtab *symtab,
    CORE_ADDR low, CORE_ADDR high,
-   int how_many, int flags, struct ui_file *stb)
+   int how_many, int flags)
 {
   int newlines = 0;
   int nlines;
@@ -462,9 +471,9 @@ do_mixed_source_and_assembly_deprecated
 	    = make_cleanup_ui_out_list_begin_end (uiout, "line_asm_insn");
 	}
 
-      num_displayed += dump_insns (gdbarch, uiout, di,
+      num_displayed += dump_insns (uiout, di,
 				   mle[i].start_pc, mle[i].end_pc,
-				   how_many, flags, stb, NULL);
+				   how_many, flags, NULL);
 
       /* When we've reached the end of the mle array, or we've seen the last
          assembly range for this source line, close out the list/tuple.  */
@@ -488,11 +497,12 @@ do_mixed_source_and_assembly_deprecated
    immediately following.  */
 
 static void
-do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
-			      struct disassemble_info *di,
+do_mixed_source_and_assembly (struct gdbarch *gdbarch,
+			      struct ui_out *uiout,
+			      gdb_disassembler *di,
 			      struct symtab *main_symtab,
 			      CORE_ADDR low, CORE_ADDR high,
-			      int how_many, int flags, struct ui_file *stb)
+			      int how_many, int flags)
 {
   const struct linetable_entry *le, *first_le;
   int i, nlines;
@@ -717,8 +727,8 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 	end_pc = std::min (sal.end, high);
       else
 	end_pc = pc + 1;
-      num_displayed += dump_insns (gdbarch, uiout, di, pc, end_pc,
-				   how_many, flags, stb, &end_pc);
+      num_displayed += dump_insns (uiout, di, pc, end_pc,
+				   how_many, flags, &end_pc);
       pc = end_pc;
 
       if (how_many >= 0 && num_displayed >= how_many)
@@ -733,16 +743,16 @@ do_mixed_source_and_assembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 }
 
 static void
-do_assembly_only (struct gdbarch *gdbarch, struct ui_out *uiout,
-		  struct disassemble_info * di,
+do_assembly_only (struct ui_out *uiout,
+		  gdb_disassembler *di,
 		  CORE_ADDR low, CORE_ADDR high,
-		  int how_many, int flags, struct ui_file *stb)
+		  int how_many, int flags)
 {
   struct cleanup *ui_out_chain;
 
   ui_out_chain = make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
 
-  dump_insns (gdbarch, uiout, di, low, high, how_many, flags, stb, NULL);
+  dump_insns (uiout, di, low, high, how_many, flags, NULL);
 
   do_cleanups (ui_out_chain);
 }
@@ -762,15 +772,15 @@ fprintf_disasm (void *stream, const char *format, ...)
   return 0;
 }
 
-struct disassemble_info
-gdb_disassemble_info (struct gdbarch *gdbarch, struct ui_file *file)
+gdb_disassembler::gdb_disassembler (struct gdbarch *gdbarch,
+				    struct ui_file *file)
+  : m_gdbarch (gdbarch),
+    m_err_memaddr (0)
 {
-  struct disassemble_info di;
-
-  init_disassemble_info (&di, file, fprintf_disasm);
-  di.flavour = bfd_target_unknown_flavour;
-  di.memory_error_func = dis_asm_memory_error;
-  di.print_address_func = dis_asm_print_address;
+  init_disassemble_info (&m_di, file, fprintf_disasm);
+  m_di.flavour = bfd_target_unknown_flavour;
+  m_di.memory_error_func = dis_asm_memory_error;
+  m_di.print_address_func = dis_asm_print_address;
   /* NOTE: cagney/2003-04-28: The original code, from the old Insight
      disassembler had a local optomization here.  By default it would
      access the executable file, instead of the target memory (there
@@ -779,14 +789,40 @@ gdb_disassemble_info (struct gdbarch *gdbarch, struct ui_file *file)
      didn't work as they relied on the access going to the target.
      Further, it has been supperseeded by trust-read-only-sections
      (although that should be superseeded by target_trust..._p()).  */
-  di.read_memory_func = dis_asm_read_memory;
-  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);
-  di.application_data = gdbarch;
-  disassemble_init_for_target (&di);
-  return di;
+  m_di.read_memory_func = dis_asm_read_memory;
+  m_di.arch = gdbarch_bfd_arch_info (gdbarch)->arch;
+  m_di.mach = gdbarch_bfd_arch_info (gdbarch)->mach;
+  m_di.endian = gdbarch_byte_order (gdbarch);
+  m_di.endian_code = gdbarch_byte_order_for_code (gdbarch);
+  m_di.application_data = this;
+  disassemble_init_for_target (&m_di);
+}
+
+int
+gdb_disassembler::print_insn (CORE_ADDR memaddr)
+{
+  m_err_memaddr = 0;
+
+  int length = gdbarch_print_insn (arch (), memaddr, &m_di);
+  if (length < 0)
+    memory_error (TARGET_XFER_E_IO, m_err_memaddr);
+  return length;
+}
+
+int
+gdb_disassembler::print_insn (CORE_ADDR memaddr,
+			      int *branch_delay_insns)
+{
+  int length = print_insn (memaddr);
+
+  if (branch_delay_insns != NULL)
+    {
+      if (m_di.insn_info_valid)
+	*branch_delay_insns = m_di.branch_delay_insns;
+      else
+	*branch_delay_insns = 0;
+    }
+  return length;
 }
 
 void
@@ -796,7 +832,7 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 {
   struct ui_file *stb = mem_fileopen ();
   struct cleanup *cleanups = make_cleanup_ui_file_delete (stb);
-  struct disassemble_info di = gdb_disassemble_info (gdbarch, stb);
+  gdb_disassembler di (gdbarch, stb);
   struct symtab *symtab;
   int nlines = -1;
 
@@ -808,15 +844,15 @@ gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 
   if (!(flags & (DISASSEMBLY_SOURCE_DEPRECATED | DISASSEMBLY_SOURCE))
       || nlines <= 0)
-    do_assembly_only (gdbarch, uiout, &di, low, high, how_many, flags, stb);
+    do_assembly_only (uiout, &di, low, high, how_many, flags);
 
   else if (flags & DISASSEMBLY_SOURCE)
     do_mixed_source_and_assembly (gdbarch, uiout, &di, symtab, low, high,
-				  how_many, flags, stb);
+				  how_many, flags);
 
   else if (flags & DISASSEMBLY_SOURCE_DEPRECATED)
-    do_mixed_source_and_assembly_deprecated (gdbarch, uiout, &di, symtab,
-					     low, high, how_many, flags, stb);
+    do_mixed_source_and_assembly_deprecated (uiout, &di, symtab,
+					     low, high, how_many, flags);
 
   do_cleanups (cleanups);
   gdb_flush (gdb_stdout);
@@ -830,19 +866,10 @@ int
 gdb_print_insn (struct gdbarch *gdbarch, CORE_ADDR memaddr,
 		struct ui_file *stream, int *branch_delay_insns)
 {
-  struct disassemble_info di;
-  int length;
 
-  di = gdb_disassemble_info (gdbarch, stream);
-  length = gdbarch_print_insn (gdbarch, memaddr, &di);
-  if (branch_delay_insns)
-    {
-      if (di.insn_info_valid)
-	*branch_delay_insns = di.branch_delay_insns;
-      else
-	*branch_delay_insns = 0;
-    }
-  return length;
+  gdb_disassembler di (gdbarch, stream);
+
+  return di.print_insn (memaddr, branch_delay_insns);
 }
 
 static void
diff --git a/gdb/disasm.h b/gdb/disasm.h
index a2b72b9..c71fa78 100644
--- a/gdb/disasm.h
+++ b/gdb/disasm.h
@@ -33,6 +33,45 @@ struct gdbarch;
 struct ui_out;
 struct ui_file;
 
+class gdb_disassembler
+{
+  using di_read_memory_ftype = decltype (disassemble_info::read_memory_func);
+
+public:
+  gdb_disassembler (struct gdbarch *gdbarch, struct ui_file *file);
+
+  int print_insn (CORE_ADDR memaddr);
+  int print_insn (CORE_ADDR memaddr, int *branch_delay_insns);
+
+  /* Prints the instruction INSN into UIOUT and returns the length of
+     the printed instruction in bytes.  */
+  int pretty_print_insn (struct ui_out *uiout,
+			 const struct disasm_insn *insn, int flags);
+
+protected:
+  void set_read_memory_func (di_read_memory_ftype func)
+  { m_di.read_memory_func = func; }
+
+  struct ui_file *stream ()
+  { return (struct ui_file *) m_di.stream; }
+
+  struct gdbarch *arch ()
+  { return m_gdbarch; }
+
+private:
+  struct disassemble_info m_di;
+  struct gdbarch *m_gdbarch;
+  CORE_ADDR m_err_memaddr;
+
+  static int dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr,
+				  unsigned int len,
+				  struct disassemble_info *info);
+  static void dis_asm_memory_error (int err, bfd_vma memaddr,
+				    struct disassemble_info *info);
+  static void dis_asm_print_address (bfd_vma addr,
+				     struct disassemble_info *info);
+};
+
 /* An instruction to be disassembled.  */
 
 struct disasm_insn
@@ -47,19 +86,6 @@ struct disasm_insn
   unsigned int is_speculative:1;
 };
 
-/* Prints the instruction INSN into UIOUT and returns the length of the
-   printed instruction in bytes.  */
-
-extern int gdb_pretty_print_insn (struct gdbarch *gdbarch, struct ui_out *uiout,
-				  struct disassemble_info * di,
-				  const struct disasm_insn *insn, int flags,
-				  struct ui_file *stb);
-
-/* Return a filled in disassemble_info object for use by gdb.  */
-
-extern struct disassemble_info gdb_disassemble_info (struct gdbarch *gdbarch,
-						     struct ui_file *file);
-
 extern void gdb_disassembly (struct gdbarch *gdbarch, struct ui_out *uiout,
 			     char *file_string, int flags, int how_many,
 			     CORE_ADDR low, CORE_ADDR high);
diff --git a/gdb/guile/scm-disasm.c b/gdb/guile/scm-disasm.c
index a7d6b14..340c5e6 100644
--- a/gdb/guile/scm-disasm.c
+++ b/gdb/guile/scm-disasm.c
@@ -37,11 +37,13 @@ static SCM address_symbol;
 static SCM asm_symbol;
 static SCM length_symbol;
 
-/* Struct used to pass "application data" in disassemble_info.  */
-
-struct gdbscm_disasm_data
+class gdbscm_disassembler : public gdb_disassembler
 {
-  struct gdbarch *gdbarch;
+public:
+  gdbscm_disassembler (struct gdbarch *gdbarch,
+		       struct ui_file *stream,
+		       SCM port, ULONGEST offset);
+
   SCM port;
   /* The offset of the address of the first instruction in PORT.  */
   ULONGEST offset;
@@ -55,7 +57,7 @@ struct gdbscm_disasm_read_data
   bfd_vma memaddr;
   bfd_byte *myaddr;
   unsigned int length;
-  struct disassemble_info *dinfo;
+  gdbscm_disassembler *dinfo;
 };
 \f
 /* Subroutine of gdbscm_arch_disassemble to simplify it.
@@ -81,13 +83,11 @@ gdbscm_disasm_read_memory_worker (void *datap)
 {
   struct gdbscm_disasm_read_data *data
     = (struct gdbscm_disasm_read_data *) datap;
-  struct disassemble_info *dinfo = data->dinfo;
-  struct gdbscm_disasm_data *disasm_data
-    = (struct gdbscm_disasm_data *) dinfo->application_data;
-  SCM seekto, newpos, port = disasm_data->port;
+  gdbscm_disassembler *dinfo = data->dinfo;
+  SCM seekto, newpos, port = dinfo->port;
   size_t bytes_read;
 
-  seekto = gdbscm_scm_from_ulongest (data->memaddr - disasm_data->offset);
+  seekto = gdbscm_scm_from_ulongest (data->memaddr - dinfo->offset);
   newpos = scm_seek (port, seekto, scm_from_int (SEEK_SET));
   if (!scm_is_eq (seekto, newpos))
     return "seek error";
@@ -108,13 +108,15 @@ gdbscm_disasm_read_memory (bfd_vma memaddr, bfd_byte *myaddr,
 			   unsigned int length,
 			   struct disassemble_info *dinfo)
 {
+  gdbscm_disassembler *self
+    = static_cast<gdbscm_disassembler *> (dinfo->application_data);
   struct gdbscm_disasm_read_data data;
   const char *status;
 
   data.memaddr = memaddr;
   data.myaddr = myaddr;
   data.length = length;
-  data.dinfo = dinfo;
+  data.dinfo = self;
 
   status = gdbscm_with_guile (gdbscm_disasm_read_memory_worker, &data);
 
@@ -123,30 +125,13 @@ gdbscm_disasm_read_memory (bfd_vma memaddr, bfd_byte *myaddr,
   return status != NULL ? -1 : 0;
 }
 
-/* disassemble_info.memory_error_func for gdbscm_print_insn_from_port.
-   Technically speaking, we don't need our own memory_error_func,
-   but to not provide one would leave a subtle dependency in the code.
-   This function exists to keep a clear boundary.  */
-
-static void
-gdbscm_disasm_memory_error (int status, bfd_vma memaddr,
-			    struct disassemble_info *info)
-{
-  memory_error (TARGET_XFER_E_IO, memaddr);
-}
-
-/* disassemble_info.print_address_func for gdbscm_print_insn_from_port.
-   Since we need to use our own application_data value, we need to supply
-   this routine as well.  */
-
-static void
-gdbscm_disasm_print_address (bfd_vma addr, struct disassemble_info *info)
+gdbscm_disassembler::gdbscm_disassembler (struct gdbarch *gdbarch,
+					  struct ui_file *stream,
+					  SCM port_, ULONGEST offset_)
+  : gdb_disassembler (gdbarch, stream),
+    port (port_), offset(offset_)
 {
-  struct gdbscm_disasm_data *data
-    = (struct gdbscm_disasm_data *) info->application_data;
-  struct gdbarch *gdbarch = data->gdbarch;
-
-  print_address (gdbarch, addr, (struct ui_file *) info->stream);
+  set_read_memory_func (gdbscm_disasm_read_memory);
 }
 
 /* Subroutine of gdbscm_arch_disassemble to simplify it.
@@ -164,30 +149,9 @@ gdbscm_print_insn_from_port (struct gdbarch *gdbarch,
 			     SCM port, ULONGEST offset, CORE_ADDR memaddr,
 			     struct ui_file *stream, int *branch_delay_insns)
 {
-  struct disassemble_info di;
-  int length;
-  struct gdbscm_disasm_data data;
-
-  di = gdb_disassemble_info (gdbarch, stream);
-  data.gdbarch = gdbarch;
-  data.port = port;
-  data.offset = offset;
-  di.application_data = &data;
-  di.read_memory_func = gdbscm_disasm_read_memory;
-  di.memory_error_func = gdbscm_disasm_memory_error;
-  di.print_address_func = gdbscm_disasm_print_address;
-
-  length = gdbarch_print_insn (gdbarch, memaddr, &di);
-
-  if (branch_delay_insns)
-    {
-      if (di.insn_info_valid)
-	*branch_delay_insns = di.branch_delay_insns;
-      else
-	*branch_delay_insns = 0;
-    }
+  gdbscm_disassembler di (gdbarch, stream, port, offset);
 
-  return length;
+  return di.print_insn (memaddr, branch_delay_insns);
 }
 
 /* (arch-disassemble <gdb:arch> address
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 7c0e39f..45392bc 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -698,7 +698,6 @@ btrace_insn_history (struct ui_out *uiout,
 {
   struct ui_file *stb;
   struct cleanup *cleanups, *ui_item_chain;
-  struct disassemble_info di;
   struct gdbarch *gdbarch;
   struct btrace_insn_iterator it;
   struct btrace_line_range last_lines;
@@ -711,7 +710,7 @@ btrace_insn_history (struct ui_out *uiout,
   gdbarch = target_gdbarch ();
   stb = mem_fileopen ();
   cleanups = make_cleanup_ui_file_delete (stb);
-  di = gdb_disassemble_info (gdbarch, stb);
+  gdb_disassembler di (gdbarch, stb);
   last_lines = btrace_mk_line_range (NULL, 0, 0);
 
   make_cleanup_ui_out_list_begin_end (uiout, "asm_insns");
@@ -773,7 +772,7 @@ btrace_insn_history (struct ui_out *uiout,
 	  if ((insn->flags & BTRACE_INSN_FLAG_SPECULATIVE) != 0)
 	    dinsn.is_speculative = 1;
 
-	  gdb_pretty_print_insn (gdbarch, uiout, &di, &dinsn, flags, stb);
+	  di.pretty_print_insn (uiout, &dinsn, flags);
 	}
     }
 
-- 
2.5.5


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-12-16 16:40 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-12 10:48 [RFC] PR 20939: Handle error in disassembly Yao Qi
2016-12-14 17:31 ` Pedro Alves
2016-12-14 22:05   ` Yao Qi
2016-12-16 16:40     ` Pedro Alves

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).