This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 02/13/2017 06:47 PM, Peter Bergner wrote:
> On 2/13/17 9:52 AM, Yao Qi wrote:
>> Peter Bergner <bergner@vnet.ibm.com> writes:
>> Function get_arm_regname_num_options is only used in gdb.  Since we've
>> have disassembler_options_arm, we can use it in gdb and remove
>> get_arm_regname_num_options.
>>
>> We use get_arm_regname_num_options in arm-tdep.c,
>>
>>    /* Get the number of possible sets of register names defined in opcodes.  */
>>    num_disassembly_options = get_arm_regname_num_options ();
>>
>> We can get 'num_disassembly_options' by iterating options from
>> disassembler_options_arm.
> 
> Done.
> 
> 
>> Likewise, we can use disassembler_options_arm in gdb and remove
>> get_arm_regnames.  We use get_arm_regnames like this in arm-tdep.c,
>>
>>   for (i = 0; i < num_disassembly_options; i++)
>>     {
>>       get_arm_regnames (i, &setname, &setdesc);
>>       valid_disassembly_styles[i] = setname;
>>       length = snprintf (rdptr, rest, "%s - %s\n", setname, setdesc);
>>       rdptr += length;
>>       rest -= length;
>>     }
>>
>> but we can replace it by disassembler_options_arm instead.
> 
> Done.
> 
> 
> 
>>> +# Functions for allowing a target to modify its disassembler options.
>>> +v:char *:disassembler_options:::0:0::0:pstring (gdbarch->disassembler_options)
>>
>> These options should be modeled as per-architecture data.  We need to
>> define a key to access that data dynamically.  grep
>> "static struct gdbarch_data *" in *.c.
> 
> Not done, as from Pedro's note, it sounded like he was arguing against
> this review suggestion.  I agree with that, since I think users would
> expect 9I know I would) that setting the disassembler_options would be
> persistent across their debug session.
> 
> 
>>> +v:const disasm_options_t *:disassembler_options_arch:::0:0::0:host_address_to_string (gdbarch->disassembler_options_arch->name)
>>
>> disassembler_options_arch is not clear to me, and I feel
>> gdbarch_disassembler_options_arch is even worse.  How about renaming it
>> to "disassembler_options_supported" or "valid_disassembler_options"?
> 
> Changed to valid_disassembler_options.
> 
> 
> Here is an updated patch with the above changes.  I'll note that I
> did not change the existing behavior of ARM defaulting to reg-names-gcc
> when disassembling with objdump, while gdb defaults to reg-names-std.

FYI, I tried applying the patch locally, to play with it a bit,
but it failed with "fatal: corrupt patch at line 178".

A couple suggestions for the future:

 - Maintain the intended git commit log as an integral part
   of the patch, and include it in patch re-posts, so that
   any revision of the patch can be reviewed as a
   self-contained entity.  There's probably some rationale for
   some changes to the tests that is written down in some
   earlier intro, but was lost meanwhile.

 - For each new revision of the patch, bump a v2, v3, etc.
   revision number in the email subject, so that's easier
   to find specific revisions, and to identify which
   email contains the latest version.

TIA :-)

Some comments on the patch follow.  (When the same comment
would apply in multiple places, I only commented once.)

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index a969d1b..26abd9c 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -8516,6 +8516,25 @@ location of the relocation table.  On some architectures, @value{GDBN}
>  might be able to resolve these to actual function names.
>  
>  @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 ''.

'' renders as a single double-quote, not two single-quotes.  In any
case, I'd suggest saying instead "The default is the empty string", or
"The default is not options".  Likewise in NEWS.

> +
> +If it is necessary to specify more than one disassembler option, then
> +multiple options can be placed together into a comma separated list.
> +Currently this command is only supported on targets ARM, PowerPC
> +and S/390.
> +
> +@kindex show disassembler-options
> +@item show disassembler-options
> +Show the current setting of the disassembler options.
> +@end table
> +
> +@table @code
>  @kindex set disassembly-flavor
>  @cindex Intel disassembly flavor
>  @cindex AT&T disassembly flavor
> diff --git a/gdb/disasm.c b/gdb/disasm.c
> index 64d6684..43ee2fb 100644

