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

[Doug Evans <dje@google.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."