From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9329 invoked by alias); 27 Aug 2013 14:09:25 -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 9279 invoked by uid 89); 27 Aug 2013 14:09:24 -0000 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 27 Aug 2013 14:09:24 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.6 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7RE9KJv012324 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 27 Aug 2013 10:09:20 -0400 Received: from host2.jankratochvil.net (ovpn-116-30.ams2.redhat.com [10.36.116.30]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r7RE9Fvt004303 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Tue, 27 Aug 2013 10:09:18 -0400 Date: Tue, 27 Aug 2013 14:09:00 -0000 From: Jan Kratochvil To: Doug Evans Cc: gdb-patches@sourceware.org Subject: Re: [patch] [7.6.1] Fix argv[0] symlink regression (PR 15415) Message-ID: <20130827140915.GA17861@host2.jankratochvil.net> References: <20130826182111.GA19509@host2.jankratochvil.net> <21019.47767.404597.352962@ruffy.mtv.corp.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <21019.47767.404597.352962@ruffy.mtv.corp.google.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2013-08/txt/msg00789.txt.bz2 On Mon, 26 Aug 2013 22:29:11 +0200, Doug Evans wrote: > Jan Kratochvil writes: > > --- 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? Sure. One of the reasons was this patch is for the stable branch so any refactorizations should be rather done as a different add-on patch. My second reason was that all the code already was used to the former xfullpath code in 7.5 so why to change something that nobody complains about. But I see now it is arguable, it was xfullpath before (dirs are canonicalized, filename is not), without xfullpath in 7.6 it can never be exactly the same, neither original pathname nor gdb_realpath. The third reason is that I find more important to use 0 as the default most common options while the OPF_DISABLE_REALPATH option is used only occasionally in a special case(s). It is also arguable what is more convenient, whether the default 0 flags or whether the positive form of flags. I do not mind, if you really think the non-DISABLE refactorization should go in I can post a second "Code cleanup" kind patch only for trunk (not 7.6.1) to switch OPF_DISABLE_REALPATH -> OPF_RETURN_REALPATH. > > @@ -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. yes: + /* gdb_bfd_open (and its variants) prefers canonicalized pathname for + better BFD caching. */ Besides that any further changes would be outside of the scope of this patch intended for stable branch and also I do not see a need for any add-on trunk patch on top of it. > 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. I agree. > > --- 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/ ? OK, done. > > --- /dev/null > > +++ b/gdb/testsuite/gdb.base/argv0-symlink.exp > > @@ -0,0 +1,63 @@ [...] > > +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? I have reworked it a bit and the testcase relies now on "ln -sf" exit code instead. > > > +if { [file type [standard_output_file $filelink]] != "link" } { > > + unsupported "$test (host does not support symbolic links)" > > + return 0 > > +} [...] Thanks, Jan gdb/ 2013-08-27 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_EXEC_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_exec_filename. * source.c (openp): Describe OPF_DISABLE_REALPATH. New variable realpath_fptr, initialize it from OPF_DISABLE_REALPATH, use it. gdb/testsuite/ 2013-08-27 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 extern int openp (const char *, int, const char *, int, char **); diff --git a/gdb/exec.c b/gdb/exec.c index 14ff6d7..c61d530 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,16 @@ exec_file_attach (char *filename, int from_tty) cleanups = make_cleanup (xfree, scratch_pathname); + /* gdb_bfd_open (and its variants) prefers canonicalized pathname for + better BFD caching. */ + canonical_pathname = gdb_realpath (scratch_pathname); + make_cleanup (xfree, canonical_pathname); + 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 +226,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..39d5ea5 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_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..d5866e1 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_exec_filename != NULL) + ui_out_field_string (uiout, "exec", inf->pspace->pspace_exec_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_exec_filename != NULL + ? inf->pspace->pspace_exec_filename : _(""))); if (inf->pid != 0) diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index c2d8501..64a8ae3 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_exec_filename != NULL) { ui_out_field_string (uiout, "executable", - bfd_get_filename (inferior->pspace->ebfd)); + inferior->pspace->pspace_exec_filename); } data.cores = 0; diff --git a/gdb/progspace.c b/gdb/progspace.c index 590ea9b..52460ab 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_exec_filename != NULL) + exec_file_attach (src->pspace_exec_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_exec_filename) + ui_out_field_string (uiout, "exec", pspace->pspace_exec_filename); else ui_out_field_skip (uiout, "exec"); diff --git a/gdb/progspace.h b/gdb/progspace.h index 9d98baf..f24a569 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_exec_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..dc11f74 --- /dev/null +++ b/gdb/testsuite/gdb.base/argv0-symlink.exp @@ -0,0 +1,62 @@ +# 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" + +remote_file host delete [standard_output_file $filelink] +set status [remote_exec host "ln -sf ${testfile} [standard_output_file $filelink]"] +if {[lindex $status 0] != 0} { + 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" + +# 'ln -sf' does not overwrite symbol link to a directory. +# 'remote_file host delete' uses stat (not lstat), therefore it refuses to +# delete a directory. +remote_exec host "rm -f [standard_output_file $dirlink]" +set status [remote_exec host "ln -sf . [standard_output_file $dirlink]"] +if {[lindex $status 0] != 0} { + 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