From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10068 invoked by alias); 26 Aug 2013 20:29:17 -0000 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 Received: (qmail 10055 invoked by uid 89); 26 Aug 2013 20:29:17 -0000 Received: from mail-vc0-f202.google.com (HELO mail-vc0-f202.google.com) (209.85.220.202) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 26 Aug 2013 20:29:17 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.3 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RP_MATCHES_RCVD,SPF_SOFTFAIL autolearn=ham version=3.3.2 X-HELO: mail-vc0-f202.google.com Received: by mail-vc0-f202.google.com with SMTP id gd11so393949vcb.1 for ; Mon, 26 Aug 2013 13:29:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-gm-message-state:from:mime-version:content-type :content-transfer-encoding:message-id:date:to:cc:subject:in-reply-to :references; bh=WQ2/XM76lXyPnuA03wv9n4jI7qDNtaJRJ9HLwqRrw+g=; b=f67QDa2Cy/lU/q+Gb+LBAOmuja2iYxaFyXoH07ofOOz17cTrM2NW8n2FPabZKx2D+v 92BTIkSlm0GTw0a5efvXxUFARAZQDLt2dWBVeDKxt2pu/RNUrIu+zV+jS+NNITTpBxm5 bUicEUb0A3v/GdRSZjpAwWIvyRW7F57UyT0pylYcr3vBLHsV2ZlzzqFs2KJfOYR+ZjJV m4onwuAnB/L9XbGE3J+Pdpw9NerhEmAVNnSwZ6UuVG0iUjMxbJP+diL9cT7Phcgkn58D oL2hlnq+5jiYLChzEhe/z39aE4Ey32TGxc1i81T0k+R6xtAepD8Q+aiUWtMyn0R78MwX YglQ== X-Gm-Message-State: ALoCoQl3DmsMcpvA5nXjjv0YGcfxpqVhKFKon7+TpQruyzbQEbfXeuGsVqrXYxo45naTqFKhRTEvTBsEVVrz/gE86hd3FcmOOc7wlwJIGneCzoDqLPTyPNOjh6icl6vsgbD8U61+kq5q+Aio7NcjD+tTPeOh4oq4gFPwX9Gf0mXHt8ollit2Vfgg/mALqOskw1vwZowmpJQheQaCx4mQqYFwBApiEnqVBQ== X-Received: by 10.236.147.70 with SMTP id s46mr6130160yhj.0.1377548952524; Mon, 26 Aug 2013 13:29:12 -0700 (PDT) Received: from corp2gmr1-2.hot.corp.google.com (corp2gmr1-2.hot.corp.google.com [172.24.189.93]) by gmr-mx.google.com with ESMTPS id h70si1045483yhk.2.1969.12.31.16.00.00 (version=TLSv1.1 cipher=AES128-SHA bits=128/128); Mon, 26 Aug 2013 13:29:12 -0700 (PDT) Received: from ruffy.mtv.corp.google.com (ruffy.mtv.corp.google.com [172.17.128.44]) by corp2gmr1-2.hot.corp.google.com (Postfix) with ESMTP id ED75C5A41FC; Mon, 26 Aug 2013 13:29:11 -0700 (PDT) From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <21019.47767.404597.352962@ruffy.mtv.corp.google.com> Date: Mon, 26 Aug 2013 20:29:00 -0000 To: Jan Kratochvil Cc: gdb-patches@sourceware.org Subject: Re: [patch] [7.6.1] Fix argv[0] symlink regression (PR 15415) In-Reply-To: <20130826182111.GA19509@host2.jankratochvil.net> References: <20130826182111.GA19509@host2.jankratochvil.net> X-SW-Source: 2013-08/txt/msg00766.txt.bz2 Jan Kratochvil writes: > Hi, > > gdb resolves symbolic links when passing argv[0] > http://sourceware.org/bugzilla/show_bug.cgi?id=15415 > > 7.6 started to pass gdb_realpath of argv[0] to spawned inferiors. > 7.5 was passing xfullpath, therefore preserving the basename. > > 7.5 > +PASS: gdb.base/argv0-symlink.exp: kept file symbolic link name > +FAIL: gdb.base/argv0-symlink.exp: kept directory symbolic link name > > 7.6 > +FAIL: gdb.base/argv0-symlink.exp: kept file symbolic link name > +FAIL: gdb.base/argv0-symlink.exp: kept directory symbolic link name > > proposed for 7.6.1: > +PASS: gdb.base/argv0-symlink.exp: kept file symbolic link name > +PASS: gdb.base/argv0-symlink.exp: kept directory symbolic link name > > IMO even 7.5 was wrong, it did not keep symbol links in the path before > basename. Yet another issue is that for example '$ gdb -ex r --args true' > will execute ./true if it exists while '$ true' does not - but fixing this > part is definitely out of the scope of 7.6.1. > > There remains an issue considered as a regression by Doug > http://sourceware.org/bugzilla/show_bug.cgi?id=15415#c5 > that gdb 7.6 now also switched xfullpath to gdb_realpath for separate debug > info files (like .dwp). Going to check what to do in a different patch. > > No regressions on {x86_64,x86_64-m32,i686}-fedora21pre-linux-gnu. > > > Thanks, > Jan Hi. A couple of comments inline. > gdb/ > 2013-08-26 Jan Kratochvil > > PR gdb/15415 > * corefile.c (get_exec_file): Use exec_filename. > * defs.h (OPF_DISABLE_REALPATH): New definition. Add new comment. > * exec.c (exec_close): Free EXEC_FILENAME. > (exec_file_attach): New variable canonical_pathname. Use > OPF_DISABLE_REALPATH. Call gdb_realpath explicitly. Set > EXEC_FILENAME. > * exec.h (exec_filename): New. > * inferior.c (print_inferior, inferior_command): Use PSPACE_FILENAME. > * mi/mi-main.c (print_one_inferior): Likewise. > * progspace.c (clone_program_space, print_program_space): Likewise. > * progspace.h (struct program_space): New field pspace_filename. > * source.c (openp): Describe OPF_DISABLE_REALPATH. New variable > realpath_fptr, initialize it from OPF_DISABLE_REALPATH, use it. > > gdb/testsuite/ > 2013-08-26 Jan Kratochvil > > PR gdb/15415 > * gdb.base/argv0-symlink.c: New file. > * gdb.base/argv0-symlink.exp: New file. > > diff --git a/gdb/corefile.c b/gdb/corefile.c > index cb7f14e..1b733e2 100644 > --- a/gdb/corefile.c > +++ b/gdb/corefile.c > @@ -182,8 +182,8 @@ validate_files (void) > char * > get_exec_file (int err) > { > - if (exec_bfd) > - return bfd_get_filename (exec_bfd); > + if (exec_filename) > + return exec_filename; > if (!err) > return NULL; > > diff --git a/gdb/defs.h b/gdb/defs.h > index 014d7d4..74b607d 100644 > --- a/gdb/defs.h > +++ b/gdb/defs.h > @@ -346,8 +346,10 @@ extern const char *pc_prefix (CORE_ADDR); > > /* From source.c */ > > +/* See openp function definition for their description. */ > #define OPF_TRY_CWD_FIRST 0x01 > #define OPF_SEARCH_IN_PATH 0x02 > +#define OPF_DISABLE_REALPATH 0x04 I realize it would be more work, but flags that one turns on in order to turn something off are less preferable. Plus I wonder if there might be more cases where we want to turn realpath off (or at least do it separately/differently), thus eventually mitigating some of the complexity of having OPF_REALPATH instead of OPF_DISABLE_REALPATH. Have you thought about this? > extern int openp (const char *, int, const char *, int, char **); > > diff --git a/gdb/exec.c b/gdb/exec.c > index 14ff6d7..4ef75e3 100644 > --- a/gdb/exec.c > +++ b/gdb/exec.c > @@ -102,6 +102,9 @@ exec_close (void) > exec_bfd_mtime = 0; > > remove_target_sections (&exec_bfd); > + > + xfree (exec_filename); > + exec_filename = NULL; > } > } > > @@ -179,12 +182,13 @@ exec_file_attach (char *filename, int from_tty) > else > { > struct cleanup *cleanups; > - char *scratch_pathname; > + char *scratch_pathname, *canonical_pathname; > int scratch_chan; > struct target_section *sections = NULL, *sections_end = NULL; > char **matching; > > - scratch_chan = openp (getenv ("PATH"), OPF_TRY_CWD_FIRST, filename, > + scratch_chan = openp (getenv ("PATH"), > + OPF_TRY_CWD_FIRST | OPF_DISABLE_REALPATH, filename, > write_files ? O_RDWR | O_BINARY : O_RDONLY | O_BINARY, > &scratch_pathname); > #if defined(__GO32__) || defined(_WIN32) || defined(__CYGWIN__) > @@ -193,7 +197,9 @@ exec_file_attach (char *filename, int from_tty) > char *exename = alloca (strlen (filename) + 5); > > strcat (strcpy (exename, filename), ".exe"); > - scratch_chan = openp (getenv ("PATH"), OPF_TRY_CWD_FIRST, exename, > + scratch_chan = openp (getenv ("PATH"), > + OPF_TRY_CWD_FIRST | OPF_DISABLE_REALPATH, > + exename, > write_files ? O_RDWR | O_BINARY : O_RDONLY | O_BINARY, > &scratch_pathname); > } > @@ -203,11 +209,14 @@ exec_file_attach (char *filename, int from_tty) > > cleanups = make_cleanup (xfree, scratch_pathname); > > + canonical_pathname = gdb_realpath (scratch_pathname); > + make_cleanup (xfree, canonical_pathname); Why call gdb_realpath here? I'm not saying it's wrong, but it's not clear why. A comment would be welcome. Guessing (and I could be wrong!), I'd say it's because gdb_bfd_open(et.al.) impose this requirement on the caller. Not that we have to fix this today, but I think this is an implementation detail of the bfd cache that should not be imposed on callers. I wonder how costly removing this restriction would be. > + > if (write_files) > - exec_bfd = gdb_bfd_fopen (scratch_pathname, gnutarget, > + exec_bfd = gdb_bfd_fopen (canonical_pathname, gnutarget, > FOPEN_RUB, scratch_chan); > else > - exec_bfd = gdb_bfd_open (scratch_pathname, gnutarget, scratch_chan); > + exec_bfd = gdb_bfd_open (canonical_pathname, gnutarget, scratch_chan); > > if (!exec_bfd) > { > @@ -215,6 +224,9 @@ exec_file_attach (char *filename, int from_tty) > scratch_pathname, bfd_errmsg (bfd_get_error ())); > } > > + gdb_assert (exec_filename == NULL); > + exec_filename = xstrdup (scratch_pathname); > + > if (!bfd_check_format_matches (exec_bfd, bfd_object, &matching)) > { > /* Make sure to close exec_bfd, or else "run" might try to use > diff --git a/gdb/exec.h b/gdb/exec.h > index 21ccba8..87aa2b4 100644 > --- a/gdb/exec.h > +++ b/gdb/exec.h > @@ -32,6 +32,7 @@ extern struct target_ops exec_ops; > > #define exec_bfd current_program_space->ebfd > #define exec_bfd_mtime current_program_space->ebfd_mtime > +#define exec_filename current_program_space->pspace_filename >From progspace.h: "A program space represents a symbolic view of an address space." Thus when I see pspace_filename I'm thinking of "symbol-file" and not "exec-file". s/pspace_filename/pspace_exec_filename/ ? > /* Builds a section table, given args BFD, SECTABLE_PTR, SECEND_PTR. > Returns 0 if OK, 1 on error. */ > diff --git a/gdb/inferior.c b/gdb/inferior.c > index b9e9a8d..d346973 100644 > --- a/gdb/inferior.c > +++ b/gdb/inferior.c > @@ -588,9 +588,8 @@ print_inferior (struct ui_out *uiout, char *requested_inferiors) > ui_out_field_string (uiout, "target-id", > inferior_pid_to_str (inf->pid)); > > - if (inf->pspace->ebfd) > - ui_out_field_string (uiout, "exec", > - bfd_get_filename (inf->pspace->ebfd)); > + if (inf->pspace->pspace_filename != NULL) > + ui_out_field_string (uiout, "exec", inf->pspace->pspace_filename); > else > ui_out_field_skip (uiout, "exec"); > > @@ -704,8 +703,8 @@ inferior_command (char *args, int from_tty) > printf_filtered (_("[Switching to inferior %d [%s] (%s)]\n"), > inf->num, > inferior_pid_to_str (inf->pid), > - (inf->pspace->ebfd > - ? bfd_get_filename (inf->pspace->ebfd) > + (inf->pspace->pspace_filename != NULL > + ? inf->pspace->pspace_filename > : _(""))); > > if (inf->pid != 0) > diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c > index c2d8501..33a3082 100644 > --- a/gdb/mi/mi-main.c > +++ b/gdb/mi/mi-main.c > @@ -573,10 +573,10 @@ print_one_inferior (struct inferior *inferior, void *xdata) > if (inferior->pid != 0) > ui_out_field_int (uiout, "pid", inferior->pid); > > - if (inferior->pspace->ebfd) > + if (inferior->pspace->pspace_filename != NULL) > { > ui_out_field_string (uiout, "executable", > - bfd_get_filename (inferior->pspace->ebfd)); > + inferior->pspace->pspace_filename); > } > > data.cores = 0; > diff --git a/gdb/progspace.c b/gdb/progspace.c > index 590ea9b..b345568 100644 > --- a/gdb/progspace.c > +++ b/gdb/progspace.c > @@ -196,8 +196,8 @@ clone_program_space (struct program_space *dest, struct program_space *src) > > set_current_program_space (dest); > > - if (src->ebfd != NULL) > - exec_file_attach (bfd_get_filename (src->ebfd), 0); > + if (src->pspace_filename != NULL) > + exec_file_attach (src->pspace_filename, 0); > > if (src->symfile_object_file != NULL) > symbol_file_add_main (src->symfile_object_file->name, 0); > @@ -336,9 +336,8 @@ print_program_space (struct ui_out *uiout, int requested) > > ui_out_field_int (uiout, "id", pspace->num); > > - if (pspace->ebfd) > - ui_out_field_string (uiout, "exec", > - bfd_get_filename (pspace->ebfd)); > + if (pspace->pspace_filename) > + ui_out_field_string (uiout, "exec", pspace->pspace_filename); > else > ui_out_field_skip (uiout, "exec"); > > diff --git a/gdb/progspace.h b/gdb/progspace.h > index 9d98baf..df71b7a 100644 > --- a/gdb/progspace.h > +++ b/gdb/progspace.h > @@ -148,6 +148,10 @@ struct program_space > bfd *ebfd; > /* The last-modified time, from when the exec was brought in. */ > long ebfd_mtime; > + /* Similar to bfd_get_filename (exec_bfd) but in original form given > + by user, without symbolic links and pathname resolved. > + It needs to be freed by xfree. It is not NULL iff EBFD is not NULL. */ > + char *pspace_filename; > > /* The address space attached to this program space. More than one > program space may be bound to the same address space. In the > diff --git a/gdb/source.c b/gdb/source.c > index e1c498b..de3fb7c 100644 > --- a/gdb/source.c > +++ b/gdb/source.c > @@ -689,6 +689,11 @@ is_regular_file (const char *name) > and the file, sigh! Emacs gets confuzzed by this when we print the > source file name!!! > > + If OPTS does not have OPF_DISABLE_REALPATH set return FILENAME_OPENED > + resolved by gdb_realpath. Even with OPF_DISABLE_REALPATH this function > + still returns filename starting with "/". If FILENAME_OPENED is NULL > + this option has no effect. > + > If a file is found, return the descriptor. > Otherwise, return -1, with errno set for the last name we tried to open. */ > > @@ -848,19 +853,27 @@ done: > /* If a file was opened, canonicalize its filename. */ > if (fd < 0) > *filename_opened = NULL; > - else if (IS_ABSOLUTE_PATH (filename)) > - *filename_opened = gdb_realpath (filename); > else > { > - /* Beware the // my son, the Emacs barfs, the botch that catch... */ > + char *(*realpath_fptr) (const char *); > + > + realpath_fptr = ((opts & OPF_DISABLE_REALPATH) != 0 > + ? xstrdup : gdb_realpath); > + > + if (IS_ABSOLUTE_PATH (filename)) > + *filename_opened = realpath_fptr (filename); > + else > + { > + /* Beware the // my son, the Emacs barfs, the botch that catch... */ > > - char *f = concat (current_directory, > - IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1]) > - ? "" : SLASH_STRING, > - filename, (char *)NULL); > + char *f = concat (current_directory, > + IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1]) > + ? "" : SLASH_STRING, > + filename, (char *)NULL); > > - *filename_opened = gdb_realpath (f); > - xfree (f); > + *filename_opened = realpath_fptr (f); > + xfree (f); > + } > } > } > > diff --git a/gdb/testsuite/gdb.base/argv0-symlink.c b/gdb/testsuite/gdb.base/argv0-symlink.c > new file mode 100644 > index 0000000..5be12fb > --- /dev/null > +++ b/gdb/testsuite/gdb.base/argv0-symlink.c > @@ -0,0 +1,22 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2013 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +int > +main (int argc, char **argv) > +{ > + return 0; > +} > diff --git a/gdb/testsuite/gdb.base/argv0-symlink.exp b/gdb/testsuite/gdb.base/argv0-symlink.exp > new file mode 100644 > index 0000000..fc61efb > --- /dev/null > +++ b/gdb/testsuite/gdb.base/argv0-symlink.exp > @@ -0,0 +1,63 @@ > +# Copyright 2013 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +standard_testfile > + > +if { [build_executable ${testfile}.exp ${testfile} ${srcfile}] == -1 } { > + return -1 > +} > + > +set test "kept file symbolic link name" > +set filelink "${testfile}-filelink" > + > +# 'file link' is tcl 8.4+ only. > +remote_exec host "ln -sf ${testfile} [standard_output_file $filelink]" > + > +# Remote host filesystem is not properly tested here. This comment is really a FIXME, right? Skip this test if remote host? > +if { [file type [standard_output_file $filelink]] != "link" } { > + unsupported "$test (host does not support symbolic links)" > + return 0 > +} > + > +clean_restart "$filelink" > + > +if ![runto_main] { > + untested "could not run to main" > + return -1 > +} > + > +gdb_test {print argv[0]} "/$filelink\"" $test > + > + > +set test "kept directory symbolic link name" > +set dirlink "${testfile}-dirlink" > + > +remote_exec host "rm -f [standard_output_file $dirlink]" > +remote_exec host "ln -sf . [standard_output_file $dirlink]" > + > +# Remote host filesystem is not properly tested here. > +if { [file type [standard_output_file $dirlink]] != "link" } { > + unsupported "$test (host does not support symbolic links)" > + return 0 > +} > + > +clean_restart "$dirlink/$filelink" > + > +if ![runto_main] { > + untested "could not run to main" > + return -1 > +} > + > +gdb_test {print argv[0]} "/$dirlink/$filelink\"" $test