From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12247 invoked by alias); 12 Jan 2012 21:30:23 -0000 Received: (qmail 12105 invoked by uid 22791); 12 Jan 2012 21:30:21 -0000 X-SWARE-Spam-Status: No, hits=-6.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_BJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 12 Jan 2012 21:30:06 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q0CLU55w011428 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 12 Jan 2012 16:30:05 -0500 Received: from host2.jankratochvil.net (ovpn-116-21.ams2.redhat.com [10.36.116.21]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q0CLU0el009775 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 12 Jan 2012 16:30:02 -0500 Date: Thu, 12 Jan 2012 21:31:00 -0000 From: Jan Kratochvil To: Paul Pluzhnikov Cc: Doug Evans , gdb-patches@sourceware.org Subject: Re: [patch] Fix for PR gdb/9538 (loading of separate debuginfo and symlinks). Message-ID: <20120112212959.GA24491@host2.jankratochvil.net> References: <20120112030648.14DBE190AFD@elbrus2.mtv.corp.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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/msg00433.txt.bz2 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