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

[Doug Evans <dje at google dot com>]

On Wed, Jan 11, 2012 at 7:06 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> Greetings,
>
> Attached is a proposed fix for PR gdb/9538.
>
> Mostly it just moves code around, and adds a "try realpath(objfile->name)
> if searching with objfile->name fails."
>
> The added test fails before the patch, and succeeds after it.
>
> Tested on Linux/x86_64.
>
> Thanks,
>
> --
> Paul Pluzhnikov
>
>
> 2012-01-11 ?Paul Pluzhnikov ?<ppluzhnikov@google.com>
>
> ? ? ? ?PR gdb/9538
> ? ? ? ?* symfile.c (find_separate_debug_file): New function.
> ? ? ? ?(terminate_after_last_dir_separator): Likewise.
> ? ? ? ?(find_separate_debug_file_by_debuglink): Also try realpath.
>
>
> testsuite/ChangeLog:
>
> ? ? ? ?PR gdb/9538
> ? ? ? ?* gdb.base/sepdebug.exp: New test.

Howdy.
The symfile.c change is ok with me, modulo can you add comments for
each of the functions?
[caveat: I think it doesn't properly handle paths with dos drives, but
the code already has that problem, so no requirement to fix that in
this patch]

I see sepdebug.exp uses remote_exec so best use that instead of exec.
Also, I'm not sure what the portability requirements are w.r.t. symlinks.
Probably best to watch for errors in the "ln -s" and handle appropriately.
[Even better, is there a utility routine that will create a symlink?
All the portability concerns could be tucked away in there.]
Also, can you use clean_restart instead of the gdb_exit/gdb_start/... sequence?

Fine with those changes.


