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

[Joel Brobecker <brobecker at adacore dot com>]

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