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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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  <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_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  <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
 
 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
 		    : _("<noexec>")));
 
   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 <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..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 <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"
+
+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