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

Paul Pluzhnikov
2012-01-12T22:27:39Z

[-- Attachment #1: Type: text/plain, Size: 6090 bytes --]

On Thu, Jan 12, 2012 at 1:29 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:

> 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.

Done.

>> +  /* 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'.

Done.

>> +}
>> +
>> +/* 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.

Where? canon_dir is passed into the utility function, which checks for NULL
(as it did before).

>> +
>> +  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.

Wouldn't lstat need a configury #ifdef to make it build?

But there is no need to do realpath on the directory again, so I just pass
dir2 as canon_dir.

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

Not all of them. I've hit this bug while debugging libc on pre-release
Ubuntu.

>> +  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;
>        }

Done.

>
>> +
>> +  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.

Done.

>
>> +  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.

Done.

>
>
>> +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.

Ok. We now leave state as is, and cleanup just before re-creating it.

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

Re-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.

[-- Attachment #2: gdb-symlink-pr9538-20120112a.txt --]
[-- Type: text/plain, Size: 7986 bytes --]

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 22:11:17 -0000
@@ -1441,63 +1441,48 @@ show_debug_file_directory (struct ui_fil
#define DEBUG_SUBDIRECTORY ".debug"
#endif

-char *
-find_separate_debug_file_by_debuglink (struct objfile *objfile)
+/* Find a separate debuginfo file for OBJFILE, using DIR as the directory
+ where the original file resides (may not be the same as
+ dirname(objfile->name) due to symlinks), and DEBUGLINK as the file we are
+ looking for. Returns the name of the debuginfo, of NULL. */
+
+static char *
+find_separate_debug_file (const char *dir,
+ const char *canon_dir,
+ const char *debuglink,
+ unsigned long crc32, struct objfile *objfile)
{
- char *basename, *debugdir;
- char *dir = NULL;
- char *debugfile = NULL;
- char *canon_name = NULL;
- unsigned long crc32;
+ char *debugdir;
+ char *debugfile;
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);
+ /* Set I to max (strlen (canon_dir), strlen (dir)). */
i = strlen (dir);
- if (canon_name && strlen (canon_name) > i)
- i = strlen (canon_name);
+ if (canon_dir != NULL && strlen (canon_dir) > i)
+ i = strlen (canon_dir);

debugfile = xmalloc (strlen (debug_file_directory) + 1
+ 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;
+ return debugfile;

/* Then try in the subdirectory named DEBUG_SUBDIRECTORY. */
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;
+ return debugfile;

/* Then try in the global debugfile directories.

@@ -1520,26 +1505,26 @@ 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;
+ return debugfile;

/* If the file is in the sysroot, try using its base path in the
global debugfile directory. */
- if (canon_name
- && filename_ncmp (canon_name, gdb_sysroot,
+ if (canon_dir != NULL
+ && filename_ncmp (canon_dir, gdb_sysroot,
strlen (gdb_sysroot)) == 0
- && IS_DIR_SEPARATOR (canon_name[strlen (gdb_sysroot)]))
+ && IS_DIR_SEPARATOR (canon_dir[strlen (gdb_sysroot)]))
{
memcpy (debugfile, debugdir, debugdir_end - debugdir);
debugfile[debugdir_end - debugdir] = 0;
- strcat (debugfile, canon_name + strlen (gdb_sysroot));
+ strcat (debugfile, canon_dir + strlen (gdb_sysroot));
strcat (debugfile, "/");
- strcat (debugfile, basename);
+ strcat (debugfile, debuglink);

if (separate_debug_file_exists (debugfile, crc32, objfile))
- goto cleanup_return_debugfile;
+ return debugfile;
}

debugdir = debugdir_end;
@@ -1547,12 +1532,78 @@ find_separate_debug_file_by_debuglink (s
while (*debugdir != 0);

xfree (debugfile);
- debugfile = NULL;
+ return NULL;
+}
+
+/* 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;
+
+ /* If I is -1 then no directory is present there and DIR will be "". */
+ path[i + 1] = '\0';
+}
+
+/* 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;
+ struct cleanup *cleanups;
+
+ 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);
+ cleanups = make_cleanup (xfree, dir1);
+ terminate_after_last_dir_separator (dir1);
+ canon_dir = lrealpath (dir1);
+
+ debugfile = find_separate_debug_file (dir1, canon_dir, debuglink,
+ crc32, objfile);
+ xfree (canon_dir);
+
+ if (debugfile != NULL)
+ goto cleanup;
+
+ /* For PR gdb/9538, try again with realpath (if different from the
+ original). */
+ dir2 = lrealpath (objfile->name);
+ if (dir2 == NULL)
+ goto cleanup;
+
+ cleanups = make_cleanup (xfree, dir2);
+ terminate_after_last_dir_separator (dir2);
+ if (strcmp (dir1, dir2) == 0)
+ {
+ /* Same directory, no point retrying. */
+ goto cleanup;
+ }
+
+ debugfile = find_separate_debug_file (dir2, dir2, debuglink,
+ crc32, objfile);

-cleanup_return_debugfile:
- xfree (canon_name);
- xfree (basename);
- xfree (dir);
+ cleanup:
+ do_cleanups (cleanups);
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 22:11:17 -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,27 @@ if [gdb_gnu_strip_debug $binfile] {
return -1
}

-gdb_exit
-gdb_start
-gdb_reinitialize_dir $srcdir/$subdir
-gdb_load ${binfile}
+#
+# PR gdb/9538. Verify that symlinked executable still finds the separate
+# debuginfo.
+#
+set old_subdir ${subdir}
+set subdir ${subdir}/pr9538
+
+# Cleanup any stale state.
+remote_exec build "rm -rf ${subdir}"
+
+remote_exec build "mkdir ${subdir}"
+remote_exec build "ln -s ${binfile} ${subdir}"
+clean_restart ${testfile}${EXEEXT}
+if { $gdb_file_cmd_debug_info != "debug" } then {
+ fail "No debug information found."
+}
+
+# Restore subdir
+set subdir ${old_subdir}
+
+clean_restart ${testfile}${EXEEXT}
if { $gdb_file_cmd_debug_info != "debug" } then {
fail "No debug information found."
}