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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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