From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 117374 invoked by alias); 14 Feb 2017 20:01:46 -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 116542 invoked by uid 89); 14 Feb 2017 20:01:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=2.8 required=5.0 tests=BAYES_50,RP_MATCHES_RCVD,SPF_HELO_PASS,UNSUBSCRIBE_BODY autolearn=no version=3.3.2 spammy=char_ptr, temple, Temple, sk:set_gdb 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; Tue, 14 Feb 2017 20:01:34 +0000 Received: from smtp.corp.redhat.com (int-mx16.intmail.prod.int.phx2.redhat.com [10.5.11.28]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F3E40C0567B3; Tue, 14 Feb 2017 20:01:34 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id B9DFD723A0; Tue, 14 Feb 2017 20:01:26 +0000 (UTC) Subject: Re: [PATCH, updated] Add support for setting disassembler-options in GDB for POWER, ARM and S390 To: Peter Bergner , Yao Qi References: <867f4uccky.fsf@gmail.com> <65f2f8ce-450b-297a-dcab-7a8bc0ebc256@vnet.ibm.com> Cc: gdb-patches@sourceware.org, Alan Modra , Ulrich Weigand , Eli Zaretskii , Nick Clifton , binutils From: Pedro Alves Message-ID: <77996338-961a-5a69-e41a-f1adbb3b23da@redhat.com> Date: Tue, 14 Feb 2017 20:01: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: <65f2f8ce-450b-297a-dcab-7a8bc0ebc256@vnet.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-02/txt/msg00396.txt.bz2 On 02/13/2017 06:47 PM, Peter Bergner wrote: > On 2/13/17 9:52 AM, Yao Qi wrote: >> Peter Bergner 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