From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18480 invoked by alias); 12 Jan 2012 22:20:06 -0000 Received: (qmail 18471 invoked by uid 22791); 12 Jan 2012 22:20:04 -0000 X-SWARE-Spam-Status: No, hits=-2.7 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,RCVD_IN_DNSWL_LOW,TW_BJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-wi0-f169.google.com (HELO mail-wi0-f169.google.com) (209.85.212.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 12 Jan 2012 22:19:50 +0000 Received: by wicr5 with SMTP id r5so1324419wic.0 for ; Thu, 12 Jan 2012 14:19:49 -0800 (PST) Received: by 10.180.90.136 with SMTP id bw8mr9940760wib.1.1326406789429; Thu, 12 Jan 2012 14:19:49 -0800 (PST) Received: by 10.180.90.136 with SMTP id bw8mr9940739wib.1.1326406789299; Thu, 12 Jan 2012 14:19:49 -0800 (PST) MIME-Version: 1.0 Received: by 10.223.144.142 with HTTP; Thu, 12 Jan 2012 14:19:17 -0800 (PST) In-Reply-To: <20120112212959.GA24491@host2.jankratochvil.net> References: <20120112030648.14DBE190AFD@elbrus2.mtv.corp.google.com> <20120112212959.GA24491@host2.jankratochvil.net> From: Paul Pluzhnikov Date: Thu, 12 Jan 2012 22:20:00 -0000 Message-ID: Subject: Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks). To: Jan Kratochvil Cc: Doug Evans , gdb-patches@sourceware.org X-System-Of-Record: true Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-01/txt/msg00436.txt.bz2 On Thu, Jan 12, 2012 at 1:29 PM, Jan Kratochvil wrote: > On Thu, 12 Jan 2012 21:35:21 +0100, Paul Pluzhnikov wrote: >> +/* Modify PATH to contain only "directory/" part of PATH. >> + =A0 If there were no, directory separators in PATH, PATH will be empty >> + =A0 string on return. =A0*/ >> + >> +static void >> +terminate_after_last_dir_separator (char *path) >> +{ >> + =A0int i; >> + >> + =A0/* Strip off the final filename part, leaving the directory name, >> + =A0 =A0 followed by a slash. =A0The directory can be relative or absol= ute. =A0*/ >> + =A0for (i =3D strlen(path) - 1; i >=3D 0; i--) >> + =A0 =A0{ >> + =A0 =A0 =A0if (IS_DIR_SEPARATOR (path[i])) >> + =A0 =A0 break; >> + =A0 =A0} > > Empty line. Done. >> + =A0/* If I is -1 then no directory is present there and DIR will be ""= . =A0*/ >> + =A0path[i+1] =3D '\0'; > > It was there already but there should be `i + 1'. Done. >> +} >> + >> +/* Find separate debuginfo for OBJFILE (using .gnu_debuglink section). >> + =A0 Returns pathname, or NULL. =A0*/ >> + >> +char * >> +find_separate_debug_file_by_debuglink (struct objfile *objfile) >> +{ >> + =A0char *debuglink; >> + =A0char *dir1, *dir2, *canon_dir; >> + =A0char *debugfile; >> + =A0unsigned long crc32; >> + >> + =A0debuglink =3D get_debug_link_info (objfile, &crc32); >> + >> + =A0if (debuglink =3D=3D NULL) >> + =A0 =A0/* There's no separate debug info, hence there's no way we could >> + =A0 =A0 =A0 load it =3D> no warning. =A0*/ >> + =A0 =A0return NULL; >> + >> + =A0dir1 =3D xstrdup (objfile->name); >> + =A0terminate_after_last_dir_separator (dir1); >> + =A0canon_dir =3D lrealpath (dir1); > > lrealpath can return NULL. =A0GDB did not crash before. =A0It will now. Where? canon_dir is passed into the utility function, which checks for NULL (as it did before). >> + >> + =A0debugfile =3D find_separate_debug_file (dir1, canon_dir, debuglink, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 crc32, objfile); >> + =A0xfree (canon_dir); >> + >> + =A0if (debugfile !=3D NULL) >> + =A0 =A0goto cleanup1; >> + >> + =A0/* For PR gdb/9538, try again with realpath (if different from the >> + =A0 =A0 original). =A0*/ >> + =A0dir2 =3D lrealpath (objfile->name); > > Maybe some optimization would be helpful. =A0realpath is expensive and the > directory path is already canonicalized. =A0Something 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. >> + =A0if (dir2 =3D=3D NULL) >> + =A0 =A0goto cleanup1; >> + >> + =A0terminate_after_last_dir_separator (dir2); >> + =A0if (strcmp (dir1, dir2) =3D=3D 0) >> + =A0 =A0/* Same directory, no point retrying. =A0*/ >> + =A0 =A0goto cleanup2; > > There was some discussion with conclusion it should be written as, nitpic= k: > =A0 =A0 =A0if (strcmp (dir1, dir2) =3D=3D 0) > =A0 =A0 =A0 =A0{ > =A0 =A0 =A0 =A0 =A0/* Same directory, no point retrying. =A0*/ > =A0 =A0 =A0 =A0 =A0goto cleanup2; > =A0 =A0 =A0 =A0} Done. > >> + >> + =A0canon_dir =3D lrealpath (dir2); >> + =A0debugfile =3D find_separate_debug_file (dir2, canon_dir, debuglink, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 crc32, objfile); >> + =A0xfree (canon_dir); >> + >> + cleanup2: >> + =A0xfree (dir2); >> + cleanup1: > > Maybe it should be finally rewritten to cleanups but it may be out of the > scope of this patch. Done. > >> + =A0xfree (dir1); >> + =A0xfree (debuglink); >> >> -cleanup_return_debugfile: >> - =A0xfree (canon_name); >> - =A0xfree (basename); >> - =A0xfree (dir); >> =A0 =A0return debugfile; >> =A0} >> >> Index: testsuite/gdb.base/sepdebug.exp >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> 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 =A0 4 Jan 2012 08:17:46 -0000 =A0 = =A0 =A0 1.33 >> +++ testsuite/gdb.base/sepdebug.exp =A0 12 Jan 2012 20:29:53 -0000 >> @@ -45,7 +45,7 @@ if =A0{ [gdb_compile "${srcdir}/${subdir}/ >> >> =A0# Note: the procedure gdb_gnu_strip_debug will produce an executable = called >> =A0# ${binfile}, which is just like the executable ($binfile) but without >> -# the debuginfo. Instead $binfile has a .gnudebuglink section which con= tains >> +# the debuginfo. Instead $binfile has a .gnu_debuglink section which co= ntains >> =A0# the name of a debuginfo only file. This file will be stored in the >> =A0# gdb.base/ subdirectory. >> >> @@ -55,10 +55,26 @@ if [gdb_gnu_strip_debug $binfile] { >> =A0 =A0 =A0return -1 >> =A0} >> >> +# >> +# PR gdb/9538. =A0Verify that symlinked executable still finds the sepa= rate >> +# 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 !=3D "debug" } then { >> + =A0 =A0fail "No debug information found." >> +} >> + >> +# make sure we are not holding subdir/binary open. >> =A0gdb_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} >> =A0if { $gdb_file_cmd_debug_info !=3D "debug" } then { >> =A0 =A0 =A0fail "No debug information found." >> =A0} > > Re-tested on Linux/x86_64. Thanks, --=20 Paul Pluzhnikov 2012-01-11 Paul Pluzhnikov 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.