> @@ -888,6 +896,8 @@ gdb_buffered_insn_length_init_dis (struct gdbarch *gdbarch,
>    di->endian = gdbarch_byte_order (gdbarch);
>    di->endian_code = gdbarch_byte_order_for_code (gdbarch);
>  
> +  di->application_data = gdbarch;

Is this above used anywhere?

> +  di->disassembler_options = gdbarch_disassembler_options (gdbarch);
>    disassemble_init_for_target (di);
>  }
>  
> @@ -904,3 +914,166 @@ gdb_buffered_insn_length (struct gdbarch *gdbarch,
>  
>    return gdbarch_print_insn (gdbarch, addr, &di);
>  }
> +
> +void
> +set_disassembler_options (char *prospective_options)
> +{
> +  struct gdbarch *gdbarch = get_current_arch ();
> +  const disasm_options_t *valid_options;
> +  char *options = remove_whitespace_and_extra_commas (prospective_options);
> +  char *iter, opt[256];

Can we get rid of the hardcoded (and not enforced) limit?
Maybe just use strtok_r instead of FOR_EACH_DISASSEMBLER_OPTION?

> +  /* Verify we have valid disassembler options.  */
> +  FOR_EACH_DISASSEMBLER_OPTION (opt, iter, options)
> +    {
> +      size_t i;
> +      for (i = 0; valid_options->name[i] != NULL; i++)
> +	if (strcmp (opt, valid_options->name[i]) == 0)
> +	  break;
> +      if (valid_options->name[i] == NULL)
> +	{
> +	  fprintf_filtered (gdb_stdlog,
> +			    _("Invalid disassembler option value: '%s'.\n"),
> +			    opt);
> +	  return;
> +	}
> +    }
> +
> +  free (gdbarch_disassembler_options (gdbarch));
> +  set_gdbarch_disassembler_options (gdbarch, xstrdup (options));
> +}
> +


> +/* A completion function for "set disassembler".  */
> +
> +static VEC (char_ptr) *
> +disassembler_options_completer (struct cmd_list_element *ignore,
> +				const char *text, const char *word)
> +{
> +  struct gdbarch *gdbarch = get_current_arch ();
> +  const disasm_options_t *opts = gdbarch_valid_disassembler_options (gdbarch);
> +
> +  if (opts != NULL)
> +    {
> +      /* Only attempt to complete on the last option text.  */
> +      const char *separator = strrchr (text, ',');
> +      if (separator != NULL)
> +	text = separator + 1;

I believe 'word' points past the comma already?

> +      while (ISSPACE (*text))
> +	text++;

skip_spaces_const.  

> +      return complete_on_enum (opts->name, text, word);
> +    }
> +  return NULL;
> +}
> +


> +void
> +_initialize_disasm (void)
> +{
> +  struct cmd_list_element *cmd;
> +
> +  /* Add the command that controls the disassembler options.  */
> +  cmd = add_setshow_string_noescape_cmd ("disassembler-options", no_class,
> +					 &prospective_options, _("\
> +Set the disassembler options.\n\
> +Usage: set disassembler <option>[,<option>...]\n\n\
> +See: show disassembler' for valid option values.\n"), _("\

Spell out "set/show disassembler-options" in full.

> +Show the disassembler options."), NULL,
> +					 set_disassembler_options_sfunc,
> +					 show_disassembler_options_sfunc,
> +					 &setlist, &showlist);
> +  set_cmd_completer (cmd, disassembler_options_completer);
> +}
> diff --git a/gdb/disasm.h b/gdb/disasm.h
> index 42c1f3a..4d29446 100644
> --- a/gdb/disasm.h
> +++ b/gdb/disasm.h
> @@ -138,4 +138,6 @@ extern int gdb_buffered_insn_length (struct gdbarch *gdbarch,
>  				     const gdb_byte *insn, int max_len,
>  				     CORE_ADDR memaddr);
>  
> +extern void set_disassembler_options (char *);

Need intro comment.

> +
>  #endif
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 54549b6..044743c 100755


> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 88ed391..635f88d 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
> @@ -218,7 +215,9 @@ static const char *disassembly_style;
>     style.  */
>  static void set_disassembly_style_sfunc(char *, int,
>  					 struct cmd_list_element *);
> -static void set_disassembly_style (void);
> +static void show_disassembly_style_sfunc (struct ui_file *, int,
> +					  struct cmd_list_element *,
> +					  const char *);
>  
>  static void convert_from_extended (const struct floatformat *, const void *,
>  				   void *, int);
> @@ -8539,9 +8538,34 @@ arm_show_force_mode (struct ui_file *file, int from_tty,
>  
>  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[64], *style = long_name;

No need for the separate pointer var, AFAICS, but...

> +  snprintf (style, 64, "reg-names-%s", disassembly_style);
> +  set_disassembler_options (style);

...how about avoiding the hard coded buffer size:

  std::string long_name = std::string ("reg-names-") + disassembly_style;
  set_disassembler_options (&long_name[0]);

> +}
> +
> +static void
> +show_disassembly_style_sfunc (struct ui_file *file, int from_tty,
> +			      struct cmd_list_element *c, const char *value)
>  {
> -  set_disassembly_style ();
> +  struct gdbarch *gdbarch = get_current_arch ();
> +  const char *options = gdbarch_disassembler_options (gdbarch);
> +  char *style = "default";

Why "default" instead of empty?

> +  char *iter, opt[64];
> +
> +  FOR_EACH_DISASSEMBLER_OPTION (opt, iter, options)
> +    {
> +      if (CONST_STRNEQ (opt, "reg-names-"))
> +	{
> +	  style = &opt[strlen ("reg-names-")];
> +	  break;
> +	}
> +    }
> +
> +  fprintf_unfiltered (file, "The disassembly style is \"%s\".\n", style);
>  }
>  
>  /* Return the ARM register name corresponding to register I.  */



> diff --git a/gdb/testsuite/gdb.arch/powerpc-altivec.exp b/gdb/testsuite/gdb.arch/powerpc-altivec.exp
> new file mode 100644
> index 0000000..8fd418e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/powerpc-altivec.exp
> @@ -0,0 +1,257 @@
> +# Copyright 2016 Free Software Foundation, Inc.

2016-2017.  Actually, if you're largely copying the file contents
from some other file, you'll need to preserve the original
copyright years in the new file (and include 2017).

> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.

Please use an updated GPLv3 copyright header.  Looks like a couple tests
sneaked in with the old v2 header.

> +
> +# Test PowerPC instructions disassembly.
> +
> +standard_testfile .s
> +set objfile [standard_output_file ${testfile}.o]
> +
> +if {![istarget "powerpc*-*-*"]} then {
> +    verbose "Skipping PowerPC instructions disassembly."
> +    return
> +}
> +
> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${objfile}" object {debug}] != "" } {
> +    untested "PowerPC instructions disassembly"
> +    return -1
> +}
> +
> +clean_restart ${objfile}
> +
> +# Disassemble the function.
> +
> +gdb_test "set disassembler-options altivec"
> +set test "disass func"
> +gdb_test_multiple $test $test {
> +    -re "\r\nDump of assembler code for function func:(\r\n.*\r\n)End of assembler dump.\r\n$gdb_prompt $" {
> +	set func $expect_out(1,string)
> +	pass $test
> +    }
> +}

