gdb/source.c: Fix source path substitution

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

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