This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] [7.6.1] Fix argv[0] symlink regression (PR 15415)


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]