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

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

[Paul Pluzhnikov]

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

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

[Doug Evans]

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

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

[Paul Pluzhnikov]

[-- 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."
 }

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

[Jan Kratochvil]

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

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

[Paul Pluzhnikov]

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.

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

[Doug Evans]

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?

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

[Jan Kratochvil]

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

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

[Paul Pluzhnikov]

[-- 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."
 }

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

[Paul Pluzhnikov]

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

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

[Paul Pluzhnikov]

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

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

[Jan Kratochvil]

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

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

[Doug Evans]

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

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

[Paul Pluzhnikov]

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

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

[Jan Kratochvil]

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

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

[Doug Evans]

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;

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

[Jan Kratochvil]

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

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

[Paul Pluzhnikov]

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

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

[Doug Evans]

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

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

[Jan Kratochvil]

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
 

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

[Paul Pluzhnikov]

[-- 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."
 }

[doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]

[Jan Kratochvil]

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

Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]

[Doug Evans]

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

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

[Jan Kratochvil]

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

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

[Paul Pluzhnikov]

[-- 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"

Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]

[Joel Brobecker]

> 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

Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]

[Eli Zaretskii]

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

Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]

[Jan Kratochvil]

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

Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]

[Pedro Alves]

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

Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]

[Eli Zaretskii]

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

Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]

[Joel Brobecker]

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

Re: [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]

[Eli Zaretskii]

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

[commit] [doc patch] gdbint: braces for two lines in code [Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks).]

[Jan Kratochvil]

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

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

[Paul Pluzhnikov]

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