From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18664 invoked by alias); 24 May 2014 00:00:36 -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 18654 invoked by uid 89); 24 May 2014 00:00:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00 autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Sat, 24 May 2014 00:00:35 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id A8E2E116290; Fri, 23 May 2014 20:00:33 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id VkQAHEyAAyDE; Fri, 23 May 2014 20:00:33 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 74ABC11625D; Fri, 23 May 2014 20:00:33 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id CAAAB4395A; Fri, 23 May 2014 17:00:34 -0700 (PDT) Date: Sat, 24 May 2014 00:00:00 -0000 From: Joel Brobecker To: Brad Mouring Cc: gdb-patches@sourceware.org, Brad Mouring Subject: Re: [PATCH] gdb/source.c: Fix source path substitution Message-ID: <20140524000034.GY22822@adacore.com> References: <1400878971-6311-1-git-send-email-brad.mouring@ni.com> <20140523234959.GX22822@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140523234959.GX22822@adacore.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-05/txt/msg00610.txt.bz2 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. > > 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. > > 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] > > 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. > 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. > > 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. > > 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