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

[Pedro Alves <palves at redhat dot com>]

On 02/22/2017 09:23 PM, Peter Bergner wrote:

> The target architecture is assumed to be s390:31-bit
> (gdb) show disassembler-options    
> The current disassembler options are 'esa'
> [snip]
> 
> (gdb) set architecture powerpc:common64 
> The target architecture is assumed to be powerpc:common64
> (gdb) show disassembler-options 
> The current disassembler options are 'power8'
> [snip]

Given this was broken but went unnoticed before, something like
the above should be added as a test.

> 
> Ok for committing now?

Not yet, but close.  Sorry about that.

There's a couple issues that I think need fixing, one
that looks like a crasher.

And then while at it, I spotted a few other nits.

> +/* A helper function for FOR_EACH_DISASSEMBLER_OPTION.  */
> +static inline char *
> +next_disassembler_option (char *options)
> +{
> +  char *opt = strchr (options, ',');
> +  if (opt != NULL)
> +    opt++;
> +  return opt;
> +}
> +
> +/* A macro for iterating over each comma separated option in OPTIONS.  */
> +#define FOR_EACH_DISASSEMBLER_OPTION(OPT, OPTIONS) \
> +  for ((OPT) = (typeof (OPT))(OPTIONS); \

typeof is a GNU extension.  I don't see any other use of it in the
tree.  I don't know whether all compilers that binutils intents to
support accept it.

But why do you need the cast?

> +       (OPT) != NULL; \
> +       (OPT) = next_disassembler_option (OPT))
> +
>  
>  /* This block of definitions is for particular callers who read instructions
>     into a buffer before calling the instruction decoder.  */


> +const disasm_options_t *
> +disassembler_options_powerpc (void)
> +{
> +  static disasm_options_t *opts = NULL;
> +
> +  if (opts == NULL)
> +    {
> +      size_t i, num_options = sizeof (ppc_opts) / sizeof (ppc_opts[0]);

While at it, all new occurrences of this
"sizeof (array) / sizeof (array[0])" idiom should really be
replaced by "ARRAY_SIZE (array)" instead.

> +      opts = XNEW (disasm_options_t);
> +      opts->name = XNEWVEC (const char *, num_options + 1);
> +      for (i = 0; i < num_options; i++)
> +	opts->name[i] = ppc_opts[i].opt;
> +      /* The array we return must be NULL terminated.  */
> +      opts->name[i] = NULL;
> +      opts->description = NULL;
> +    }
> +
> +  return opts;
> +}


>  @table @code
> +@kindex set disassembler-options
> +@cindex disassembler options
> +@item set disassembler-options @var{option1}[,@var{option2}@dots{}]
> +This command controls the passing of target specific information to the
> +disassembler.  For a list of valid options, please refer to the
> +@code{-M}/@code{--disassembler-options} section of the @samp{objdump}
> +manual and/or the output of @kbd{objdump --help}.  The default value
> +is empty string.

Missing "the" in "is the empty string".

While at it, I think it'd be nice to add a ref to the section in binutils
manual directly.  We have a few like that already in the manual.  For
example, we have:

(@pxref{Overlay Description,,, ld.info, Using ld: the GNU linker})

> @@ -780,6 +787,7 @@ gdb_disassembler::gdb_disassembler (struct gdbarch *gdbarch,
>    m_di.endian = gdbarch_byte_order (gdbarch);
>    m_di.endian_code = gdbarch_byte_order_for_code (gdbarch);
>    m_di.application_data = this;
> +  m_di.disassembler_options = *gdbarch_disassembler_options (gdbarch);

Isn't this going to crash on archs that don't set gdbarch_disassembler_options?

Do the "gdb.base/all-architectures-*.exp" tests pass on an
--enable-targets=all build of gdb with this patch?

>    disassemble_init_for_target (&m_di);
>  }
>  

> +static void
> +show_disassembler_options_sfunc (struct ui_file *file, int from_tty,
> +				 struct cmd_list_element *c, const char *value)
> +{


> +  else
> +    {
> +      size_t i, col;
> +      for (i = 0, col = 0; valid_options->name[i] != NULL; i++)
> +	{
> +	  /* Include the " " and "," we print below.  */
> +	  size_t len = strlen (valid_options->name[i]) + 2;
> +	  if (col + len > 80)

Is this 80 here supposed to be the screen width?  Do we
really need this manual wrapping?

> +	    {
> +	      fprintf_filtered (file, "\n");
> +	      col = 0;
> +	    }
> +	  if (col == 0)
> +	    fprintf_filtered (file, "  %s", valid_options->name[i]);
> +	  else
> +	    fprintf_filtered (file, ", %s", valid_options->name[i]);
> +	  col += len;
> +	}
> +      fprintf_filtered (file, "\n");
> +    }
> +}
> +

> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 88ed391..7cdb23c 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -145,9 +145,6 @@ static const char *const arm_mode_strings[] =
>  static const char *arm_fallback_mode_string = "auto";
>  static const char *arm_force_mode_string = "auto";
>  
> -/* Number of different reg name sets (options).  */
> -static int num_disassembly_options;
> -
>  /* The standard register names, and all the valid aliases for them.  Note
>     that `fp', `sp' and `pc' are not added in this alias list, because they
>     have been added as builtin user registers in
> @@ -208,6 +205,9 @@ static const char *const arm_register_names[] =
>   "f4",  "f5",  "f6",  "f7",	/* 20 21 22 23 */
>   "fps", "cpsr" };		/* 24 25       */
>  
> +/* Holds the current set of options to be passed to the disassembler.  */
> +static char *disassembler_options;

Even though these are going to be statics, can you please name
them $cpu_disassembler_options, etc.  It'll make debugging
gdb easier.  "(gdb) print arm_disassembler_options", etc.

> +static void
> +show_disassembly_style_sfunc (struct ui_file *file, int from_tty,
> +			      struct cmd_list_element *c, const char *value)
> +{
> +  struct gdbarch *gdbarch = get_current_arch ();
> +  const char *options = *gdbarch_disassembler_options (gdbarch);
> +  const char *style = "";
> +  int len = 0;
> +  char *opt;
> +
> +  FOR_EACH_DISASSEMBLER_OPTION (opt, options)
> +    {
> +      if (CONST_STRNEQ (opt, "reg-names-"))
> +	{
> +	  style = &opt[strlen ("reg-names-")];
> +	  len = strcspn(style, ",");

missing space before parens.

> +	}

> +    }
> +
> +  fprintf_unfiltered (file, "The disassembly style is \"%.*s\".\n", len, style);
>  }
>  

Thanks,
Pedro Alves