* [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
@ 2012-01-12 3:12 Paul Pluzhnikov
2012-01-12 17:28 ` Doug Evans
0 siblings, 1 reply; 33+ messages in thread
From: Paul Pluzhnikov @ 2012-01-12 3:12 UTC (permalink / raw)
To: gdb-patches; +Cc: ppluzhnikov
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.
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."
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 3:12 [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks) Paul Pluzhnikov
@ 2012-01-12 17:28 ` Doug Evans
2012-01-12 20:59 ` Paul Pluzhnikov
0 siblings, 1 reply; 33+ messages in thread
From: Doug Evans @ 2012-01-12 17:28 UTC (permalink / raw)
To: Paul Pluzhnikov; +Cc: gdb-patches
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."
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 17:28 ` Doug Evans
@ 2012-01-12 20:59 ` Paul Pluzhnikov
2012-01-12 21:31 ` Jan Kratochvil
2012-01-12 22:26 ` Doug Evans
0 siblings, 2 replies; 33+ messages in thread
From: Paul Pluzhnikov @ 2012-01-12 20:59 UTC (permalink / raw)
To: Doug Evans; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1514 bytes --]
On Thu, Jan 12, 2012 at 9:23 AM, Doug Evans <dje@google.com> wrote:
> The symfile.c change is ok with me, modulo can you add comments for
> each of the functions?
Done.
I've also refactored the code a bit more to avoid searching the same
directory again.
> [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 don't think ".gnu_debuglink" is supported on non-ELF platforms, so not
handling DOS drives is likely not a problem.
> I see sepdebug.exp uses remote_exec so best use that instead of exec.
Done.
> 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.
The test exits if gdb_gnu_strip_debug fails. I don't believe it will
currently succeed on any platform without symlinks.
Testing for errors will just add noise.
> [Even better, is there a utility routine that will create a symlink?
> All the portability concerns could be tucked away in there.]
I didn't find one.
> Also, can you use clean_restart instead of the gdb_exit/gdb_start/... sequence?
Done.
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-20120112.txt --]
[-- Type: text/plain, Size: 7965 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 20:29:53 -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 && 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,80 @@ 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;
+
+ 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);
+
+ 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);
+ if (dir2 == NULL)
+ goto cleanup1;
+
+ terminate_after_last_dir_separator (dir2);
+ 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:
+ 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}"
+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}"
+set subdir ${old_subdir}
+
+clean_restart ${testfile}${EXEEXT}
if { $gdb_file_cmd_debug_info != "debug" } then {
fail "No debug information found."
}
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 20:59 ` Paul Pluzhnikov
@ 2012-01-12 21:31 ` Jan Kratochvil
2012-01-12 22:20 ` Paul Pluzhnikov
2012-01-12 22:29 ` Paul Pluzhnikov
2012-01-12 22:26 ` Doug Evans
1 sibling, 2 replies; 33+ messages in thread
From: Jan Kratochvil @ 2012-01-12 21:31 UTC (permalink / raw)
To: Paul Pluzhnikov; +Cc: Doug Evans, gdb-patches
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 21:31 ` Jan Kratochvil
@ 2012-01-12 22:20 ` Paul Pluzhnikov
2012-01-12 22:27 ` Jan Kratochvil
2012-01-12 22:29 ` Paul Pluzhnikov
1 sibling, 1 reply; 33+ messages in thread
From: Paul Pluzhnikov @ 2012-01-12 22:20 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches
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.
Done. Also no need to do realpath on the directory again.
> Not a requirement from me (modern systems use .build-id anyway).
Not all of them. I've hit this 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.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 20:59 ` Paul Pluzhnikov
2012-01-12 21:31 ` Jan Kratochvil
@ 2012-01-12 22:26 ` Doug Evans
2012-01-12 22:40 ` Paul Pluzhnikov
1 sibling, 1 reply; 33+ messages in thread
From: Doug Evans @ 2012-01-12 22:26 UTC (permalink / raw)
To: Paul Pluzhnikov; +Cc: gdb-patches
On Thu, Jan 12, 2012 at 12:35 PM, Paul Pluzhnikov
<ppluzhnikov@google.com> wrote:
> I don't think ".gnu_debuglink" is supported on non-ELF platforms, so not
> handling DOS drives is likely not a problem.
cygwin?
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 22:20 ` Paul Pluzhnikov
@ 2012-01-12 22:27 ` Jan Kratochvil
2012-01-12 22:31 ` Paul Pluzhnikov
0 siblings, 1 reply; 33+ messages in thread
From: Jan Kratochvil @ 2012-01-12 22:27 UTC (permalink / raw)
To: Paul Pluzhnikov; +Cc: Doug Evans, gdb-patches
On Thu, 12 Jan 2012 23:19:17 +0100, Paul Pluzhnikov wrote:
> On Thu, Jan 12, 2012 at 1:29 PM, Jan Kratochvil
> <jan.kratochvil@redhat.com> wrote:
> >> + Â 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).
I misread some comment as a code before, sorry. I agree.
> 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.
I do not see the new patch but the described changes look OK.
Thanks,
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 21:31 ` Jan Kratochvil
2012-01-12 22:20 ` Paul Pluzhnikov
@ 2012-01-12 22:29 ` Paul Pluzhnikov
2012-01-12 22:50 ` Jan Kratochvil
2012-01-12 23:25 ` Doug Evans
1 sibling, 2 replies; 33+ messages in thread
From: Paul Pluzhnikov @ 2012-01-12 22:29 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches
[-- 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."
}
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 22:27 ` Jan Kratochvil
@ 2012-01-12 22:31 ` Paul Pluzhnikov
0 siblings, 0 replies; 33+ messages in thread
From: Paul Pluzhnikov @ 2012-01-12 22:31 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches
On Thu, Jan 12, 2012 at 2:26 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> I do not see the new patch but the described changes look OK.
Sorry, clicked the wrong button in GMail before the message was ready ;-(
--
Paul Pluzhnikov
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 22:26 ` Doug Evans
@ 2012-01-12 22:40 ` Paul Pluzhnikov
2012-01-12 22:52 ` Doug Evans
0 siblings, 1 reply; 33+ messages in thread
From: Paul Pluzhnikov @ 2012-01-12 22:40 UTC (permalink / raw)
To: Doug Evans; +Cc: gdb-patches
On Thu, Jan 12, 2012 at 2:20 PM, Doug Evans <dje@google.com> wrote:
> On Thu, Jan 12, 2012 at 12:35 PM, Paul Pluzhnikov
> <ppluzhnikov@google.com> wrote:
>> I don't think ".gnu_debuglink" is supported on non-ELF platforms, so not
>> handling DOS drives is likely not a problem.
>
> cygwin?
Cygwin appears to support (simulate) symlinks.
--
Paul Pluzhnikov
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 22:29 ` Paul Pluzhnikov
@ 2012-01-12 22:50 ` Jan Kratochvil
2012-01-12 23:12 ` Paul Pluzhnikov
2012-01-12 23:25 ` Doug Evans
1 sibling, 1 reply; 33+ messages in thread
From: Jan Kratochvil @ 2012-01-12 22:50 UTC (permalink / raw)
To: Paul Pluzhnikov; +Cc: Doug Evans, gdb-patches
On Thu, 12 Jan 2012 23:26:39 +0100, Paul Pluzhnikov wrote:
> Wouldn't lstat need a configury #ifdef to make it build?
Yes. There exists lstat in gnulib but configury #ifdef would be more
appropriate here I think.
> But there is no need to do realpath on the directory again, so I just pass
> dir2 as canon_dir.
Still you call there:
+ dir1 = xstrdup (objfile->name);
+ canon_dir = lrealpath (dir1);
and later:
+ dir2 = lrealpath (objfile->name);
For all the directory components of objfile->name lrealpath will lstat them by
syscalls twice (for NFS it may get hopefully cached, not sure). There were
some complaints on GDB slowness on NFS.
I do not require this optimization, there are still worse issues on NFS.
Thanks,
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 22:40 ` Paul Pluzhnikov
@ 2012-01-12 22:52 ` Doug Evans
0 siblings, 0 replies; 33+ messages in thread
From: Doug Evans @ 2012-01-12 22:52 UTC (permalink / raw)
To: Paul Pluzhnikov; +Cc: gdb-patches
On Thu, Jan 12, 2012 at 2:32 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Thu, Jan 12, 2012 at 2:20 PM, Doug Evans <dje@google.com> wrote:
>
>> On Thu, Jan 12, 2012 at 12:35 PM, Paul Pluzhnikov
>> <ppluzhnikov@google.com> wrote:
>>> I don't think ".gnu_debuglink" is supported on non-ELF platforms, so not
>>> handling DOS drives is likely not a problem.
>>
>> cygwin?
>
> Cygwin appears to support (simulate) symlinks.
Yeah, and guess who added that support ... :-)
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 22:50 ` Jan Kratochvil
@ 2012-01-12 23:12 ` Paul Pluzhnikov
2012-01-12 23:18 ` Jan Kratochvil
0 siblings, 1 reply; 33+ messages in thread
From: Paul Pluzhnikov @ 2012-01-12 23:12 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches
On Thu, Jan 12, 2012 at 2:40 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> There exists lstat in gnulib
There doesn't appear to be one in my (freshly sync'd) copy:
grep -r lstat gnulib
gnulib/Makefile.in:localstatedir = @localstatedir@
> Still you call there:
> + dir1 = xstrdup (objfile->name);
> + canon_dir = lrealpath (dir1);
> and later:
> + dir2 = lrealpath (objfile->name);
If objfile->name is "/foo/bar/baz", and is a symlink to "/zork/zark/baz"
(and nothing else is a symlink) then the two realpath() calls will return
"/foo/bar" and "/zork/zark/baz" and (I think) are unavoidable.
Thanks,
--
Paul Pluzhnikov
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 23:12 ` Paul Pluzhnikov
@ 2012-01-12 23:18 ` Jan Kratochvil
0 siblings, 0 replies; 33+ messages in thread
From: Jan Kratochvil @ 2012-01-12 23:18 UTC (permalink / raw)
To: Paul Pluzhnikov; +Cc: Doug Evans, gdb-patches
On Fri, 13 Jan 2012 00:05:06 +0100, Paul Pluzhnikov wrote:
> On Thu, Jan 12, 2012 at 2:40 PM, Jan Kratochvil
> <jan.kratochvil@redhat.com> wrote:
>
> > There exists lstat in gnulib
>
> There doesn't appear to be one in my (freshly sync'd) copy:
>
> grep -r lstat gnulib
> gnulib/Makefile.in:localstatedir = @localstatedir@
I meant upstream gnulib. GDB contains only several pieces of it already in
use by GDB. We could import more of them, if needed.
> > Still you call there:
> > + Â dir1 = xstrdup (objfile->name);
> > + Â canon_dir = lrealpath (dir1);
> > and later:
> > + Â dir2 = lrealpath (objfile->name);
>
> If objfile->name is "/foo/bar/baz", and is a symlink to "/zork/zark/baz"
> (and nothing else is a symlink) then the two realpath() calls will return
> "/foo/bar" and "/zork/zark/baz" and (I think) are unavoidable.
But in this case "baz" is a symlink. So `lstat (objfile->name)' would report
it is a symlink. This would indicate we should run second lrealpath.
In most cases `lstat (objfile->name)' will report is is a regular file.
In such case the second (new) part of the function can be IMO skipped as it
cannot find anything more.
Thanks,
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 22:29 ` Paul Pluzhnikov
2012-01-12 22:50 ` Jan Kratochvil
@ 2012-01-12 23:25 ` Doug Evans
2012-01-12 23:27 ` Jan Kratochvil
2012-01-12 23:28 ` [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks) Paul Pluzhnikov
1 sibling, 2 replies; 33+ messages in thread
From: Doug Evans @ 2012-01-12 23:25 UTC (permalink / raw)
To: Paul Pluzhnikov; +Cc: Jan Kratochvil, gdb-patches
On Thu, Jan 12, 2012 at 2:26 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>>> +
>>> + 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?
[for reference sake, one patch in my basenames-may-differ series (not
submitted) had a use for lstat.
IWBN to have lstat in gdb so we could (appropriately of course) use it.
OOC, Jan, what discussion led rise to having braces here:
+ if (strcmp (dir1, dir2) == 0)
+ {
+ /* Same directory, no point retrying. */
+ goto cleanup;
+ }
and does that reasoning apply here:
+ if (debuglink == NULL)
+ /* There's no separate debug info, hence there's no way we could
+ load it => no warning. */
+ return NULL;
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 23:25 ` Doug Evans
@ 2012-01-12 23:27 ` Jan Kratochvil
2012-01-12 23:40 ` Doug Evans
2012-01-12 23:28 ` [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks) Paul Pluzhnikov
1 sibling, 1 reply; 33+ messages in thread
From: Jan Kratochvil @ 2012-01-12 23:27 UTC (permalink / raw)
To: Doug Evans; +Cc: Paul Pluzhnikov, gdb-patches
On Fri, 13 Jan 2012 00:17:55 +0100, Doug Evans wrote:
> OOC, Jan, what discussion led rise to having braces here:
There was only Mark's agreement with me as I read now:
Re: Code formatting [Re: [patch] s390*: watchpoints regression [repost]]
http://sourceware.org/ml/gdb-patches/2011-12/msg00600.html
So still not sure what is the general consensus.
> and does that reasoning apply here:
>
> + if (debuglink == NULL)
> + /* There's no separate debug info, hence there's no way we could
> + load it => no warning. */
> + return NULL;
There exist already many cases in GDB not compliant with this rule.
Regards,
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 23:25 ` Doug Evans
2012-01-12 23:27 ` Jan Kratochvil
@ 2012-01-12 23:28 ` Paul Pluzhnikov
2012-01-12 23:55 ` Jan Kratochvil
1 sibling, 1 reply; 33+ messages in thread
From: Paul Pluzhnikov @ 2012-01-12 23:28 UTC (permalink / raw)
To: Doug Evans; +Cc: Jan Kratochvil, gdb-patches
On Thu, Jan 12, 2012 at 3:17 PM, Doug Evans <dje@google.com> wrote:
>> Wouldn't lstat need a configury #ifdef to make it build?
>
> [for reference sake, one patch in my basenames-may-differ series (not
> submitted) had a use for lstat.
> IWBN to have lstat in gdb so we could (appropriately of course) use it.
Right.
If there was an existing HAVE_LSTAT facility, I would definitely be for
using it here. But I am not sufficiently proficient with configury to add
it myself.
> OOC, Jan, what discussion led rise to having braces here:
>
> + if (strcmp (dir1, dir2) == 0)
> + {
> + /* Same directory, no point retrying. */
> + goto cleanup;
> + }
>
> and does that reasoning apply here:
>
> + if (debuglink == NULL)
> + /* There's no separate debug info, hence there's no way we could
> + load it => no warning. */
> + return NULL;
I've added braces here (personally, I am all for braces ;-)
--
Paul Pluzhnikov
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 23:27 ` Jan Kratochvil
@ 2012-01-12 23:40 ` Doug Evans
2012-01-13 0:21 ` [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).] Jan Kratochvil
0 siblings, 1 reply; 33+ messages in thread
From: Doug Evans @ 2012-01-12 23:40 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Paul Pluzhnikov, gdb-patches
On Thu, Jan 12, 2012 at 3:25 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 13 Jan 2012 00:17:55 +0100, Doug Evans wrote:
>> OOC, Jan, what discussion led rise to having braces here:
>
> There was only Mark's agreement with me as I read now:
> Re: Code formatting [Re: [patch] s390*: watchpoints regression [repost]]
> http://sourceware.org/ml/gdb-patches/2011-12/msg00600.html
>
> So still not sure what is the general consensus.
Thanks.
I think we should make the braces required.
>> and does that reasoning apply here:
>>
>> + if (debuglink == NULL)
>> + /* There's no separate debug info, hence there's no way we could
>> + load it => no warning. */
>> + return NULL;
>
> There exist already many cases in GDB not compliant with this rule.
Yeah. Asking for it is a start.
>
>
> Regards,
> Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 23:28 ` [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks) Paul Pluzhnikov
@ 2012-01-12 23:55 ` Jan Kratochvil
2012-01-13 0:21 ` Paul Pluzhnikov
0 siblings, 1 reply; 33+ messages in thread
From: Jan Kratochvil @ 2012-01-12 23:55 UTC (permalink / raw)
To: Paul Pluzhnikov; +Cc: Doug Evans, gdb-patches
On Fri, 13 Jan 2012 00:26:29 +0100, Paul Pluzhnikov wrote:
> If there was an existing HAVE_LSTAT facility, I would definitely be for
> using it here. But I am not sufficiently proficient with configury to add
> it myself.
If you find it useful here is a patch. For lstat it is AFAIK simple.
Thanks,
Jan
gdb/
2012-01-12 Jan Kratochvil <jan.kratochvil@redhat.com>
* configure.ac (AC_CHECK_FUNCS): Add lstat.
* configure: Regenerate.
* config.in: Regenerate.
--- gdb/configure.ac 4 Jan 2012 08:17:00 -0000 1.152
+++ gdb/configure.ac 12 Jan 2012 23:40:36 -0000
@@ -1059,7 +1059,7 @@ AC_CHECK_FUNCS([canonicalize_file_name r
getgid pipe poll pread64 resize_term sbrk setpgid setpgrp setsid \
sigaction sigprocmask sigsetmask socketpair syscall \
ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
- setrlimit getrlimit posix_madvise waitpid])
+ setrlimit getrlimit posix_madvise waitpid lstat])
AM_LANGINFO_CODESET
# Check the return and argument types of ptrace. No canned test for
--- gdb/configure 12 Jan 2012 23:38:47 -0000 1.337
+++ gdb/configure 12 Jan 2012 23:40:34 -0000
@@ -12918,7 +12918,7 @@ for ac_func in canonicalize_file_name re
getgid pipe poll pread64 resize_term sbrk setpgid setpgrp setsid \
sigaction sigprocmask sigsetmask socketpair syscall \
ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
- setrlimit getrlimit posix_madvise waitpid
+ setrlimit getrlimit posix_madvise waitpid lstat
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
--- gdb/config.in 12 Jan 2012 23:38:47 -0000 1.130
+++ gdb/config.in 12 Jan 2012 23:40:28 -0000
@@ -260,6 +260,9 @@
/* Define to 1 if the system has the type `long long int'. */
#undef HAVE_LONG_LONG_INT
+/* Define to 1 if you have the `lstat' function. */
+#undef HAVE_LSTAT
+
/* Define if <sys/procfs.h> has lwpid_t. */
#undef HAVE_LWPID_T
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-12 23:55 ` Jan Kratochvil
@ 2012-01-13 0:21 ` Paul Pluzhnikov
2012-01-13 0:48 ` Jan Kratochvil
0 siblings, 1 reply; 33+ messages in thread
From: Paul Pluzhnikov @ 2012-01-13 0:21 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches
[-- Attachment #1: Type: text/plain, Size: 619 bytes --]
On Thu, Jan 12, 2012 at 3:42 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> If you find it useful here is a patch. For lstat it is AFAIK simple.
That *was* easy ;-)
Could you commit it?
Updated patch attached. (I also noticed incorrect use of make_cleanup;
now fixed.)
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-20120112b.txt --]
[-- Type: text/plain, Size: 8239 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 13 Jan 2012 00:15:19 -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,91 @@ 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 *dir, *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;
+ }
+
+ dir = xstrdup (objfile->name);
+ cleanups = make_cleanup (xfree, dir);
+ terminate_after_last_dir_separator (dir);
+ canon_dir = lrealpath (dir);
+
+ debugfile = find_separate_debug_file (dir, canon_dir, debuglink,
+ crc32, objfile);
+ xfree (canon_dir);
+
+ if (debugfile != NULL)
+ goto cleanup;
+
+#ifdef HAVE_LSTAT
+
+ /* For PR gdb/9538, try again with realpath (if different from the
+ original). */
+ {
+ char *symlink_dir;
+ struct stat st_buf;
+
+ if (lstat (objfile->name, &st_buf) != 0 || !S_ISLNK(st_buf.st_mode))
+ goto cleanup;
+
+ symlink_dir = lrealpath (objfile->name);
+ if (symlink_dir == NULL)
+ goto cleanup;
+
+ make_cleanup (xfree, symlink_dir);
+ terminate_after_last_dir_separator (symlink_dir);
+ if (strcmp (dir, symlink_dir) == 0)
+ {
+ /* Same directory, no point retrying. */
+ goto cleanup;
+ }
+
+ debugfile = find_separate_debug_file (symlink_dir, symlink_dir, debuglink,
+ crc32, objfile);
+ }
+#endif /* HAVE_LSTAT */
-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 13 Jan 2012 00:15:19 -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."
}
^ permalink raw reply [flat|nested] 33+ messages in thread
* [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]
2012-01-12 23:40 ` Doug Evans
@ 2012-01-13 0:21 ` Jan Kratochvil
2012-01-13 0:27 ` Doug Evans
2012-01-13 9:05 ` Eli Zaretskii
0 siblings, 2 replies; 33+ messages in thread
From: Jan Kratochvil @ 2012-01-13 0:21 UTC (permalink / raw)
To: Doug Evans; +Cc: Paul Pluzhnikov, gdb-patches
On Fri, 13 Jan 2012 00:28:35 +0100, Doug Evans wrote:
> I think we should make the braces required.
OK to check in?
Thanks,
Jan
gdb/doc/
2012-01-12 Jan Kratochvil <jan.kratochvil@redhat.com>
* gdbint.texinfo (Coding Standards): Require braces for two lines of
code.
--- a/gdb/doc/gdbint.texinfo
+++ b/gdb/doc/gdbint.texinfo
@@ -5849,6 +5849,26 @@ the following guidelines:
@tab (pointer dereference)
@end multitable
+Any two lines in code should be wrapped in braces as they look as separate
+statements:
+
+@smallexample
+if (i)
+ @{
+ /* Return success. */
+ return 0;
+ @}
+@end smallexample
+
+@noindent
+and not:
+
+@smallexample
+if (i)
+ /* Return success. */
+ return 0;
+@end smallexample
+
@subsection Comments
@cindex comment formatting
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]
2012-01-13 0:21 ` [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).] Jan Kratochvil
@ 2012-01-13 0:27 ` Doug Evans
2012-01-13 4:02 ` Joel Brobecker
2012-01-13 9:05 ` Eli Zaretskii
1 sibling, 1 reply; 33+ messages in thread
From: Doug Evans @ 2012-01-13 0:27 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Paul Pluzhnikov, gdb-patches
On Thu, Jan 12, 2012 at 3:54 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 13 Jan 2012 00:28:35 +0100, Doug Evans wrote:
>> I think we should make the braces required.
>
> OK to check in?
Ok by me!
[give it awhile to see if anyone objects :-)]
>
>
> Thanks,
> Jan
>
>
> gdb/doc/
> 2012-01-12 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * gdbint.texinfo (Coding Standards): Require braces for two lines of
> code.
>
> --- a/gdb/doc/gdbint.texinfo
> +++ b/gdb/doc/gdbint.texinfo
> @@ -5849,6 +5849,26 @@ the following guidelines:
> @tab (pointer dereference)
> @end multitable
>
> +Any two lines in code should be wrapped in braces as they look as separate
> +statements:
> +
> +@smallexample
> +if (i)
> + @{
> + /* Return success. */
> + return 0;
> + @}
> +@end smallexample
> +
> +@noindent
> +and not:
> +
> +@smallexample
> +if (i)
> + /* Return success. */
> + return 0;
> +@end smallexample
> +
> @subsection Comments
>
> @cindex comment formatting
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-13 0:21 ` Paul Pluzhnikov
@ 2012-01-13 0:48 ` Jan Kratochvil
2012-01-13 3:39 ` Paul Pluzhnikov
0 siblings, 1 reply; 33+ messages in thread
From: Jan Kratochvil @ 2012-01-13 0:48 UTC (permalink / raw)
To: Paul Pluzhnikov; +Cc: Doug Evans, gdb-patches
On Fri, 13 Jan 2012 01:20:37 +0100, Paul Pluzhnikov wrote:
> Could you commit it?
Feel free to commit it along. The patch looks fine to me but maybe keep it
here for a day or two.
> Updated patch attached. (I also noticed incorrect use of make_cleanup;
> now fixed.)
It would be easy to remove the last "goto" there now when cleanups are in use.
Thanks,
Jan
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-13 0:48 ` Jan Kratochvil
@ 2012-01-13 3:39 ` Paul Pluzhnikov
2012-01-18 19:15 ` Paul Pluzhnikov
0 siblings, 1 reply; 33+ messages in thread
From: Paul Pluzhnikov @ 2012-01-13 3:39 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]
On Thu, Jan 12, 2012 at 4:27 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Fri, 13 Jan 2012 01:20:37 +0100, Paul Pluzhnikov wrote:
>> Could you commit it?
>
> Feel free to commit it along. The patch looks fine to me but maybe keep it
> here for a day or two.
Ok, thanks.
>> Updated patch attached. (I also noticed incorrect use of make_cleanup;
>> now fixed.)
>
> It would be easy to remove the last "goto" there now when cleanups are in use.
Done.
Combined patch attached.
--
Paul Pluzhnikov
2012-01-11 Paul Pluzhnikov <ppluzhnikov@google.com>
Jan Kratochvil <jan.kratochvil@redhat.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.
* configure.ac (AC_CHECK_FUNCS): Add lstat.
* configure: Regenerate.
* config.in: Regenerate.
testsuite/ChangeLog:
PR gdb/9538
* gdb.base/sepdebug.exp: New test.
[-- Attachment #2: gdb-symlink-pr9538-20120112c.txt --]
[-- Type: text/plain, Size: 10197 bytes --]
Index: configure.ac
===================================================================
RCS file: /cvs/src/src/gdb/configure.ac,v
retrieving revision 1.152
diff -u -p -r1.152 configure.ac
--- configure.ac 4 Jan 2012 08:17:00 -0000 1.152
+++ configure.ac 13 Jan 2012 00:39:01 -0000
@@ -1059,7 +1059,7 @@ AC_CHECK_FUNCS([canonicalize_file_name r
getgid pipe poll pread64 resize_term sbrk setpgid setpgrp setsid \
sigaction sigprocmask sigsetmask socketpair syscall \
ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
- setrlimit getrlimit posix_madvise waitpid])
+ setrlimit getrlimit posix_madvise waitpid lstat])
AM_LANGINFO_CODESET
# Check the return and argument types of ptrace. No canned test for
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 13 Jan 2012 00:39:01 -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,90 @@ 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 *dir, *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;
+ }
+
+ dir = xstrdup (objfile->name);
+ cleanups = make_cleanup (xfree, dir);
+ terminate_after_last_dir_separator (dir);
+ canon_dir = lrealpath (dir);
+
+ debugfile = find_separate_debug_file (dir, canon_dir, debuglink,
+ crc32, objfile);
+ xfree (canon_dir);
+
+ if (debugfile == NULL)
+ {
+#ifdef HAVE_LSTAT
+ /* For PR gdb/9538, try again with realpath (if different from the
+ original). */
+
+ struct stat st_buf;
+
+ if (lstat (objfile->name, &st_buf) == 0 && S_ISLNK(st_buf.st_mode))
+ {
+ char *symlink_dir;
+
+ symlink_dir = lrealpath (objfile->name);
+ if (symlink_dir != NULL)
+ {
+ make_cleanup (xfree, symlink_dir);
+ terminate_after_last_dir_separator (symlink_dir);
+ if (strcmp (dir, symlink_dir) != 0)
+ {
+ /* Different directory, so try using it. */
+ debugfile = find_separate_debug_file (symlink_dir,
+ symlink_dir,
+ debuglink,
+ crc32,
+ objfile);
+ }
+ }
+ }
+#endif /* HAVE_LSTAT */
+ }
-cleanup_return_debugfile:
- xfree (canon_name);
- xfree (basename);
- xfree (dir);
+ 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 13 Jan 2012 00:39: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,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."
}
Index: config.in
===================================================================
RCS file: /cvs/src/src/gdb/config.in,v
retrieving revision 1.130
diff -u -p -r1.130 config.in
--- config.in 12 Jan 2012 23:38:47 -0000 1.130
+++ config.in 13 Jan 2012 00:39:00 -0000
@@ -260,6 +260,9 @@
/* Define to 1 if the system has the type `long long int'. */
#undef HAVE_LONG_LONG_INT
+/* Define to 1 if you have the `lstat' function. */
+#undef HAVE_LSTAT
+
/* Define if <sys/procfs.h> has lwpid_t. */
#undef HAVE_LWPID_T
Index: configure
===================================================================
RCS file: /cvs/src/src/gdb/configure,v
retrieving revision 1.337
diff -u -p -r1.337 configure
--- configure 12 Jan 2012 23:38:47 -0000 1.337
+++ configure 13 Jan 2012 00:39:01 -0000
@@ -12918,7 +12918,7 @@ for ac_func in canonicalize_file_name re
getgid pipe poll pread64 resize_term sbrk setpgid setpgrp setsid \
sigaction sigprocmask sigsetmask socketpair syscall \
ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
- setrlimit getrlimit posix_madvise waitpid
+ setrlimit getrlimit posix_madvise waitpid lstat
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]
2012-01-13 0:27 ` Doug Evans
@ 2012-01-13 4:02 ` Joel Brobecker
2012-01-13 11:09 ` Jan Kratochvil
0 siblings, 1 reply; 33+ messages in thread
From: Joel Brobecker @ 2012-01-13 4:02 UTC (permalink / raw)
To: Doug Evans; +Cc: Jan Kratochvil, Paul Pluzhnikov, gdb-patches
> Ok by me!
> [give it awhile to see if anyone objects :-)]
My 2 cents: I think it's too trivial to object... I do not think
there are any solid argument in favor of one or the other. A decision
has been made, let's go with it.
--
Joel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]
2012-01-13 0:21 ` [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).] Jan Kratochvil
2012-01-13 0:27 ` Doug Evans
@ 2012-01-13 9:05 ` Eli Zaretskii
1 sibling, 0 replies; 33+ messages in thread
From: Eli Zaretskii @ 2012-01-13 9:05 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: dje, ppluzhnikov, gdb-patches
> Date: Fri, 13 Jan 2012 00:54:51 +0100
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> Cc: Paul Pluzhnikov <ppluzhnikov@google.com>, gdb-patches@sourceware.org
>
> On Fri, 13 Jan 2012 00:28:35 +0100, Doug Evans wrote:
> > I think we should make the braces required.
>
> OK to check in?
Yes, with one comment:
> +Any two lines in code should be wrapped in braces as they look as separate
> +statements:
> +
> +@smallexample
> +if (i)
> + @{
> + /* Return success. */
> + return 0;
> + @}
> +@end smallexample
If you really want to make a point that this rule is valid even if the
second line is a comment, then I suggest to say so explicitly in the
text.
Otherwise, I'd suggest to replace the comment with a non-comment line.
Thanks.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]
2012-01-13 4:02 ` Joel Brobecker
@ 2012-01-13 11:09 ` Jan Kratochvil
2012-01-13 11:23 ` Pedro Alves
0 siblings, 1 reply; 33+ messages in thread
From: Jan Kratochvil @ 2012-01-13 11:09 UTC (permalink / raw)
To: Joel Brobecker, Eli Zaretskii; +Cc: Doug Evans, Paul Pluzhnikov, gdb-patches
On Fri, 13 Jan 2012 04:46:02 +0100, Joel Brobecker wrote:
> My 2 cents: I think it's too trivial to object... I do not think
> there are any solid argument in favor of one or the other. A decision
> has been made, let's go with it.
I was posting the patch more for its language + understandability part.
On Fri, 13 Jan 2012 09:08:04 +0100, Eli Zaretskii wrote:
> > +@smallexample
> > +if (i)
> > + @{
> > + /* Return success. */
> > + return 0;
> > + @}
> > +@end smallexample
>
> If you really want to make a point that this rule is valid even if the
> second line is a comment, then I suggest to say so explicitly in the
> text.
Two non-comment statements really require a block. It would not work
otherwise. The poi
> Otherwise, I'd suggest to replace the comment with a non-comment line.
The point was the braces should be there even if it is a comment line. This
has no compiler justification, it is just the coding style.
OK this way?
It also addresses:
ok:
if (i)
{
/* Here is no operation needed and even if the comment is a single
statement braces would be better. */
}
not ok:
if (i)
/* Here is no operation needed and even if the comment is a single
statement braces would be better. */;
This mail became a real nitpick/bikeshed.
Thanks,
Jan
gdb/doc/
2012-01-13 Jan Kratochvil <jan.kratochvil@redhat.com>
* gdbint.texinfo (Coding Standards): Require braces for two lines of
code.
--- a/gdb/doc/gdbint.texinfo
+++ b/gdb/doc/gdbint.texinfo
@@ -5849,6 +5849,27 @@ the following guidelines:
@tab (pointer dereference)
@end multitable
+Any two or more lines in code should be wrapped in braces even if one of
+them is a comment line. The readability is improved as such two lines look
+as separate code lines which would require the braces for compiler. Use:
+
+@smallexample
+if (i)
+ @{
+ /* Return success. */
+ return 0;
+ @}
+@end smallexample
+
+@noindent
+and not:
+
+@smallexample
+if (i)
+ /* Return success. */
+ return 0;
+@end smallexample
+
@subsection Comments
@cindex comment formatting
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]
2012-01-13 11:09 ` Jan Kratochvil
@ 2012-01-13 11:23 ` Pedro Alves
2012-01-13 11:55 ` Eli Zaretskii
0 siblings, 1 reply; 33+ messages in thread
From: Pedro Alves @ 2012-01-13 11:23 UTC (permalink / raw)
To: Jan Kratochvil
Cc: Joel Brobecker, Eli Zaretskii, Doug Evans, Paul Pluzhnikov, gdb-patches
On 01/13/2012 11:06 AM, Jan Kratochvil wrote:
> This mail became a real nitpick/bikeshed.
But it's a useful one, because it eliminates all future possible nitpicks,
by casting the rule in stone. I'm glad you're doing this. Thank you.
--
Pedro Alves
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]
2012-01-13 11:23 ` Pedro Alves
@ 2012-01-13 11:55 ` Eli Zaretskii
2012-01-13 12:15 ` Joel Brobecker
0 siblings, 1 reply; 33+ messages in thread
From: Eli Zaretskii @ 2012-01-13 11:55 UTC (permalink / raw)
To: Pedro Alves; +Cc: jan.kratochvil, brobecker, dje, ppluzhnikov, gdb-patches
> Date: Fri, 13 Jan 2012 11:13:46 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: Joel Brobecker <brobecker@adacore.com>, Eli Zaretskii <eliz@gnu.org>,
> Doug Evans <dje@google.com>, Paul Pluzhnikov <ppluzhnikov@google.com>,
> gdb-patches@sourceware.org
>
> On 01/13/2012 11:06 AM, Jan Kratochvil wrote:
> > This mail became a real nitpick/bikeshed.
>
> But it's a useful one, because it eliminates all future possible nitpicks,
> by casting the rule in stone. I'm glad you're doing this. Thank you.
Seconded.
FWIW, all I meant is to leave your original example intact, and modify
the text as follows:
Any two or more lines in code should be wrapped in braces, even if
they are comments, as they look as separate statements:
Thanks.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]
2012-01-13 11:55 ` Eli Zaretskii
@ 2012-01-13 12:15 ` Joel Brobecker
2012-01-13 12:40 ` Eli Zaretskii
0 siblings, 1 reply; 33+ messages in thread
From: Joel Brobecker @ 2012-01-13 12:15 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Pedro Alves, jan.kratochvil, dje, ppluzhnikov, gdb-patches
I like the new text. Just a thought from a non-native English speaker:
> Any two or more lines in code should be wrapped in braces, even if
> they are comments, as they look as separate statements:
^^ like ?
--
Joel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]
2012-01-13 12:15 ` Joel Brobecker
@ 2012-01-13 12:40 ` Eli Zaretskii
2012-01-13 14:37 ` [commit] " Jan Kratochvil
0 siblings, 1 reply; 33+ messages in thread
From: Eli Zaretskii @ 2012-01-13 12:40 UTC (permalink / raw)
To: Joel Brobecker; +Cc: palves, jan.kratochvil, dje, ppluzhnikov, gdb-patches
> Date: Fri, 13 Jan 2012 16:11:17 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Pedro Alves <palves@redhat.com>, jan.kratochvil@redhat.com,
> dje@google.com, ppluzhnikov@google.com, gdb-patches@sourceware.org
>
> I like the new text. Just a thought from a non-native English speaker:
>
> > Any two or more lines in code should be wrapped in braces, even if
> > they are comments, as they look as separate statements:
> ^^ like ?
Fine with me, FWIW.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [commit] [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]
2012-01-13 12:40 ` Eli Zaretskii
@ 2012-01-13 14:37 ` Jan Kratochvil
0 siblings, 0 replies; 33+ messages in thread
From: Jan Kratochvil @ 2012-01-13 14:37 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: Joel Brobecker, palves, dje, ppluzhnikov, gdb-patches
On Fri, 13 Jan 2012 13:39:14 +0100, Eli Zaretskii wrote:
> > > Any two or more lines in code should be wrapped in braces, even if
> > > they are comments, as they look as separate statements:
> > ^^ like ?
>
> Fine with me, FWIW.
Checked in.
Thanks,
Jan
http://sourceware.org/ml/gdb-cvs/2012-01/msg00125.html
--- src/gdb/doc/ChangeLog 2012/01/11 10:34:21 1.1263
+++ src/gdb/doc/ChangeLog 2012/01/13 14:31:55 1.1264
@@ -1,3 +1,9 @@
+2012-01-13 Jan Kratochvil <jan.kratochvil@redhat.com>
+ Eli Zaretskii <eliz@gnu.org>
+
+ * gdbint.texinfo (Coding Standards): Require braces for two lines of
+ code.
+
2012-01-11 Paul Hilfinger <hilfingr@adacore.com>
* gdb.texinfo (Variables): Document use of :: for non-static
--- src/gdb/doc/gdbint.texinfo 2012/01/05 09:41:03 1.334
+++ src/gdb/doc/gdbint.texinfo 2012/01/13 14:31:55 1.335
@@ -5849,6 +5849,26 @@
@tab (pointer dereference)
@end multitable
+Any two or more lines in code should be wrapped in braces, even if
+they are comments, as they look like separate statements:
+
+@smallexample
+if (i)
+ @{
+ /* Return success. */
+ return 0;
+ @}
+@end smallexample
+
+@noindent
+and not:
+
+@smallexample
+if (i)
+ /* Return success. */
+ return 0;
+@end smallexample
+
@subsection Comments
@cindex comment formatting
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).
2012-01-13 3:39 ` Paul Pluzhnikov
@ 2012-01-18 19:15 ` Paul Pluzhnikov
0 siblings, 0 replies; 33+ messages in thread
From: Paul Pluzhnikov @ 2012-01-18 19:15 UTC (permalink / raw)
To: Jan Kratochvil; +Cc: Doug Evans, gdb-patches
On Thu, Jan 12, 2012 at 4:27 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> Feel free to commit it along. The patch looks fine to me but maybe keep it
> here for a day or two.
Committed.
Thanks,
--
Paul Pluzhnikov
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2012-01-18 19:01 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-12 3:12 [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks) Paul Pluzhnikov
2012-01-12 17:28 ` Doug Evans
2012-01-12 20:59 ` Paul Pluzhnikov
2012-01-12 21:31 ` Jan Kratochvil
2012-01-12 22:20 ` Paul Pluzhnikov
2012-01-12 22:27 ` Jan Kratochvil
2012-01-12 22:31 ` Paul Pluzhnikov
2012-01-12 22:29 ` Paul Pluzhnikov
2012-01-12 22:50 ` Jan Kratochvil
2012-01-12 23:12 ` Paul Pluzhnikov
2012-01-12 23:18 ` Jan Kratochvil
2012-01-12 23:25 ` Doug Evans
2012-01-12 23:27 ` Jan Kratochvil
2012-01-12 23:40 ` Doug Evans
2012-01-13 0:21 ` [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).] Jan Kratochvil
2012-01-13 0:27 ` Doug Evans
2012-01-13 4:02 ` Joel Brobecker
2012-01-13 11:09 ` Jan Kratochvil
2012-01-13 11:23 ` Pedro Alves
2012-01-13 11:55 ` Eli Zaretskii
2012-01-13 12:15 ` Joel Brobecker
2012-01-13 12:40 ` Eli Zaretskii
2012-01-13 14:37 ` [commit] " Jan Kratochvil
2012-01-13 9:05 ` Eli Zaretskii
2012-01-12 23:28 ` [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks) Paul Pluzhnikov
2012-01-12 23:55 ` Jan Kratochvil
2012-01-13 0:21 ` Paul Pluzhnikov
2012-01-13 0:48 ` Jan Kratochvil
2012-01-13 3:39 ` Paul Pluzhnikov
2012-01-18 19:15 ` Paul Pluzhnikov
2012-01-12 22:26 ` Doug Evans
2012-01-12 22:40 ` Paul Pluzhnikov
2012-01-12 22:52 ` Doug Evans
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).