Message ID | 1400878971-6311-1-git-send-email-brad.mouring@ni.com |
---|---|
State | Superseded |
Headers |
Received: (qmail 4348 invoked by alias); 23 May 2014 21:02:59 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 4337 invoked by uid 89); 23 May 2014 21:02:58 -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 skprod2.natinst.com (HELO ni.com) (130.164.80.23) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 23 May 2014 21:02:58 +0000 Received: from us-aus-mgwout2.amer.corp.natinst.com (nb-chan1-1338.natinst.com [130.164.19.134]) by us-aus-skprod2.natinst.com (8.14.5/8.14.5) with ESMTP id s4NL2tmB010997 for <gdb-patches@sourceware.org>; Fri, 23 May 2014 16:02:56 -0500 Received: from linuxgetsreal.amer.corp.natinst.com ([130.164.14.198]) by us-aus-mgwout2.amer.corp.natinst.com (Lotus Domino Release 8.5.3FP5) with SMTP id 2014052316025498-279406 ; Fri, 23 May 2014 16:02:54 -0500 Received: by linuxgetsreal.amer.corp.natinst.com (sSMTP sendmail emulation); Fri, 23 May 2014 16:02:54 -0500 From: "Brad Mouring" <bmouring@ni.com> To: gdb-patches@sourceware.org Cc: Brad Mouring <brad.mouring@ni.com> Subject: [PATCH] gdb/source.c: Fix source path substitution Date: Fri, 23 May 2014 16:02:51 -0500 Message-Id: <1400878971-6311-1-git-send-email-brad.mouring@ni.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:5.11.96, 1.0.14, 0.0.0000 definitions=2014-05-23_07:2014-05-23, 2014-05-23, 1970-01-01 signatures=0 |
Commit Message
Brad Mouring
May 23, 2014, 9:02 p.m. UTC
Substitute source path functionality never worked on non-Windows
platforms due to straight strcmp tests returning non-zeros.
Signed-off-by: Brad Mouring <brad.mouring@ni.com>
---
gdb/source.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Comments
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] <hack hack hack on GDB> % 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 <brad.mouring@ni.com> > --- > 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
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] > <hack hack hack on GDB> > % 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 <brad.mouring@ni.com> > > --- > > 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
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] > > <hack hack hack on GDB> > > % 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 <brad.mouring@ni.com> > > > --- > > > 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
> > > 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? Can you explain which info you are referring to? About cover letters, they typically seem to be used when submitting a series of patch, rather than a patch alone, to introduce the series, or to provide info that really does not belong in the revision log, Other than that, having the details in the revision log is usually mostly positive, but we can help you with our review if we think we can help improving it.
On Tue, May 27, 2014 at 11:10:33AM -0700, Joel Brobecker wrote: > > > > 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? > > Can you explain which info you are referring to? The details concerning the issue that I'm fixing > > About cover letters, they typically seem to be used when submitting > a series of patch, rather than a patch alone, to introduce the series, > or to provide info that really does not belong in the revision log, > Other than that, having the details in the revision log is usually > mostly positive, but we can help you with our review if we think > we can help improving it. > > -- > Joel One additional question that I had that was not answered on the IRC channel is I wanted some validation that such a simple change does not require copyright assignment since this fixes existing functionality and provides no new, patentable functionality. -Brad
> > > Will do. I take it this info belongs in the commit message, or would > > > you rather it be a cover letter-type email? > > > > Can you explain which info you are referring to? > The details concerning the issue that I'm fixing Generally speaking, the best place for explaining why you do what you do is the code. The more goes into the code, the better (usually). The rest should be in the revision log. The idea is to help us avoid doing most of the archeology based purely on the repository. Tracking emails is quite a bit more labor-intensive... For an example of a commit that I thought was pretty nice, take a look at: commit 6a3cb8e88a739c967bb9b2d8774bf96b87a7fda4 Author: Pedro Alves <palves@redhat.com> Date: Wed May 21 18:30:47 2014 +0100 Allow making GDB not automatically connect to the native target. It provides the context, shows what we had before, what we get now, motivation, etc. > One additional question that I had that was not answered on the IRC > channel is I wanted some validation that such a simple change does > not require copyright assignment since this fixes existing functionality > and provides no new, patentable functionality. If you keep your contributions to obvious changes, and/or small ones, they are deemed "not legally significant", and we can accept one or two of them. The guideline is that it should be under 15 lines.
Let's try this one again. The check for the source (or "from") directory snippet in listing matching path substitution rules currently will not match anything other than a direct match of the "from" field in a substitution rule, resulting in the incorrect behavior below ... (gdb) set substitute-path /a/path /another/path (gdb) show substitute-path List of all source path substitution rules: `/a/path' -> `/another/path'. (gdb) show substitute-path /a/path/to/a/file.ext Source path substitution rule matching `/a/path/to/a/file.ext': (gdb) show substitute-path /a/path Source path substitution rule matching `/a/path': `/a/path' -> `/another/path'. ... This change effects the following behavior by (sanely) checking with the length of the "from" portion of a rule and ensuring that the next character of the path considered for substitution is a path delimiter. With this change, the following behavior is garnered ... (gdb) set substitute-path /a/path /another/path (gdb) show substitute-path List of all source path substitution rules: `/a/path' -> `/another/path'. (gdb) show substitute-path /a/path/to/a/file.ext Source path substitution rule matching `/a/path/to/a/file.ext': `/a/path' -> `/another/path'. (gdb) show substitute-path /a/pathological/case/that/should/fail.err Source path substitution rule matching `/a/pathological/case/that/should/fail.err': (gdb) Also included in this submission is a couple of new tests to verify this behavior in the test suite. Cheers and thanks for your attention. ChangeLog entry for gdb/ChangeLog: 2014-05-28 Brad Mouring <brad.mouring@ni.com> * source.c (show_substitute_path_command): Fix display of matching substitution rules * subst.exp: Add tests to verify changes in source.c
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; }