Re: [RFA] set/unset/show substitute-path commands (take 2)

[Daniel Jacobowitz <drow at false dot org>]

On Wed, Jul 19, 2006 at 11:15:16AM -0700, Joel Brobecker wrote:
> I adjusted the testcase according to the modifications in the code.
> I also tried to add a test that would verify that the substitution
> correctly takes place, but as expected, I'm having some trouble
> doing that. Here is what I did:
> 
>         * gdb.base/subst.exp
>         * gdb.base/subst.c
>         * gdb.base/subst.dir/subst.c
> 
> The version of subst.c in subst.dir is a modified version of subst.c.
> I built the subst executable using gdb.base/subst.c, as usual. And then
> tried to add a substitution rule as follow:
> 
>         (gdb) set substitute-path '${srcdir}/${subdir}' '${srcdir}/${subdir}/subst.dir'
> 
> But the problem is that ${srcdir} is not string equal to what compiler
> put in the debugging information. As far as I can tell from the testcase
> output, ${srcdir} is something like ../../gdb/testsuite. I don't see how
> to reliably get the source path in a way that would be identical to what
> the compiler sets.
> 
> Perhaps this is telling us that, instead of just FILENAME_CMP, we should
> use something more sophisticated like xfullpath to check for path equality.
> But I'm very reluctant to even suggest that, because performance would
> probably become horrendous on large applications where lots and lots of
> files are used.

Let's not go towards xfullpath for this please.  The whole problem is
that we don't want to rely on build-time paths if told not to; we
shouldn't be affected by what's on the disk now.

The best way to handle this is to ask the compiler.  Try loading the
binary and running "info source"; it will print out the compilation
directory.

>    /* Quick way out if we already know its full name */
> +
>    if (*fullname)
>      {
> +      {
> +        /* The user may have requested that source paths be rewritten
> +           according to substitution rules he provided.  If a substitution
> +           rule applies to this path, then apply it.  */
> +        char *rewritten_fullname = rewrite_source_path (*fullname);
> +
> +        if (rewritten_fullname != NULL)
> +           {
> +             xfree (*fullname);
> +             *fullname = rewritten_fullname;
> +           }
> +      }
> +
>        result = open (*fullname, OPEN_MODE);
>        if (result >= 0)
>  	return result;

You don't need the extra { } here.

> +/* If the last character of PATH is a directory separator, then strip it.  */
> +
> +static void
> +strip_trailing_directory_separator (char *path)
> +{
> +  const int last = strlen (path) - 1;
> +
> +  if (last < 0)
> +    return;  /* No stripping is needed if PATH is the empty string.  */
> +
> +  if (IS_DIR_SEPARATOR (path[last]))
> +    path[last] = '\0';
> +}

Is this going to get you in trouble if PATH is "/" ?  I can imagine
that happening.

> +/* A convenience function to push ARGV in the cleanup queue.  */
> +
> +static void
> +make_cleanup_argv (char **argv)
> +{
> +  make_cleanup ((make_cleanup_ftype *) freeargv, argv);
> +}

Casting function types is a good way to get in trouble.  In this case,
there's an answer handy: make_cleanup_freeargv.


Otherwise looks great!

-- 
Daniel Jacobowitz
CodeSourcery