This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH, updated] Add support for setting disassembler-options in GDB for POWER, ARM and S390
- From: Pedro Alves <palves at redhat dot com>
- To: Peter Bergner <bergner at vnet dot ibm dot com>, Yao Qi <qiyaoltc at gmail dot com>
- Cc: gdb-patches at sourceware dot org, Alan Modra <amodra at gmail dot com>, Ulrich Weigand <uweigand at de dot ibm dot com>, Eli Zaretskii <eliz at gnu dot org>, Nick Clifton <nickc at redhat dot com>, binutils <binutils at sourceware dot org>
- Date: Tue, 14 Feb 2017 20:01:24 +0000
- Subject: Re: [PATCH, updated] Add support for setting disassembler-options in GDB for POWER, ARM and S390
- Authentication-results: sourceware.org; auth=none
- References: <beb0d776-c509-ab82-755a-0b4ff8a7a41a@vnet.ibm.com> <867f4uccky.fsf@gmail.com> <65f2f8ce-450b-297a-dcab-7a8bc0ebc256@vnet.ibm.com>
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