RE: [PATCH] arc: Select CPU model properly before disassembling

[Anton Kolesov <Anton dot Kolesov at synopsys dot com>]

Hi Simon

> >
> > 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
 
> Hi Anton,
> 
> This is not very clear to me (I haven't really touched disassembly yet),
> so let me train to summarize the situation.  Let's say you feed the
> right executable to GDB, but connect to a gdbserver that doesn't report
> the cpu arch in the target description.  arc_disassembler_options won't
> be set, so we won't pass an explicit disassembler option.  opcodes
> internals could try to fall back on data from the BFD if
> disassemble_info::section was set.  However, this is only done in
> arc_delayed_print_insn, which is not used in that case (it was removed
> as a gdbarch callback in commit [2]).  Does that sound right?

Yes, your understanding is correct. For ARC this change will not be a
regression, because today disassembler never selects properly between ARC EM
and HS at all cases - this patch will fix some cases, but not the others, that
why I feel this makes sense.

> 
> Could gdb_disassembler set the section field of disassemble_info itself?
>   What you are doing to set the section field here is not ARC-specific,
> it looks like it could potentially help other architectures to have that
> field set.
> 
> Other places that use the disassembler would have to set it too, for
> example in the object prepared by the arc_disassemble_info function.

I think this info->section initialization code can be moved to
arch-utils.c:default_print_insn, however I'm not sure if that wouldn't cause
any troubles with other architectures. Doing initialization in gdb_disassembler
constructor is complicated, because we don't really know what would be the
disassembled address at this stage, hence what would be the section.
Gdb_dissassembler construction can use .text section as a default, but then
might now work with some multi-arch ELFs, I presume (unlikely to be a
problem for ARC, though).

Another option is, of course, to partially revert [2] for ARC - make
arc_delayed_print_insn a printer for ARC, but change it, so it will only
set info->section and then call default_print_insn. I believe that same change
is needed in mep-tdep.c. At least for ARC we would need to use print_insn
anyway to disassemble, because opcodes/arc-dis.c:arc_insn_decode relies on it.

> > +	  std::string disasm_options
> > +	    = string_printf ("cpu=%s", tdesc_arch->printable_name);
> > +	  arc_disassembler_options = xstrdup (disasm_options.c_str ());
> 
> You could shorten this by using xstrprintf directly.

Will do in v2.

Anton

> 
> > +	  set_gdbarch_disassembler_options (gdbarch,
> > +					    &arc_disassembler_options);
> > +	}
> > +    }
> > +
> >    tdesc_use_registers (gdbarch, tdesc, tdesc_data);
> >
> >    return gdbarch;
> 
> This part, which sets the disassembler options based on the target
> description, looks good to me.
> 
> Simon