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

["Brad Mouring" <bmouring at ni dot com>]

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