From: Pedro Alves <palves@redhat.com>
To: Peter Bergner <bergner@vnet.ibm.com>, gdb-patches@sourceware.org
Cc: Yao Qi <qiyaoltc@gmail.com>, Alan Modra <amodra@gmail.com>,
Ulrich Weigand <uweigand@de.ibm.com>,
binutils <binutils@sourceware.org>
Subject: Re: [PATCH, v17] Add support for setting disassembler-options in GDB for POWER, ARM and S390
Date: Thu, 23 Feb 2017 12:26:00 -0000 [thread overview]
Message-ID: <3b6b8153-cfff-8fb3-2cc0-2612f3d2d710@redhat.com> (raw)
In-Reply-To: <a752c132-4933-866a-3cc9-a463ebf86427@vnet.ibm.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))
> +
> \f
> /* 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);
> }
> \f
Thanks,
Pedro Alves
next prev parent reply other threads:[~2017-02-23 12:26 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-22 21:23 Peter Bergner
2017-02-23 12:26 ` Pedro Alves [this message]
2017-02-24 4:58 ` Peter Bergner
2017-02-24 15:06 ` Pedro Alves
2017-02-24 17:13 ` Peter Bergner
2017-02-24 17:25 ` Pedro Alves
2017-02-24 17:28 ` Peter Bergner
2017-02-28 11:24 ` Yao Qi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3b6b8153-cfff-8fb3-2cc0-2612f3d2d710@redhat.com \
--to=palves@redhat.com \
--cc=amodra@gmail.com \
--cc=bergner@vnet.ibm.com \
--cc=binutils@sourceware.org \
--cc=gdb-patches@sourceware.org \
--cc=qiyaoltc@gmail.com \
--cc=uweigand@de.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).