[PATCH] gdb/source.c: Fix source path substitution

[PATCH] gdb/source.c: Fix source path substitution

[Brad Mouring]

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(-)

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

Re: [PATCH] gdb/source.c: Fix source path substitution

[Joel Brobecker]

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

-- 
Joel

Re: [PATCH] gdb/source.c: Fix source path substitution

[Joel Brobecker]

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

-- 
Joel

Re: [PATCH] gdb/source.c: Fix source path substitution

[Brad Mouring]

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

Re: [PATCH] gdb/source.c: Fix source path substitution

[Joel Brobecker]

> > > 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.

-- 
Joel

Re: [PATCH] gdb/source.c: Fix source path substitution

[Brad Mouring]

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

Re: [PATCH] gdb/source.c: Fix source path substitution

[Joel Brobecker]

> > > 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.

-- 
Joel

Fix matching path substitution rule listing, add tests

[Brad Mouring]

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

[PATCH 1/2] testsuite/subst: Add tests for printing matches

[Brad Mouring]

The existing tests for printing matching substitution rules was
insufficient as it was not testing that matching rules would actually
be printed when checking with the "show substitute-path" command

Signed-off-by: Brad Mouring <brad.mouring@ni.com>
---
 gdb/testsuite/gdb.base/subst.exp | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gdb/testsuite/gdb.base/subst.exp b/gdb/testsuite/gdb.base/subst.exp
index e132809..e99735b 100644
--- a/gdb/testsuite/gdb.base/subst.exp
+++ b/gdb/testsuite/gdb.base/subst.exp
@@ -95,6 +95,14 @@ gdb_test "show substitute-path depuis" \
          "Source path substitution rule matching `depuis':\r\n +`depuis' -> `vers'." \
          "show substitute-path depuis, after all paths added"
 
+gdb_test "show substitute-path from/path" \
+         "Source path substitution rule matching `from/path':\r\n +`from' -> `to'." \
+         "show substitute-path from/path, after all paths added"
+
+gdb_test "show substitute-path from_a_bad_path" \
+         "Source path substitution rule matching `from_a_bad_path':" \
+         "show substitute-path from_a_bad_path, after all paths added"
+
 gdb_test "show substitute-path garbage" \
          "Source path substitution rule matching `garbage':" \
          "show substitute-path garbage, after all paths added"
-- 
1.8.3-rc3

[PATCH 2/2] gdb/source.c: Fix matching path substitute rule listing

[Brad Mouring]

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)

Signed-off-by: Brad Mouring <brad.mouring@ni.com>
---
 gdb/source.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/source.c b/gdb/source.c
index c77a4f4..a32872f 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1875,6 +1875,7 @@ show_substitute_path_command (char *args, int from_tty)
   char **argv;
   char *from = NULL;
   struct cleanup *cleanup;
+  int rule_from_len;
   
   argv = gdb_buildargv (args);
   cleanup = make_cleanup_freeargv (argv);
@@ -1897,7 +1898,11 @@ show_substitute_path_command (char *args, int from_tty)
 
   while (rule != NULL)
     {
-      if (from == NULL || FILENAME_CMP (rule->from, from) == 0)
+      rule_from_len = strlen(rule->from);
+      if (from == NULL ||
+        ((filename_ncmp (rule->from, from, rule_from_len) == 0) &&
+          (IS_DIR_SEPARATOR (from[rule_from_len]) ||
+          from[rule_from_len] == 0)))
         printf_filtered ("  `%s' -> `%s'.\n", rule->from, rule->to);
       rule = rule->next;
     }
-- 
1.8.3-rc3

Re: [PATCH 2/2] gdb/source.c: Fix matching path substitute rule listing

[Joel Brobecker]

Brad,

I would merge both patches into one, in this case, as I suspect
this will simplify submission for you. Make sure to always include
the ChangeLog entry in your revision log for every patch.

The introduction email (aka cover letter) explains very well what
you are trying to do, and I would use that as your revision log
for the merged patch.