>
>
>
> Index: symfile.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/symfile.c,v
> retrieving revision 1.325
> diff -u -p -r1.325 symfile.c
> --- symfile.c ? 12 Jan 2012 00:00:01 -0000 ? ? ?1.325
> +++ symfile.c ? 12 Jan 2012 01:34:01 -0000
> @@ -1441,35 +1441,15 @@ show_debug_file_directory (struct ui_fil
> ?#define DEBUG_SUBDIRECTORY ".debug"
> ?#endif
>
> -char *
> -find_separate_debug_file_by_debuglink (struct objfile *objfile)
> -{
> - ?char *basename, *debugdir;
> - ?char *dir = NULL;
> - ?char *debugfile = NULL;
> - ?char *canon_name = NULL;
> - ?unsigned long crc32;
> +static char *
> +find_separate_debug_file (const char *dir, const char *debuglink,
> + ? ? ? ? ? ? ? ? ? ? ? ? unsigned long crc32, struct objfile *objfile)
> +{
> + ?char *debugdir;
> + ?char *debugfile;
> + ?char *canon_name;
> ? int i;
>
> - ?basename = get_debug_link_info (objfile, &crc32);
> -
> - ?if (basename == NULL)
> - ? ?/* There's no separate debug info, hence there's no way we could
> - ? ? ? load it => no warning. ?*/
> - ? ?goto cleanup_return_debugfile;
> -
> - ?dir = xstrdup (objfile->name);
> -
> - ?/* Strip off the final filename part, leaving the directory name,
> - ? ? followed by a slash. ?The directory can be relative or absolute. ?*/
> - ?for (i = strlen(dir) - 1; i >= 0; i--)
> - ? ?{
> - ? ? ?if (IS_DIR_SEPARATOR (dir[i]))
> - ? ? ? break;
> - ? ?}
> - ?/* If I is -1 then no directory is present there and DIR will be "". ?*/
> - ?dir[i+1] = '\0';
> -
> ? /* Set I to max (strlen (canon_name), strlen (dir)). ?*/
> ? canon_name = lrealpath (dir);
> ? i = strlen (dir);
> @@ -1480,12 +1460,12 @@ find_separate_debug_file_by_debuglink (s
> ? ? ? ? ? ? ? ? ? ? ? + i
> ? ? ? ? ? ? ? ? ? ? ? + strlen (DEBUG_SUBDIRECTORY)
> ? ? ? ? ? ? ? ? ? ? ? + strlen ("/")
> - ? ? ? ? ? ? ? ? ? ? ?+ strlen (basename)
> + ? ? ? ? ? ? ? ? ? ? ?+ strlen (debuglink)
> ? ? ? ? ? ? ? ? ? ? ? + 1);
>
> ? /* First try in the same directory as the original file. ?*/
> ? strcpy (debugfile, dir);
> - ?strcat (debugfile, basename);
> + ?strcat (debugfile, debuglink);
>
> ? if (separate_debug_file_exists (debugfile, crc32, objfile))
> ? ? goto cleanup_return_debugfile;
> @@ -1494,7 +1474,7 @@ find_separate_debug_file_by_debuglink (s
> ? strcpy (debugfile, dir);
> ? strcat (debugfile, DEBUG_SUBDIRECTORY);
> ? strcat (debugfile, "/");
> - ?strcat (debugfile, basename);
> + ?strcat (debugfile, debuglink);
>
> ? if (separate_debug_file_exists (debugfile, crc32, objfile))
> ? ? goto cleanup_return_debugfile;
> @@ -1520,7 +1500,7 @@ find_separate_debug_file_by_debuglink (s
> ? ? ? debugfile[debugdir_end - debugdir] = 0;
> ? ? ? strcat (debugfile, "/");
> ? ? ? strcat (debugfile, dir);
> - ? ? ?strcat (debugfile, basename);
> + ? ? ?strcat (debugfile, debuglink);
>
> ? ? ? if (separate_debug_file_exists (debugfile, crc32, objfile))
> ? ? ? ?goto cleanup_return_debugfile;
> @@ -1536,7 +1516,7 @@ find_separate_debug_file_by_debuglink (s
> ? ? ? ? ?debugfile[debugdir_end - debugdir] = 0;
> ? ? ? ? ?strcat (debugfile, canon_name + strlen (gdb_sysroot));
> ? ? ? ? ?strcat (debugfile, "/");
> - ? ? ? ? strcat (debugfile, basename);
> + ? ? ? ? strcat (debugfile, debuglink);
>
> ? ? ? ? ?if (separate_debug_file_exists (debugfile, crc32, objfile))
> ? ? ? ? ? ?goto cleanup_return_debugfile;
> @@ -1551,8 +1531,61 @@ find_separate_debug_file_by_debuglink (s
>
> ?cleanup_return_debugfile:
> ? xfree (canon_name);
> - ?xfree (basename);
> + ?return debugfile;
> +}
> +
> +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;
> + ? ?}
> + ?/* If I is -1 then no directory is present there and DIR will be "". ?*/
> + ?path[i+1] = '\0';
> +}
> +
> +char *
> +find_separate_debug_file_by_debuglink (struct objfile *objfile)
> +{
> + ?char *debuglink;
> + ?char *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;
> +
> + ?dir = xstrdup (objfile->name);
> + ?terminate_after_last_dir_separator (dir);
> +
> + ?debugfile = find_separate_debug_file (dir, debuglink, crc32, objfile);
> ? xfree (dir);
> +
> + ?if (debugfile != NULL)
> + ? ?goto cleanup_return_debugfile;
> +
> + ?/* For PR gdb/9538, try again with realpath. ?*/
> + ?dir = lrealpath (objfile->name);
> + ?if (dir == NULL)
> + ? ?goto cleanup_return_debugfile;
> +
> + ?terminate_after_last_dir_separator (dir);
> + ?debugfile = find_separate_debug_file (dir, debuglink, crc32, objfile);
> + ?xfree (dir);
> +
> + cleanup_return_debugfile:
> + ?xfree (debuglink);
> +
> ? 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 01:34:01 -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,9 +55,25 @@ if [gdb_gnu_strip_debug $binfile] {
> ? ? return -1
> ?}
>
> +#
> +# PR gdb/9538. ?Verify that symlinked executable still finds the separate
> +# debuginfo.
> +#
> ?gdb_exit
> ?gdb_start
> ?gdb_reinitialize_dir $srcdir/$subdir
> +set subsubdir ${objdir}/${subdir}/pr9538
> +exec mkdir ${subsubdir}
> +exec ln -s ${binfile} ${subsubdir}
> +gdb_load ${subsubdir}/${testfile}${EXEEXT}
> +if { $gdb_file_cmd_debug_info != "debug" } then {
> + ? ?fail "No debug information found."
> +}
> +gdb_exit
> +exec rm -rf ${subsubdir}
> +
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> ?gdb_load ${binfile}
> ?if { $gdb_file_cmd_debug_info != "debug" } then {
> ? ? fail "No debug information found."