From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 112989 invoked by alias); 31 Jan 2017 21:13:48 -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 112955 invoked by uid 89); 31 Jan 2017 21:13:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.0 required=5.0 tests=AWL,BAYES_50,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=@code, OPTIONS, @table, regnames X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0a-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.156.1) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 31 Jan 2017 21:13:37 +0000 Received: from pps.filterd (m0098394.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v0VL8ljK121036 for ; Tue, 31 Jan 2017 16:13:36 -0500 Received: from e36.co.us.ibm.com (e36.co.us.ibm.com [32.97.110.154]) by mx0a-001b2d01.pphosted.com with ESMTP id 28audpaar9-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 31 Jan 2017 16:13:35 -0500 Received: from localhost by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 31 Jan 2017 14:13:35 -0700 Received: from d03dlp01.boulder.ibm.com (9.17.202.177) by e36.co.us.ibm.com (192.168.1.136) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 31 Jan 2017 14:13:31 -0700 Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id A09811FF0023; Tue, 31 Jan 2017 14:13:08 -0700 (MST) Received: from b03ledav001.gho.boulder.ibm.com (b03ledav001.gho.boulder.ibm.com [9.17.130.232]) by b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v0VLDUgc8978848; Tue, 31 Jan 2017 14:13:30 -0700 Received: from b03ledav001.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 6E43C6E03F; Tue, 31 Jan 2017 14:13:30 -0700 (MST) Received: from otta.rchland.ibm.com (unknown [9.10.86.53]) by b03ledav001.gho.boulder.ibm.com (Postfix) with ESMTP id A731F6E041; Tue, 31 Jan 2017 14:13:29 -0700 (MST) Subject: Re: [PATCH 1/2] Add support for setting disassembler-options in GDB for POWER, ARM and S390 To: Yao Qi References: <83eg28dcjk.fsf@gnu.org> <019eaf5d-9ace-539e-8501-feb3cb0eed6c@vnet.ibm.com> <20170125142335.GT28060@E107787-LIN> Cc: Pedro Alves , Eli Zaretskii , nickc@redhat.com, gdb-patches@sourceware.org, uweigand@de.ibm.com, amodra@gmail.com, binutils@sourceware.org From: Peter Bergner Date: Tue, 31 Jan 2017 21:13:00 -0000 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170125142335.GT28060@E107787-LIN> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 17013121-0020-0000-0000-00000B3DD869 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006532; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000201; SDB=6.00815374; UDB=6.00398058; IPR=6.00592816; BA=6.00005103; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00014127; XFM=3.00000011; UTC=2017-01-31 21:13:33 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17013121-0021-0000-0000-000059B6D029 Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-01-31_10:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1612050000 definitions=main-1701310170 X-IsSubscribed: yes X-SW-Source: 2017-01/txt/msg00676.txt.bz2 Hmmm, sorry to get back to you on this, but the first time I saw your note, I only saw your comment regarding the gdb.texinfo issue and I missed everything else you said. :-( On 1/25/17 8:23 AM, Yao Qi wrote: >> +@table @code >> +@kindex set disassembler-options > > Looks "@kindex set disassembly-flavor" is removed by mistake. Fixed, thanks. >> +extern const char **disassembler_options_names_powerpc (void); >> +extern const char **disassembler_options_names_arm (void); >> +extern const char **disassembler_options_desc_arm (void); >> +extern const char **disassembler_options_names_s390 (void); >> +extern const char **disassembler_options_desc_s390 (void); > > We end up having three things from opcodes for disassembler in each > target, the disassembler_ftype function pointer, the options names, > and options descs. Every new added target needs to implement these > three things, and hook them correctly with gdb and objdump. > > Why don't we merge functions "_names_" and "_desc_" into one function, Well not all architectures have the _desc_ strings (ie, powerpc), but I guess that those types of arches can just set the description pointer to NULL. Ok, I'll give that a try. >> typedef struct >> { >> const char *name; >> + const char *long_name; > > Is this field necessary? "reg-names-"$name is the long_name, isn't? The option name scanning is done in architecture independent code, so it needs the long name which it gets via the call to disassembler_options_names_arm(). The old "set disassembler ..." command on the other hand uses the short name, so for backward compatibility with the old command, I need the short name. That said, the only use of the short name is in get_arm_regnames and parse_arm_disassembler_option. I suppose I could skip over the reg-names- part of the long name instead, which would allow us to only need the long version of name. I'll see if it's doable. Obviously, I'd just use .name for the field name. >> +const char ** >> +disassembler_options_names_arm (void) >> +{ >> + static const char **options = NULL; >> + >> + if (options == NULL) >> + { >> + size_t i; >> + size_t num_options = NUM_ARM_REGNAMES + 2; >> + options = XNEWVEC (const char *, num_options + 1); >> + for (i = 0; i < NUM_ARM_REGNAMES; i++) >> + options[i] = regnames[i].long_name; > > We can get options from .name, > > options[i] = xmalloc (sizeof ("reg-names-") + strlen (regnames[i].name) + 1); > strcpy (options[i], "reg-names-"); > strcat (options[i], strlen (regnames[i].name)); This is moot if I can combine the option names as I described above. >> void >> print_arm_disassembler_options (FILE *stream) >> { > > print_arm_disassembler_options can be adjusted as what you changed to > print_s390_disassembler_options, otherwise, we may have inconsistency > in the future. Can you be more specific about what I did to the s390 code you want done to the arm code? I'll note I have changed things due to Alan wanting the strings to support internationalization. >> 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[256], *style = long_name; >> + snprintf (style, 256, "reg-names-%s", *(char **)c->var); > > You can get the option set in command from disassembly_style, so you > don't have to access c->var, and no need to include "cli/cli-decode.h". >> >> + c->var = &style; >> + set_disassembler_options (args, from_tty, c); > > We usually don't relay the command call this way. It is fragile, IMO. > Can you factor out a function from set_disassembler_options, which > accept "normalized" options, and both set_disassembly_style_sfunc > and set_disassembler_options can call it. I have an idea, let me play with it. > One problem I can see is that "set arm disassembler" overwrite > "disassembler-options", > > (gdb) set disassembler-options reg-names-gcc,no-force-thumb > (gdb) show disassembler-options > The current disassembler options are 'reg-names-gcc,no-force-thumb' > > .... > (gdb) set arm disassembler std > (gdb) show disassembler-options > The current disassembler options are 'reg-names-std' > > "no-force-thumb" is disappeared. Yes, this is as expected, because... > I don't know how to fix it. Looks command "disassembler-options" is a > super set of "arm disassembler". Yes, "disassembler-options" is a superset of "arm disassembler", that is why "arm disassembler" is built on top of "disassembler-options". If you change the disassembly style using one of the commands, the other command had better agree, otherwise things are just broken. Now given that disassembler-options is the more feature rich interface, you're probably better off using it rather than the older "arm disassembler" interface. But we do want them to work together. >> +/* Remove whitespace and consecutive commas from OPTIONS. */ >> + >> +char * >> +cleanup_disassembler_options (char *options) > > "cleanup" is a special term in gdb, so better to rename it, > > remove_whitespace_and_comma or parse_preparation, for example? Ok, I'll change it to not use cleanup. >> +void >> +set_disassembler_options (char *args, int from_tty, struct cmd_list_element *c) >> +{ >> + struct gdbarch *gdbarch = get_current_arch (); > > "set disassembler-options" applies to the current gdbarch, rather than a > global setting, but we can't tell this from the command name. GDB has > multiple instances of gdbarch, and disassembler-options is arch-specific. > We may set disassembler-options when gdbarch is A, but disassembler-options > become something different when current gdbarch is B. Pedro and I discussed this already here: https://sourceware.org/ml/gdb-patches/2016-10/msg00806.html If you set the disassembler-options for A and then switch to B, you don't want A's disassembler-options when disassembling for B. Then if you switch back to A, your options will be restored. > However, I am not sure "set $ARCH disassembler-options" is good idea or not. We discussed this too and ended up deciding not to have the arch specific command and use a generic command that all the arches would use. Peter