> diff --git a/gdb/source.c b/gdb/source.c
> index c77a4f4..a32872f 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -1875,6 +1875,7 @@ show_substitute_path_command (char *args, int from_tty)
>    char **argv;
>    char *from = NULL;
>    struct cleanup *cleanup;
> +  int rule_from_len;
>    
>    argv = gdb_buildargv (args);
>    cleanup = make_cleanup_freeargv (argv);
> @@ -1897,7 +1898,11 @@ show_substitute_path_command (char *args, int from_tty)
>  
>    while (rule != NULL)
>      {
> -      if (from == NULL || FILENAME_CMP (rule->from, from) == 0)
> +      rule_from_len = strlen(rule->from);
> +      if (from == NULL ||
> +        ((filename_ncmp (rule->from, from, rule_from_len) == 0) &&
> +          (IS_DIR_SEPARATOR (from[rule_from_len]) ||
> +          from[rule_from_len] == 0)))

Why not use substitute_path_rule_matches, here?

-- 
Joel

[PATCH] gdb/source.c: Fix matching path substitute rule listing

[Brad Mouring]

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 (or NULL). 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 is a couple of tests added to subst.exp to verify this behavior
in the test suite.

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
---
 gdb/source.c                     | 7 ++++++-
 gdb/testsuite/gdb.base/subst.exp | 8 ++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/gdb/source.c b/gdb/source.c
index c77a4f4..a32872f 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1875,6 +1875,7 @@ show_substitute_path_command (char *args, int from_tty)
   char **argv;
   char *from = NULL;
   struct cleanup *cleanup;
+  int rule_from_len;
   
   argv = gdb_buildargv (args);
   cleanup = make_cleanup_freeargv (argv);
@@ -1897,7 +1898,11 @@ show_substitute_path_command (char *args, int from_tty)
 
   while (rule != NULL)
     {
-      if (from == NULL || FILENAME_CMP (rule->from, from) == 0)
+      rule_from_len = strlen(rule->from);
+      if (from == NULL ||
+        ((filename_ncmp (rule->from, from, rule_from_len) == 0) &&
+          (IS_DIR_SEPARATOR (from[rule_from_len]) ||
+          from[rule_from_len] == 0)))
         printf_filtered ("  `%s' -> `%s'.\n", rule->from, rule->to);
       rule = rule->next;
     }
diff --git a/gdb/testsuite/gdb.base/subst.exp b/gdb/testsuite/gdb.base/subst.exp
index e132809..e99735b 100644
--- a/gdb/testsuite/gdb.base/subst.exp
+++ b/gdb/testsuite/gdb.base/subst.exp
@@ -95,6 +95,14 @@ gdb_test "show substitute-path depuis" \
          "Source path substitution rule matching `depuis':\r\n +`depuis' -> `vers'." \
          "show substitute-path depuis, after all paths added"
 
+gdb_test "show substitute-path from/path" \
+         "Source path substitution rule matching `from/path':\r\n +`from' -> `to'." \
+         "show substitute-path from/path, after all paths added"
+
+gdb_test "show substitute-path from_a_bad_path" \
+         "Source path substitution rule matching `from_a_bad_path':" \
+         "show substitute-path from_a_bad_path, after all paths added"
+
 gdb_test "show substitute-path garbage" \
          "Source path substitution rule matching `garbage':" \
          "show substitute-path garbage, after all paths added"
-- 
1.8.3-rc3

[PATCH] gdb/source.c: Fix matching path substitute rule listing

[Brad Mouring]

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 (or NULL). 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 is a couple of tests added to subst.exp to verify this behavior
in the test suite.

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
---
 gdb/source.c                     | 4 +++-
 gdb/testsuite/gdb.base/subst.exp | 8 ++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/gdb/source.c b/gdb/source.c
index c112765..240062c 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1857,6 +1857,7 @@ show_substitute_path_command (char *args, int from_tty)
   char **argv;
   char *from = NULL;
   struct cleanup *cleanup;
+  int rule_from_len;
   
   argv = gdb_buildargv (args);
   cleanup = make_cleanup_freeargv (argv);
@@ -1879,7 +1880,8 @@ show_substitute_path_command (char *args, int from_tty)
 
   while (rule != NULL)
     {
-      if (from == NULL || FILENAME_CMP (rule->from, from) == 0)
+      rule_from_len = strlen(rule->from);
+      if (from == NULL || substitute_path_rule_matches (rule, from) != 0)
         printf_filtered ("  `%s' -> `%s'.\n", rule->from, rule->to);
       rule = rule->next;
     }
diff --git a/gdb/testsuite/gdb.base/subst.exp b/gdb/testsuite/gdb.base/subst.exp
index e132809..e99735b 100644
--- a/gdb/testsuite/gdb.base/subst.exp
+++ b/gdb/testsuite/gdb.base/subst.exp
@@ -95,6 +95,14 @@ gdb_test "show substitute-path depuis" \
          "Source path substitution rule matching `depuis':\r\n +`depuis' -> `vers'." \
          "show substitute-path depuis, after all paths added"
 
+gdb_test "show substitute-path from/path" \
+         "Source path substitution rule matching `from/path':\r\n +`from' -> `to'." \
+         "show substitute-path from/path, after all paths added"
+
+gdb_test "show substitute-path from_a_bad_path" \
+         "Source path substitution rule matching `from_a_bad_path':" \
+         "show substitute-path from_a_bad_path, after all paths added"
+
 gdb_test "show substitute-path garbage" \
          "Source path substitution rule matching `garbage':" \
          "show substitute-path garbage, after all paths added"
-- 
1.8.3-rc3

Re: [PATCH] gdb/source.c: Fix matching path substitute rule listing

[Joel Brobecker]

Hi Brad,

> 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

Much better, thank you!

You are still missing a couple of elements:

  * You patch needs two ChangeLog entries, one in gdb/ChangeLog,
    and one in gdb/testsuite/ChangeLog. The revision log should
    have both at the end. Here is an example of another patch
    where both GDB and testsuite where changed thus resulting
    in 2 CL entries: 938f0e2f6766e90a5ddc5df00e97a68873fd1252

  * You forgot to tell us on which platform you tested your patch.

Comment about the patch itself below...

Please don't be discouraged, you have very close to getting it in!

> ---
>  gdb/source.c                     | 4 +++-
>  gdb/testsuite/gdb.base/subst.exp | 8 ++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/source.c b/gdb/source.c
> index c112765..240062c 100644
> --- a/gdb/source.c
> +++ b/gdb/source.c
> @@ -1857,6 +1857,7 @@ show_substitute_path_command (char *args, int from_tty)
>    char **argv;
>    char *from = NULL;
>    struct cleanup *cleanup;
> +  int rule_from_len;
>    
>    argv = gdb_buildargv (args);
>    cleanup = make_cleanup_freeargv (argv);
> @@ -1879,7 +1880,8 @@ show_substitute_path_command (char *args, int from_tty)
>  
>    while (rule != NULL)
>      {
> -      if (from == NULL || FILENAME_CMP (rule->from, from) == 0)
> +      rule_from_len = strlen(rule->from);
> +      if (from == NULL || substitute_path_rule_matches (rule, from) != 0)
>          printf_filtered ("  `%s' -> `%s'.\n", rule->from, rule->to);
>        rule = rule->next;

You do not need "rule_from_len" anymore. The final patch for this file
should be a one-line change.

-- 
Joel

[PATCH] gdb/source.c: Fix matching path substitute rule listing

[Brad Mouring]

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 (or NULL). 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 is a couple of tests added to subst.exp to verify this behavior
in the test suite.

This was tested on x86_64 Linux 3.0.0-21

gdb/ChangeLog:

    * source.c (show_substitute_path_command): Fix display of matching
    substitution rules

gdb/testsuite/ChangeLog:

    * subst.exp: Add tests to verify partial path matching output
---
 gdb/source.c                     | 2 +-
 gdb/testsuite/gdb.base/subst.exp | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdb/source.c b/gdb/source.c
index c112765..282acb5 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1879,7 +1879,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 || substitute_path_rule_matches (rule, from) != 0)
         printf_filtered ("  `%s' -> `%s'.\n", rule->from, rule->to);
       rule = rule->next;
     }
