Re: [PATCH 1/2] Add support for setting disassembler-options in GDB for POWER, ARM and S390

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

Hmmm, sorry to get back to you on this, but the first time I
saw your note, I only saw your comment regarding the gdb.texinfo
issue and I missed everything else you said. :-(


On 1/25/17 8:23 AM, Yao Qi wrote:
>> +@table @code
>> +@kindex set disassembler-options
> 
> Looks "@kindex set disassembly-flavor" is removed by mistake.

Fixed, thanks.


>> +extern const char **disassembler_options_names_powerpc (void);
>> +extern const char **disassembler_options_names_arm (void);
>> +extern const char **disassembler_options_desc_arm (void);
>> +extern const char **disassembler_options_names_s390 (void);
>> +extern const char **disassembler_options_desc_s390 (void);
> 
> We end up having three things from opcodes for disassembler in each
> target, the disassembler_ftype function pointer, the options names,
> and options descs.  Every new added target needs to implement these
> three things, and hook them correctly with gdb and objdump.
> 
> Why don't we merge functions "_names_" and "_desc_" into one function,

Well not all architectures have the _desc_ strings (ie, powerpc),
but I guess that those types of arches can just set the description
pointer to NULL.  Ok, I'll give that a try.



>>  typedef struct
>>  {
>>    const char *name;
>> +  const char *long_name;
> 
> Is this field necessary?  "reg-names-"$name is the long_name, isn't?

The option name scanning is done in architecture independent code,
so it needs the long name which it gets via the call to
disassembler_options_names_arm().  The old "set disassembler ..."
command on the other hand uses the short name, so for backward
compatibility with the old command, I need the short name.

That said, the only use of the short name is in get_arm_regnames
and parse_arm_disassembler_option.  I suppose I could skip over
the reg-names- part of the long name instead, which would allow
us to only need the long version of name.  I'll see if it's doable.
Obviously, I'd just use .name for the field name.




>> +const char **
>> +disassembler_options_names_arm (void)
>> +{
>> +  static const char **options = NULL;
>> +
>> +  if (options == NULL)
>> +    {
>> +      size_t i;
>> +      size_t num_options = NUM_ARM_REGNAMES + 2;
>> +      options = XNEWVEC (const char *, num_options + 1);
>> +      for (i = 0; i < NUM_ARM_REGNAMES; i++)
>> +	options[i] = regnames[i].long_name;
> 
> We can get options from .name,
> 
>            options[i] = xmalloc (sizeof ("reg-names-") + strlen (regnames[i].name) + 1);
>            strcpy (options[i], "reg-names-");
>            strcat (options[i], strlen (regnames[i].name));

This is moot if I can combine the option names as I described above.




>>  void
>>  print_arm_disassembler_options (FILE *stream)
>>  {
> 
> print_arm_disassembler_options can be adjusted as what you changed to
> print_s390_disassembler_options, otherwise, we may have inconsistency
> in the future.

Can you be more specific about what I did to the s390 code you want
done to the arm code?  I'll note I have changed things due to Alan
wanting the strings to support internationalization.



>>  static void
>>  set_disassembly_style_sfunc (char *args, int from_tty,
>> -			      struct cmd_list_element *c)
>> +			     struct cmd_list_element *c)
>> +{
>> +  /* Convert the short style name into the long style name (eg, reg-names-*)
>> +     before calling the generic set_disassembler_options() function.  */
>> +  char long_name[256], *style = long_name;
>> +  snprintf (style, 256, "reg-names-%s", *(char **)c->var);
> 
> You can get the option set in command from disassembly_style, so you
> don't have to access c->var, and no need to include "cli/cli-decode.h".
>>
>> +  c->var = &style;
>> +  set_disassembler_options (args, from_tty, c);
> 
> We usually don't relay the command call this way.  It is fragile, IMO.
> Can you factor out a function from set_disassembler_options, which
> accept "normalized" options, and both set_disassembly_style_sfunc
> and set_disassembler_options can call it.

I have an idea, let me play with it.



> One problem I can see is that "set arm disassembler" overwrite
> "disassembler-options",
> 
> (gdb) set disassembler-options reg-names-gcc,no-force-thumb
> (gdb) show disassembler-options 
> The current disassembler options are 'reg-names-gcc,no-force-thumb'
> 
> ....
> (gdb) set arm disassembler std
> (gdb) show disassembler-options 
> The current disassembler options are 'reg-names-std'
> 
> "no-force-thumb" is disappeared.

Yes, this is as expected, because...


> I don't know how to fix it.  Looks command "disassembler-options" is a
> super set of "arm disassembler".

Yes, "disassembler-options" is a superset of "arm disassembler", that
is why "arm disassembler" is built on top of "disassembler-options".
If you change the disassembly style using one of the commands, the other
command had better agree, otherwise things are just broken.

Now given that disassembler-options is the more feature rich interface,
you're probably better off using it rather than the older "arm disassembler"
interface.  But we do want them to work together.



>> +/* Remove whitespace and consecutive commas from OPTIONS.  */
>> +
>> +char *
>> +cleanup_disassembler_options (char *options)
> 
> "cleanup" is a special term in gdb, so better to rename it,
> 
> remove_whitespace_and_comma or parse_preparation, for example?

Ok, I'll change it to not use cleanup.




>> +void
>> +set_disassembler_options (char *args, int from_tty, struct cmd_list_element *c)
>> +{
>> +  struct gdbarch *gdbarch = get_current_arch ();
> 
> "set disassembler-options" applies to the current gdbarch, rather than a
> global setting, but we can't tell this from the command name.  GDB has
> multiple instances of gdbarch, and disassembler-options is arch-specific.
> We may set disassembler-options when gdbarch is A, but disassembler-options
> become something different when current gdbarch is B.

Pedro and I discussed this already here:

  https://sourceware.org/ml/gdb-patches/2016-10/msg00806.html

If you set the disassembler-options for A and then switch to B, you
don't want A's disassembler-options when disassembling for B.
Then if you switch back to A, your options will be restored.


> However, I am not sure "set $ARCH disassembler-options" is good idea or not.

We discussed this too and ended up deciding not to have the arch specific
command and use a generic command that all the arches would use.

Peter