From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28827 invoked by alias); 27 May 2014 13:17:28 -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 28803 invoked by uid 89); 27 May 2014 13:17:27 -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,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: ni.com Received: from skprod3.natinst.com (HELO ni.com) (130.164.80.24) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 27 May 2014 13:17:25 +0000 Received: from us-aus-mgwout1.amer.corp.natinst.com (nb-snip2-1338.natinst.com [130.164.19.135]) by us-aus-skprod3.natinst.com (8.14.5/8.14.5) with ESMTP id s4RDHH4n013279; Tue, 27 May 2014 08:17:17 -0500 Received: from linuxgetsreal.amer.corp.natinst.com ([130.164.14.198]) by us-aus-mgwout1.amer.corp.natinst.com (Lotus Domino Release 8.5.3FP5) with SMTP id 2014052708171680-357753 ; Tue, 27 May 2014 08:17:16 -0500 Received: by linuxgetsreal.amer.corp.natinst.com (sSMTP sendmail emulation); Tue, 27 May 2014 08:17:16 -0500 From: "Brad Mouring" Date: Tue, 27 May 2014 13:17:00 -0000 To: Joel Brobecker Cc: Brad Mouring , gdb-patches@sourceware.org Subject: Re: [PATCH] gdb/source.c: Fix source path substitution Message-ID: <20140527131716.GA8300@linuxgetsreal> References: <1400878971-6311-1-git-send-email-brad.mouring@ni.com> <20140523234959.GX22822@adacore.com> <20140524000034.GY22822@adacore.com> MIME-Version: 1.0 In-Reply-To: <20140524000034.GY22822@adacore.com> User-Agent: Mutt/1.5.21 (2010-09-15) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.11.96,1.0.14,0.0.0000 definitions=2014-05-27_02:2014-05-26,2014-05-27,1970-01-01 signatures=0 X-SW-Source: 2014-05/txt/msg00667.txt.bz2 On Fri, May 23, 2014 at 05:00:34PM -0700, Joel Brobecker wrote: > On Fri, May 23, 2014 at 04:49:59PM -0700, Joel Brobecker wrote: > > Brad, > > > > > Substitute source path functionality never worked on non-Windows > > > platforms due to straight strcmp tests returning non-zeros. > > > > Thanks for the patch. > > > > First of all, the administrative stuff. There are a few important pieces > > missing from your subscription. I invite you read the file gdb/CONTRIBUTE > > which should explain it all. Do not hesitate to ask questions if needed. Will do. I take it this info belongs in the commit message, or would you rather it be a cover letter-type email? > > > > Second, we'd like all submissions to come with a more detailed > > description of what's wrong and how your patch fixes things. > > Ideally, we'd like a testcase be also added, if at all possible. Will add to the commit message. > > > > This brings me to another topic: It's important that you state how > > your patch has been tested, and in particular, we require that > > every patch be tested against the GDB testsuite. For native platforms, > > I usually run it like so: > > > > % cd gdb/testsuite > > % make -j16 check > > % [save gdb.sum and gdb.log] > > > > % cd gdb/testsuite > > % make -j16 check > > % [compare saved gdb.sum with new gdb.sum to make sure no new > > failure was introduced by your patch] > > ACK'd. Will do. > > At first sight, I don't see how your patch can make sense, because > > FILENAME_CMP is defined as: > > > > extern int filename_cmp (const char *s1, const char *s2); > > #define FILENAME_CMP(s1, s2) filename_cmp(s1, s2) > > > > And at the same time strlen(path_start) and strlen(rule->from) > > is from_len; so filename_cmp should work the same as what you > > propose. That's why I think it's important to give a detailed > > description of what's going on and what your analysis of the issue > > is. An image is worth a thousand words, so you'll often see people > > copy/pasting GDB sessions to describe their problem. > > > > In the case of the second change, I think you might be right, > > as the test would not work if the rule was "/this/path", and > > the given argument was "/this/path/to/somewhere". But your fix > > wouldn't be complete, since I believe you would also print the > > rule if given "/this/path/too/long which would be wrong. I think > > Gaah - brain fart; read: "/this/path2/somewhere" which would be wrong. > > Basically, we need to make sure that we're the "from" part of > the rule matches either the entire path name, or else stops at > a directory separator. Ah, thanks for the catch. I'll do more of a complete fix to cover that in lieu of the quick hack I did here. I very well got the order mixed up and I certainly overlooked the same-start-of-the-dirname issue. > > > we should actually be using substitute_path_rule_matches instead > > of hard-coding the test there. > > > > If those two situations are not tested by our current testsuite, > > I believe it should be fairly easy to add them, so I would consider > > it a requirement of inclusion of your patch. I'll add some tests to gdb/testsuite/gdb.base/subst.exp after running the testsuite to make sure that my changes added no new badness. > > > > Lastly, the two changes appear to be independent of each other, > > so it would be better if they were submitted each on their own, > > and each checked in individually. > > I'll break the changes into two patches. > > Thank you! > > > > > Signed-off-by: Brad Mouring > > > --- > > > gdb/source.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/gdb/source.c b/gdb/source.c > > > index c77a4f4..7b59d77 100644 > > > --- a/gdb/source.c > > > +++ b/gdb/source.c > > > @@ -946,7 +946,7 @@ substitute_path_rule_matches (const struct substitute_path_rule *rule, > > > strncpy (path_start, path, from_len); > > > path_start[from_len] = '\0'; > > > > > > - if (FILENAME_CMP (path_start, rule->from) != 0) > > > + if (filename_ncmp (path_start, rule->from, from_len) != 0) > > > return 0; > > > > > > /* Make sure that the region in the path that matches the substitution > > > @@ -1897,7 +1897,7 @@ show_substitute_path_command (char *args, int from_tty) > > > > > > while (rule != NULL) > > > { > > > - if (from == NULL || FILENAME_CMP (rule->from, from) == 0) > > > + if (from == NULL || filename_ncmp (rule->from, from, strlen(rule->from)) == 0) > > > printf_filtered (" `%s' -> `%s'.\n", rule->from, rule->to); > > > rule = rule->next; > > > } > > > -- > > > 1.8.3-rc3 > > > > -- > > Joel > > -- > Joel