diff --git a/gdb/testsuite/gdb.base/subst.exp b/gdb/testsuite/gdb.base/subst.exp
index e132809..e99735b 100644
--- a/gdb/testsuite/gdb.base/subst.exp
+++ b/gdb/testsuite/gdb.base/subst.exp
@@ -95,6 +95,14 @@ gdb_test "show substitute-path depuis" \
          "Source path substitution rule matching `depuis':\r\n +`depuis' -> `vers'." \
          "show substitute-path depuis, after all paths added"
 
+gdb_test "show substitute-path from/path" \
+         "Source path substitution rule matching `from/path':\r\n +`from' -> `to'." \
+         "show substitute-path from/path, after all paths added"
+
+gdb_test "show substitute-path from_a_bad_path" \
+         "Source path substitution rule matching `from_a_bad_path':" \
+         "show substitute-path from_a_bad_path, after all paths added"
+
 gdb_test "show substitute-path garbage" \
          "Source path substitution rule matching `garbage':" \
          "show substitute-path garbage, after all paths added"
-- 
1.8.3-rc3

Re: [PATCH] gdb/source.c: Fix matching path substitute rule listing

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]

> This was tested on x86_64 Linux 3.0.0-21
> 
> gdb/ChangeLog:
> 
>     * source.c (show_substitute_path_command): Fix display of matching
>     substitution rules
> 
> gdb/testsuite/ChangeLog:
> 
>     * subst.exp: Add tests to verify partial path matching output

