Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).

Jan Kratochvil
2012-01-12T21:30:23Z

On Thu, 12 Jan 2012 21:35:21 +0100, Paul Pluzhnikov wrote:
> +/* Modify PATH to contain only "directory/" part of PATH.
> + If there were no, directory separators in PATH, PATH will be empty
> + string on return. */
> +
> +static void
> +terminate_after_last_dir_separator (char *path)
> +{
> + int i;
> +
> + /* Strip off the final filename part, leaving the directory name,
> + followed by a slash. The directory can be relative or absolute. */
> + for (i = strlen(path) - 1; i >= 0; i--)
> + {
> + if (IS_DIR_SEPARATOR (path[i]))
> + break;
> + }

Empty line.

> + /* If I is -1 then no directory is present there and DIR will be "". */
> + path[i+1] = '\0';

It was there already but there should be `i + 1'.

> +}
> +
> +/* Find separate debuginfo for OBJFILE (using .gnu_debuglink section).
> + Returns pathname, or NULL. */
> +
> +char *
> +find_separate_debug_file_by_debuglink (struct objfile *objfile)
> +{
> + char *debuglink;
> + char *dir1, *dir2, *canon_dir;
> + char *debugfile;
> + unsigned long crc32;
> +
> + debuglink = get_debug_link_info (objfile, &crc32);
> +
> + if (debuglink == NULL)
> + /* There's no separate debug info, hence there's no way we could
> + load it => no warning. */
> + return NULL;
> +
> + dir1 = xstrdup (objfile->name);
> + terminate_after_last_dir_separator (dir1);
> + canon_dir = lrealpath (dir1);

lrealpath can return NULL. GDB did not crash before. It will now.

> +
> + debugfile = find_separate_debug_file (dir1, canon_dir, debuglink,
> + crc32, objfile);
> + xfree (canon_dir);
> +
> + if (debugfile != NULL)
> + goto cleanup1;
> +
> + /* For PR gdb/9538, try again with realpath (if different from the
> + original). */
> + dir2 = lrealpath (objfile->name);

Maybe some optimization would be helpful. realpath is expensive and the
directory path is already canonicalized. Something like lstat (objfile->name)
and do this step only if it is a symlink.

Not a requirement from me (modern systems use .build-id anyway).

> + if (dir2 == NULL)
> + goto cleanup1;
> +
> + terminate_after_last_dir_separator (dir2);
> + if (strcmp (dir1, dir2) == 0)
> + /* Same directory, no point retrying. */
> + goto cleanup2;

There was some discussion with conclusion it should be written as, nitpick:
if (strcmp (dir1, dir2) == 0)
{
/* Same directory, no point retrying. */
goto cleanup2;
}

> +
> + canon_dir = lrealpath (dir2);
> + debugfile = find_separate_debug_file (dir2, canon_dir, debuglink,
> + crc32, objfile);
> + xfree (canon_dir);
> +
> + cleanup2:
> + xfree (dir2);
> + cleanup1:

Maybe it should be finally rewritten to cleanups but it may be out of the
scope of this patch.

> + xfree (dir1);
> + xfree (debuglink);
>
> -cleanup_return_debugfile:
> - xfree (canon_name);
> - xfree (basename);
> - xfree (dir);
> return debugfile;
> }
>
> Index: testsuite/gdb.base/sepdebug.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/sepdebug.exp,v
> retrieving revision 1.33
> diff -u -p -r1.33 sepdebug.exp
> --- testsuite/gdb.base/sepdebug.exp 4 Jan 2012 08:17:46 -0000 1.33
> +++ testsuite/gdb.base/sepdebug.exp 12 Jan 2012 20:29:53 -0000
> @@ -45,7 +45,7 @@ if { [gdb_compile "${srcdir}/${subdir}/
>
> # Note: the procedure gdb_gnu_strip_debug will produce an executable called
> # ${binfile}, which is just like the executable ($binfile) but without
> -# the debuginfo. Instead $binfile has a .gnudebuglink section which contains
> +# the debuginfo. Instead $binfile has a .gnu_debuglink section which contains
> # the name of a debuginfo only file. This file will be stored in the
> # gdb.base/ subdirectory.
>
> @@ -55,10 +55,26 @@ if [gdb_gnu_strip_debug $binfile] {
> return -1
> }
>
> +#
> +# PR gdb/9538. Verify that symlinked executable still finds the separate
> +# debuginfo.
> +#
> +set old_subdir ${subdir}
> +set subdir ${subdir}/pr9538
> +remote_exec build "mkdir ${subdir}"
> +remote_exec build "ln -s ${binfile} ${subdir}"

You should clean up first any stale files there.

> +clean_restart ${testfile}${EXEEXT}
> +if { $gdb_file_cmd_debug_info != "debug" } then {
> + fail "No debug information found."
> +}
> +
> +# make sure we are not holding subdir/binary open.
> gdb_exit
> -gdb_start
> -gdb_reinitialize_dir $srcdir/$subdir
> -gdb_load ${binfile}
> +
> +remote_exec build "rm -rf ${subdir}"

It is not great the FAIL is not easily reproducible after it happens.

> +set subdir ${old_subdir}
> +
> +clean_restart ${testfile}${EXEEXT}
> if { $gdb_file_cmd_debug_info != "debug" } then {
> fail "No debug information found."
> }

Thanks,
Jan