* [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 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 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 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: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: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: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
* [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: [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 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: [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: [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: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
* 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: [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
* 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: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: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
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).