If this test fails, $func is left unset, and then the following
tests that use it error out with a tcl error.  Set it to
some bogus default before calling gdb_test_multiple.

I understand that you're largely copying the mechanism
from an existing test, but, I should mention that this extracting
the function disassembly in one go seems fragile -- at some point
this can grow enough to overflow expect's buffer.

How about instead using gdb_test_sequence ?  Something like:

set insn_sequence_re ""

foreach insn {
  "dss     3"
  "dssall"
  "dst     r5,r4,1"
  ....
} {
    lappend insn_list_re [instr_to_patt $insn]
}

gdb_test_sequence "some test name" "" $insn_list_re

You could even go a step further and extract the
instruction list from the .s file, since you already
have the disassembled instructions written there:

 +	.long  0x117e0001    /* vmul10cuq v11,v30          */
 +	.long  0x13c1b807    /* vcmpneb v30,v1,v23         */

That'd result in smaller .exp files, and truly only one
place to update the tests whenever they need to change.

Bonus would be to use "disassemble /r" and match the raw opcodes
as well.

> +
> +proc instr_to_patt {instr} {
> +    # 0x0000000000000018 <+24>:	stxvd2x vs43,r4,r5
> +    return ".*\r\n\[ \t\]*0x\[0-9a-f\]+ <\\+\[0-9a-f\]*>:\[ \t\]*[string map [list { } "\[ \t\]+" . {\.}] $instr]\[ \t\]*\r\n.*"

Run $instr through string_to_regexp instead of string
map.

Could also use $hex instead of the the 0-9a-f patterns.

> +}
> +
> +proc func_check {instr} {
> +    global func
> +
> +    set test "Found: $instr"

lowercase "found:"  (all test names in gdb are lowercase.)

> +gdb_test_multiple "set disassembler-options invalid_option_value" \
> +	"set disassembler-options to invalid option value" {
> +  -re "Invalid disassembler option value: 'invalid_option_value'\." {
> +    pass "set disassembler-options invalid_option_value"
> +  }
> +  -re "'set disassembler-options \.\.\.' is not supported on this architecture\." {
> +    pass "set disassembler-options invalid_option_value"
> +  }

The usual style is to put the test name in a variable, like:

set test "set disassembler-options invalid_option_value"
gdb_test_multiple $test $test {
  -re "Invalid disassembler option value: 'invalid_option_value'\." {
    pass $test
  }
  -re "'set disassembler-options \.\.\.' is not supported on this architecture\." {
    pass $test
  }
}

This makes it copy/paste typo-proof.

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]