From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92343 invoked by alias); 16 Feb 2017 00:21:27 -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 92324 invoked by uid 89); 16 Feb 2017 00:21:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=guidelines, submissions, sk:set_cmd, valid_options 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, 16 Feb 2017 00:21:15 +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 2A3D17F342; Thu, 16 Feb 2017 00:21:15 +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 v1G0L7Mf019006; Wed, 15 Feb 2017 19:21:12 -0500 Subject: Re: [PATCH, updated] Add support for setting disassembler-options in GDB for POWER, ARM and S390 To: Peter Bergner References: <867f4uccky.fsf@gmail.com> <65f2f8ce-450b-297a-dcab-7a8bc0ebc256@vnet.ibm.com> <77996338-961a-5a69-e41a-f1adbb3b23da@redhat.com> Cc: Yao Qi , gdb-patches@sourceware.org, Alan Modra , Ulrich Weigand , Eli Zaretskii , Nick Clifton , binutils From: Pedro Alves Message-ID: Date: Thu, 16 Feb 2017 00:21: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=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-02/txt/msg00430.txt.bz2 On 02/15/2017 11:14 PM, Peter Bergner wrote: > On 2/14/17 2:01 PM, Pedro Alves wrote: >> - 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. > > I'm not a git expert, as I'm paid to work on gcc and we still use > subversion. I am willing to learn though, so can you explain what > you mean by the above? Play with "git log gdb/" a bit and you'll get a feel. You'll notice that we put the rationale for changes in the commit log, unlike in gcc, where folks just tend to copy the ChangeLog entry to the svn log at commit time. In practice, what this means is just that you use "git commit --ammend", and edit the commit message to include the rationale for the patch, and update it / maintain it whenever the patch changes in a way that might change the description/rationale for the patch. The commit log is reviewed as a part of the patch as well. More info here http://sourceware.org/gdb/wiki/ContributionChecklist. I'd recommend reading Linux patch submission guidelines, there are many such documents, and tend to explain these things very nicely. For example: https://kernelnewbies.org/PatchPhilosophy >> - 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 whic >> email contains the latest version. > > Easily done, as I've been doing just that internally. > I'm frightened to say that I'm at v25 and counting. :-( Internal revisions don't count, only public submissions. :-) > >>> + 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) > > The problem with strtok{,_r} is that it is destructive to the option > string, so we'd have to dup the string before scanning it, which > doesn't seem very elegant either. We would also need to remember > to free the dup'd string as well when we were done with it. > > The reason I copied the parsed option into a char array was that I > needed a null terminated string that I could use with strcmp. > Unfortunately, the POWER port has several options that have a > common prefix: > > Eg: "e500" & "e500mc", "ppc" & "ppc32" and "ppc64", etc. > > ...which strncmp cannot disambiguate, because it doesn't enforce the > two strings have the same length. You could handle that with: if (optlen == strlen (valid_options->name[i]) && strncmp (opt, optlen, valid_options->name[i]) == 0) and adjust the macro to return the len up to the comma (minus whitespace) in optlen, and make opt be a pointer that points directly to the input string. > I have two ideas, one is to write our own strcmp that treats option > delimiters like ',' just like '\0'. Or something like that. > The other idea would be to > modify the disassembler_options string so that we use '\0' as the > delimiter between the different options. We'd need an extra '\0' > at the end to know when we've run out of options though. If we > did this, then we could just use standard strcmp on the options. Can't see how this would work without an interning step, given the delimiters come from user input. >> I believe 'word' points past the comma already? > > 'word' does point past the last comma, but sometimes, it points well > past the last comma. For example, if I type: > > set disassembler-options force-thumb, reg-name-g > > ...then 'text' will equal "force-thumb, reg-name-g" and 'word' > will equal "g". To get the completer to match "reg-names-gcc", > I have to modify 'text' to be "reg-names-g". Ah, that's because "-" is a word break character too. That'd be fixable by implementing the set_cmd_completer_handle_brkchars hook, but let's not bother. Please just check that (gdb) complete set disassembler-options force-thumb, reg-name-g does the right thing. >> 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. > > I'm just using the mechanism that the original HUGE test case used. > The fact that I'm breaking that test case up into many much smaller > test cases is an improvement and makes overflowing expect's buffer > less likely with the patch than before the patch. This mention of "breaking the test cases up in many smaller test cases" is the sort of rationale that would be nice to put in the commit log. :-) I wasn't actually sure that that's what you're doing, and why. Seemed like you've changed the tests to avoid hardcoding offsets too? (it would probabably be clearer to do that with a separate preparatory patch, with the added advantage that that part could probably be merged to master quickly, but fine to keep it together if you prefer.) > That's sounds great, but I'm afraid I have no idea how to do the > above and I don't see any examples like it in the testsuite to copy. > Unless you can show me a lot more context to your idea, I'm afraid > I'm going to have to leave the test cases as they are. OK, let's leave it. Thanks, Pedro Alves