From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 7136 invoked by alias); 23 May 2014 23:50:02 -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 7115 invoked by uid 89); 23 May 2014 23:50:01 -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; Fri, 23 May 2014 23:50:00 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id ABBEC116261; Fri, 23 May 2014 19:49:58 -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 QR9jlGzDxASc; Fri, 23 May 2014 19:49:58 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 785D611628D; Fri, 23 May 2014 19:49:58 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id AEB304395A; Fri, 23 May 2014 16:49:59 -0700 (PDT) Date: Fri, 23 May 2014 23:50: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: <20140523234959.GX22822@adacore.com> References: <1400878971-6311-1-git-send-email-brad.mouring@ni.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1400878971-6311-1-git-send-email-brad.mouring@ni.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-05/txt/msg00609.txt.bz2 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 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