Re: [PATCH, RFC] Add support for choosing disassembler cpu in GDB for POWER.

[Peter Bergner <bergner@vnet.ibm.com>]

On 9/30/16 11:19 AM, Ulrich Weigand wrote:
> The implementation in the patch does appear to be a bit ad-hoc, however :-)
> Why would we want to pass that information via a new global variable, if
> there is already an element "disassembler_options" in the struct
> disassemble_info that GDB passes to bfd?  See e.g. i386_print_insn.

Yes, I saw that code.  The problem is that same solution won't work for
us, since print_insn_*() doesn't look at info->disassembler_options
at all.  Instead, it uses the dialect value which we initialize from
info->disassembler_options in powerpc_init_dialect(), but that happens
only once and well before gdb_print_insn_powerpc() is called.  That means
changing info->disassembler_options in gdb_print_insn_powerpc() won't
affect anything.

Now we could recompute the dialect each time we call gdb_print_insn_powerpc(),
but that is done for each insn we emit and scanning through the ppc_opts table
every time would be very expensive, since our table is pretty big.

The only other method would be for set_disassembler_cpu() to somehow
convert its arg into a dialect value that gdb_print_insn_powerpc()
could stuff into the struct disassemble_info each time, which would
be fairly inexpensive.  However, the dialect value is computed by
powerpc_init_dialect which takes a struct disassemble_info pointer
and at this point in the disassembly process, it hasn't been created
yet.  I suppose we could try and create a fake one to pass in.

So given that gdb_print_insn_powerpc() can't affect the disassembly
cpu by changing info->disassembler_options and set_disassembler_cpu()
isn't passed a struct disassemble_info pointer,that's why I resorted
to the global variable.

Given your reaction to the patch as is, how about if I try and create
a temporary struct disassemble_info to pass to powerpc_init_dialect()
and stash the resulting dialect in a global var that is private to
gdb so that gdb_print_insn_powerpc() can stuff it into into the
struct disassemble_info?  Would that satisfy your reservations?

Note to self, we should probably sort the ppc_opts table with the more
commonly used (ie, newer) cpus listed first rather than adding them at
the end of the table like we have.


> I'm also not really happy about the tight integration of opcode/ppc.h
> and the ppc_opts struct into GDB code ...  Can't we ask a BFD routine
> whether a particular CPU option is valid?  E.g. by just making a "test"
> call to print_insn_* and see if it succeeds?

As I said above, print_insn_*() doesn't look at info->disassembler_options,
so we can't use that method, but I could add a new BFD routine to do what
you ask.  I did it this way, so I could emit the valid cpus values in the
"show disassembler-cpu" output.  I suppose I could also move that to BFD
and just call it from GDB.


> Apart from those implementation details, I'm wondering whether we might
> want to generalize the feature to allow setting any disassembler option,
> not just CPU levels.  Also, this could really be useful on any platform,
> not just Power :-)  But I see that some other architectures already use
> info->disassembler_options to pass some special options, which might
> make the generic solution more complex.  Therefore I'd be OK with just
> doing the Power implementation for now.

I'm not against it, but as it is today, the POWER port only expects
info->disassembler_options to hold cpu values.  If we were to allow
other options, we'd have to parse them more carefully.  I'm also not
sure whether like the disassembly dialect, if we might have problems
changing the values due to when they get parsed versus when they are
used.

Peter