Thank you. This is mostly OK. I made the following little fixes:
  - In the ChangeLog, the sentences should all end with a period.
  - In the second ChangeLog, the patch to the file being changed
    should be relative to the directory where the ChangeLog is
    (missing "gdb.ada/" in this case).
  - For the platform on which this was tested, I couldn't tell what
    the 3.0.0-21 was about (kernel version?), but that part is usually
    not significant and often omitted, so I just removed it.
    I've also moved that information after the ChangeLog entries
    as we traditionally place it there, but that's just because
    I was already making minor edits. There are otherwise no requirement
    for the placement of that information.

I've pushed the attached commit under the "tiny patch" rule. There
are only so much code we'll be able to accept from you under that
rule, so if you think you might have other contributions coming,
you'll need to have a copyright assignment on file with the FSF
(it's a bit of a lengthy process, even if they seem to have improved
in the past).

Thanks again for the patch (and your patience!).
-- 
Joel

[-- Attachment #2: 0001-gdb-source.c-Fix-matching-path-substitute-rule-listi.patch --]
[-- Type: text/x-diff, Size: 4499 bytes --]

From 1e2ccb612d2b61014bb7e9fef3eb58e4947b9d2b Mon Sep 17 00:00:00 2001
From: Brad Mouring <bmouring@ni.com>
Date: Mon, 2 Jun 2014 15:55:10 -0500
Subject: [PATCH] gdb/source.c: Fix matching path substitute rule listing

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 (or NULL). 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 is a couple of tests added to subst.exp to verify
this behavior in the test suite.

gdb/ChangeLog:

        * source.c (show_substitute_path_command): Fix display of matching
        substitution rules.

gdb/testsuite/ChangeLog:

        * gdb.ada/subst.exp: Add tests to verify partial path matching
        output.

This was tested on x86_64 Linux.
---
 gdb/ChangeLog                    | 6 ++++++
 gdb/source.c                     | 2 +-
 gdb/testsuite/ChangeLog          | 5 +++++
 gdb/testsuite/gdb.base/subst.exp | 8 ++++++++
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index bda46b5..698c15c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2014-06-03  Brad Mouring  <bmouring@ni.com>  (tiny patch)
+
+	Pushed by Joel Brobecker  <brobecker@adacore.com>
+	* source.c (show_substitute_path_command): Fix display of matching
+	substitution rules.
+
 2014-06-03  Gary Benson  <gbenson@redhat.com>
 
 	* gnu-v2-abi.c (gnuv2_value_rtti_type): Use gdb_demangle.
diff --git a/gdb/source.c b/gdb/source.c
index c985a1b..14b1f71 100644
--- a/gdb/source.c
+++ b/gdb/source.c
@@ -1890,7 +1890,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 || substitute_path_rule_matches (rule, from) != 0)
         printf_filtered ("  `%s' -> `%s'.\n", rule->from, rule->to);
       rule = rule->next;
     }
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 8217403..40518b6 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-06-03  Brad Mouring  <bmouring@ni.com>  (tiny patch)
+
+	* gdb.base/subst.exp: Add tests to verify partial path matching
+	output.
+
 2014-06-03  Pedro Alves  <palves@redhat.com>
 
 	* gdb.base/sss-bp-on-user-bp-2.exp: Skip if testing with a remote
diff --git a/gdb/testsuite/gdb.base/subst.exp b/gdb/testsuite/gdb.base/subst.exp
index e132809..e99735b 100644
--- a/gdb/testsuite/gdb.base/subst.exp
+++ b/gdb/testsuite/gdb.base/subst.exp
@@ -95,6 +95,14 @@ gdb_test "show substitute-path depuis" \
          "Source path substitution rule matching `depuis':\r\n +`depuis' -> `vers'." \
          "show substitute-path depuis, after all paths added"
 
+gdb_test "show substitute-path from/path" \
+         "Source path substitution rule matching `from/path':\r\n +`from' -> `to'." \
+         "show substitute-path from/path, after all paths added"
+
+gdb_test "show substitute-path from_a_bad_path" \
+         "Source path substitution rule matching `from_a_bad_path':" \
+         "show substitute-path from_a_bad_path, after all paths added"
+
 gdb_test "show substitute-path garbage" \
          "Source path substitution rule matching `garbage':" \
          "show substitute-path garbage, after all paths added"
-- 
1.9.1