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

[Joel Brobecker <brobecker at adacore dot com>]

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