This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] [7.6.1] Fix argv[0] symlink regression (PR 15415)
- From: Doug Evans <dje at google dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 26 Aug 2013 13:29:11 -0700
- Subject: Re: [patch] [7.6.1] Fix argv[0] symlink regression (PR 15415)
- Authentication-results: sourceware.org; auth=none
- References: <20130826182111 dot GA19509 at host2 dot jankratochvil dot net>
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 <jan.kratochvil@redhat.com>
>
> 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 <jan.kratochvil@redhat.com>
>
> 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
> : _("<noexec>")));
>
> 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 <http://www.gnu.org/licenses/>. */
> +
> +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 <http://www.gnu.org/licenses/>.
> +
> +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