Patchwork [v3] arc: Select CPU model properly before disassembling

login
register
mail settings
Submitter Anton Kolesov
Date June 15, 2017, 1:20 p.m.
Message ID <20170615132040.11554-1-Anton.Kolesov@synopsys.com>
Download mbox | patch
Permalink /patch/21037/
State New
Headers show

Comments

Anton Kolesov - June 15, 2017, 1:20 p.m.
Changes in V3:

* Minor formatting fix.
* Do not assign disassemble_info->section if it is already set.
* Free arc_disassembler_options before assignment.

Changes in V2:

* Use xstrprintf instead of string_printf
* Reinstate arc_delayed_print_insn as a print instruction function for ARC.
* Change arc_delayed_print_insn to use default_print_insn.
* Change commit description accordingly.

---

Enforce CPU model for disassembler via its options, if it was specified in XML
target description, otherwise use default method of determining CPU implemented
in disassembler - scanning ELF private header.  The latter requires
disassemble_info->section to be properly initialized.  To make sure that
info->section is set in all cases this patch partially reverts [1] for ARC: it
reinstates arc_delayed_print_insn as a "print_insn" function for ARC, but
now this function only sets disassemble_info->section and then calls
default_print_insn to do the rest of the job.

Support for CPU in disassembler options for ARC has been added in [2].

[1] https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=39503f82427e22ed8e04d986ccdc8562091ec62e
[2] https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=10045478d984f9924cb945423388ba25b7dd3ffe

gdb/ChangeLog:

yyyy-mm-dd  Anton Kolesov  <anton.kolesov@synopsys.com>

	* arc-tdep.c (arc_disassembler_options): New variable.
	(arc_gdbarch_init): Set and use it. Use arc_delayed_print_insn instead
	of default_print_insn.
	(arc_delayed_print_insn): Set info->section when needed,
	use default_print_insn to retrieve a disassembler.
---
 gdb/arc-tdep.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 57 insertions(+), 6 deletions(-)
Simon Marchi - June 15, 2017, 9:12 p.m.
On 2017-06-15 15:20, Anton Kolesov wrote:
> @@ -2041,6 +2067,31 @@ arc_gdbarch_init (struct gdbarch_info info,
> struct gdbarch_list *arches)
>    if (tdep->jb_pc >= 0)
>      set_gdbarch_get_longjmp_target (gdbarch, arc_get_longjmp_target);
> 
> +  /* Disassembler options.  Enforce CPU if it was specified in XML 
> target
> +     description, otherwise use default method of determining CPU (ELF 
> private
> +     header).  */
> +  if (info.target_desc != NULL)
> +    {
> +      const struct bfd_arch_info *tdesc_arch
> +	= tdesc_architecture (info.target_desc);
> +      if (tdesc_arch != NULL)
> +	{
> +	  free (arc_disassembler_options);

Nit: Use xfree instead of free.

You can fix that and push directly.

Thanks!

Simon
Anton Kolesov - June 16, 2017, 12:03 p.m.
> > +     header).  */
> > +  if (info.target_desc != NULL)
> > +    {
> > +      const struct bfd_arch_info *tdesc_arch
> > +	= tdesc_architecture (info.target_desc);
> > +      if (tdesc_arch != NULL)
> > +	{
> > +	  free (arc_disassembler_options);
> 
> Nit: Use xfree instead of free.
> 
> You can fix that and push directly.

Pushed, thanks for the review!

Anton


> 
> Thanks!
> 
> Simon

Patch

diff --git a/gdb/arc-tdep.c b/gdb/arc-tdep.c
index d9ee5c6..374e31d 100644
--- a/gdb/arc-tdep.c
+++ b/gdb/arc-tdep.c
@@ -145,6 +145,8 @@  static const char *const core_arcompact_register_names[] = {
   "lp_count", "reserved", "limm", "pcl",
 };
 
+static char *arc_disassembler_options = NULL;
+
 /* Functions are sorted in the order as they are used in the
    _initialize_arc_tdep (), which uses the same order as gdbarch.h.  Static
    functions are defined before the first invocation.  */
@@ -1405,12 +1407,34 @@  arc_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
 int
 arc_delayed_print_insn (bfd_vma addr, struct disassemble_info *info)
 {
-  int (*print_insn) (bfd_vma, struct disassemble_info *);
-  /* exec_bfd may be null, if GDB is run without a target BFD file.  Opcodes
-     will handle NULL value gracefully.  */
-  print_insn = arc_get_disassembler (exec_bfd);
-  gdb_assert (print_insn != NULL);
-  return print_insn (addr, info);
+  /* Standard BFD "machine number" field allows libocodes disassembler to
+     distinguish ARC 600, 700 and v2 cores, however v2 encompasses both ARC EM
+     and HS, which have some difference between.  There are two ways to specify
+     what is the target core:
+     1) via the disassemble_info->disassembler_options;
+     2) otherwise libopcodes will use private (architecture-specific) ELF
+     header.
+
+     Using disassembler_options is preferable, because it comes directly from
+     GDBserver which scanned an actual ARC core identification info.  However,
+     not all GDBservers report core architecture, so as a fallback GDB still
+     should support analysis of ELF header.  The libopcodes disassembly code
+     uses the section to find the BFD and the BFD to find the ELF header,
+     therefore this function should set disassemble_info->section properly.
+
+     disassembler_options was already set by non-target specific code with
+     proper options obtained via gdbarch_disassembler_options ().
+
+     This function might be called multiple times in a sequence, reusing same
+     disassemble_info.  */
+  if ((info->disassembler_options == NULL) && (info->section == NULL))
+    {
+      struct obj_section *s = find_pc_section (addr);
+      if (s != NULL)
+	info->section = s->the_bfd_section;
+    }
+
+  return default_print_insn (addr, info);
 }
 
 /* Baremetal breakpoint instructions.
@@ -2013,6 +2037,8 @@  arc_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   set_gdbarch_frame_align (gdbarch, arc_frame_align);
 
+  set_gdbarch_print_insn (gdbarch, arc_delayed_print_insn);
+
   set_gdbarch_cannot_step_breakpoint (gdbarch, 1);
 
   /* "nonsteppable" watchpoint means that watchpoint triggers before
@@ -2041,6 +2067,31 @@  arc_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   if (tdep->jb_pc >= 0)
     set_gdbarch_get_longjmp_target (gdbarch, arc_get_longjmp_target);
 
+  /* Disassembler options.  Enforce CPU if it was specified in XML target
+     description, otherwise use default method of determining CPU (ELF private
+     header).  */
+  if (info.target_desc != NULL)
+    {
+      const struct bfd_arch_info *tdesc_arch
+	= tdesc_architecture (info.target_desc);
+      if (tdesc_arch != NULL)
+	{
+	  free (arc_disassembler_options);
+	  /* FIXME: It is not really good to change disassembler options
+	     behind the scene, because that might override options
+	     specified by the user.  However as of now ARC doesn't support
+	     `set disassembler-options' hence this code is the only place
+	     where options are changed.  It also changes options for all
+	     existing gdbarches, which also can be problematic, if
+	     arc_gdbarch_init will start reusing existing gdbarch
+	     instances.  */
+	  arc_disassembler_options = xstrprintf ("cpu=%s",
+						 tdesc_arch->printable_name);
+	  set_gdbarch_disassembler_options (gdbarch,
+					    &arc_disassembler_options);
+	}
+    }
+
   tdesc_use_registers (gdbarch, tdesc, tdesc_data);
 
   return gdbarch;