From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9984 invoked by alias); 23 Feb 2017 12:26:13 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 9964 invoked by uid 89); 23 Feb 2017 12:26:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=enable-targets, enabletargets X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 23 Feb 2017 12:26:10 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A8CD8C04B941; Thu, 23 Feb 2017 12:26:10 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1NCQ6Al006698; Thu, 23 Feb 2017 07:26:07 -0500 Subject: Re: [PATCH, v17] Add support for setting disassembler-options in GDB for POWER, ARM and S390 To: Peter Bergner , gdb-patches@sourceware.org References: Cc: Yao Qi , Alan Modra , Ulrich Weigand , binutils From: Pedro Alves Message-ID: <3b6b8153-cfff-8fb3-2cc0-2612f3d2d710@redhat.com> Date: Thu, 23 Feb 2017 12:26:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-02/txt/msg00618.txt.bz2 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