This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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: gdb-symlink-pr9538-20120112a.txt
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]