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

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

[Jan Kratochvil]

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


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
 
 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);
+
       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
 
 /* 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.
+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

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

[Doug Evans]

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

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

[Jan Kratochvil]

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

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

[Doug Evans]

Jan Kratochvil writes:
 > 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.

I'm all for minimal invasiveness on release branches.
OTOH, we do push back at times for changes going into trunk.

 > 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.

I'm not fond of xfullpath much either.
But it was the law-of-the-land, so to speak, so I was working with it.

 > 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 disagree.
realpath is a heavy weight operation and it's not always intuitive.
Nor is it necessarily what's wanted, even when the caller thinks
that's what s/he wants: symlinks can be used to implement well-defined
layers of abstractions and gdb is never going to always know when
it's correct to cross that boundary.
Plus humans aren't always good at processing double-negatives.
Ergo, if the caller wants it they need to ask for it. [In an ideal world.]

Separately,
I think we need to separate when/why we call realpath() for the user's
benefit and when/why we call it for gdb's benefit.
[And when we call it for gdb's benefit, I think such calls should, in general,
be hidden behind gdb "module" boundaries.]

I can imagine good reasons to traverse at least some symlinks for the
user's benefit.  Whether a sufficient number of such reasons can be
codified into something gdb can reasonably implement ... dunno.
So I'm not saying this is necessarily an easy problem to solve, but
I am trying to make the case that the default for OPF_* should be !realpath.

OTOH, calling gdb_realpath is like the last thing openp does.
I think a reasonable case can be made that it's trying to do too much, and the
caller should call gdb_realpath if s/he wants.  One could provide a wrapper
that calls openp and then realpath to simplify things, which in some sense
is just a case of six-of-one.  I guess it just feels cleaner to build
features out of functions instead 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.

That would be awesome.  Thanks!
[I don't have too strong an opinion on OPF_RETURN_REALPATH vs a new function
openp_with_real_path (or some such) that wraps openp and then calls realpath,
but if it works for you I think(!) I prefer the latter.]

 > >  > @@ -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.

"works for me"

 > > 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.

Cool. :)

 > >  > --- 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.

thx

 > >  > --- /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.

LGTM

[commit+7.6.1] [patch] [7.6.1] Fix argv[0] symlink regression (PR 15415)

[Jan Kratochvil]

On Wed, 28 Aug 2013 18:50:54 +0200, Doug Evans wrote:
> I can imagine good reasons to traverse at least some symlinks for the
> user's benefit.

Which one?  This is discussed in:
	Subject: [patch 2/3] Use gdb_realpath in gdb_bfd_open
	https://sourceware.org/ml/gdb-patches/2013-08/msg00839.html
	Message-ID: <20130828160547.GC23977@host2.jankratochvil.net>


> OTOH, calling gdb_realpath is like the last thing openp does.
> I think a reasonable case can be made that it's trying to do too much, and the
> caller should call gdb_realpath if s/he wants.  One could provide a wrapper
> that calls openp and then realpath to simplify things, which in some sense
> is just a case of six-of-one.  I guess it just feels cleaner to build
> features out of functions instead of flags.

That seems as a good idea, I will update:
	[patch 1/3] Code cleanup: OPF_DISABLE_REALPATH -> OPF_RETURN_REALPATH
	https://sourceware.org/ml/gdb-patches/2013-08/msg00838.html
	Message-ID: <20130828160537.GB23977@host2.jankratochvil.net>


>  > 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.
> 
> LGTM

Therefore checked in:
	https://sourceware.org/ml/gdb-cvs/2013-08/msg00149.html
and for 7.6.1:
	https://sourceware.org/ml/gdb-cvs/2013-08/msg00150.html


Thanks,
Jan

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

[Pedro Alves]

It seems this patch introduces some output inconsistency
(only tried mainline):

 $ ./gdb
 ...
 (gdb) file ./gdb
 Reading symbols from /home/pedro/gdb/mygit/build/gdb/gdb...done.
 Setting up the environment for debugging gdb.
 (top-gdb) info inferiors
   Num  Description       Executable
 * 1    <null>            /home/pedro/gdb/mygit/build/gdb/./gdb
 (top-gdb)

Note "gdb/gdb" vs "gdb/./gdb".

 (top-gdb) file gdbserver/../gdb
 Load new symbol table from "/home/pedro/gdb/mygit/build/gdb/gdb"? (y or n) y
 Reading symbols from /home/pedro/gdb/mygit/build/gdb/gdb...done.
 (top-gdb) info inferiors
   Num  Description       Executable
 * 1    <null>            /home/pedro/gdb/mygit/build/gdb/gdbserver/../gdb
 (top-gdb)

Note ".../gdb/gdb" vs ".../gdb/gdbserver/../gdb".

I tried your new series at
<https://sourceware.org/ml/gdb-patches/2013-08/msg00837.html>, and
seems there's still some inconsistency:

 (gdb) file ./gdb
 Reading symbols from /home/pedro/gdb/mygit/build/gdb/./gdb...done.
 Setting up the environment for debugging gdb.
 (top-gdb) info inferiors
   Num  Description       Executable
 * 1    <null>            /home/pedro/gdb/mygit/build/gdb/./gdb
 (top-gdb) info files
 Symbols from "/home/pedro/gdb/mygit/build/gdb/./gdb".
 Local exec file:
         `/home/pedro/gdb/mygit/build/gdb/./gdb', file type elf64-x86-64.

This one's consistent now, but then this one's odd:

 (top-gdb) file gdbserver/../gdb
 Load new symbol table from "/home/pedro/gdb/mygit/build/gdb/./gdb"? (y or n) y
 Reading symbols from /home/pedro/gdb/mygit/build/gdb/./gdb...done.

 (top-gdb) info inferiors
   Num  Description       Executable
 * 1    <null>            /home/pedro/gdb/mygit/build/gdb/gdbserver/../gdb
 (top-gdb)

Hmm.  It seems to only happen after having loaded "./gdb" first.  This
sounds like related to the filename handling in the gdb/bfd cache?  GDB
finds reuses the same bfd (as gdbserver/../gdb is the same file as
the previous ./gdb), and then we printing the filename that had
been associated with the bfd before, instead of the one that was
specified in the second "file" ?

-- 
Pedro Alves

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

[Doug Evans]

On Wed, Sep 4, 2013 at 3:48 AM, Pedro Alves <palves@redhat.com> wrote:
> It seems this patch introduces some output inconsistency
> (only tried mainline):
>
>  $ ./gdb
>  ...
>  (gdb) file ./gdb
>  Reading symbols from /home/pedro/gdb/mygit/build/gdb/gdb...done.
>  Setting up the environment for debugging gdb.
>  (top-gdb) info inferiors
>    Num  Description       Executable
>  * 1    <null>            /home/pedro/gdb/mygit/build/gdb/./gdb
>  (top-gdb)
>
> Note "gdb/gdb" vs "gdb/./gdb".
>
>  (top-gdb) file gdbserver/../gdb
>  Load new symbol table from "/home/pedro/gdb/mygit/build/gdb/gdb"? (y or n) y
>  Reading symbols from /home/pedro/gdb/mygit/build/gdb/gdb...done.
>  (top-gdb) info inferiors
>    Num  Description       Executable
>  * 1    <null>            /home/pedro/gdb/mygit/build/gdb/gdbserver/../gdb
>  (top-gdb)
>
> Note ".../gdb/gdb" vs ".../gdb/gdbserver/../gdb".
>
> I tried your new series at
> <https://sourceware.org/ml/gdb-patches/2013-08/msg00837.html>, and
> seems there's still some inconsistency:
>
>  (gdb) file ./gdb
>  Reading symbols from /home/pedro/gdb/mygit/build/gdb/./gdb...done.
>  Setting up the environment for debugging gdb.
>  (top-gdb) info inferiors
>    Num  Description       Executable
>  * 1    <null>            /home/pedro/gdb/mygit/build/gdb/./gdb
>  (top-gdb) info files
>  Symbols from "/home/pedro/gdb/mygit/build/gdb/./gdb".
>  Local exec file:
>          `/home/pedro/gdb/mygit/build/gdb/./gdb', file type elf64-x86-64.
>
> This one's consistent now, but then this one's odd:
>
>  (top-gdb) file gdbserver/../gdb
>  Load new symbol table from "/home/pedro/gdb/mygit/build/gdb/./gdb"? (y or n) y
>  Reading symbols from /home/pedro/gdb/mygit/build/gdb/./gdb...done.
>
>  (top-gdb) info inferiors
>    Num  Description       Executable
>  * 1    <null>            /home/pedro/gdb/mygit/build/gdb/gdbserver/../gdb
>  (top-gdb)
>
> Hmm.  It seems to only happen after having loaded "./gdb" first.  This
> sounds like related to the filename handling in the gdb/bfd cache?  GDB
> finds reuses the same bfd (as gdbserver/../gdb is the same file as
> the previous ./gdb), and then we printing the filename that had
> been associated with the bfd before, instead of the one that was
> specified in the second "file" ?

Hi.  Some random thoughts.

If the user wants to debug foo/bar/../baz, I don't mind gdb printing
it as foo/bar/../baz.
Other's might of course.

Removing ./ from paths is easy and reasonable enough.

If gdb is debugging a new file I would have expected the old file to
be gone from bfd's cache.
[The problem may be in the sequencing.]

We do need to be consistent with which flavor of a path we use.

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

[Yufeng Zhang]

On 08/26/13 19:21, Jan Kratochvil wrote:
> 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.

I wonder if the tests shall be skipped in the remote environment where 
gdb has no control over argv[0].

Thanks,
Yufeng

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

[Jan Kratochvil]

On Wed, 04 Sep 2013 18:46:07 +0200, Yufeng Zhang wrote:
> On 08/26/13 19:21, Jan Kratochvil wrote:
> >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.
> 
> I wonder if the tests shall be skipped in the remote environment
> where gdb has no control over argv[0].

The testcase runs gdbserver (even in gdbserver mode) so it has control over
argv[0].  The testcase PASSes for me both with gdbserver and with gdbserver in
extended mode running on localhost (Fedora Rawhide x86_64).

If one runs gdbserver on remote host with different filesystem I believe the
testcase still should work as I test there only the latest filename component
(and latest directory component).

Could you post your FAIL gdb.log or do you have just theoretical objectives?


Thanks,
Jan

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

[Pedro Alves]

On 09/04/2013 05:31 PM, Doug Evans wrote:
> On Wed, Sep 4, 2013 at 3:48 AM, Pedro Alves <palves@redhat.com> wrote:
>> It seems this patch introduces some output inconsistency
>> (only tried mainline):
>>
>>  $ ./gdb
>>  ...
>>  (gdb) file ./gdb
>>  Reading symbols from /home/pedro/gdb/mygit/build/gdb/gdb...done.
>>  Setting up the environment for debugging gdb.
>>  (top-gdb) info inferiors
>>    Num  Description       Executable
>>  * 1    <null>            /home/pedro/gdb/mygit/build/gdb/./gdb
>>  (top-gdb)
>>
>> Note "gdb/gdb" vs "gdb/./gdb".
>>
>>  (top-gdb) file gdbserver/../gdb
>>  Load new symbol table from "/home/pedro/gdb/mygit/build/gdb/gdb"? (y or n) y
>>  Reading symbols from /home/pedro/gdb/mygit/build/gdb/gdb...done.
>>  (top-gdb) info inferiors
>>    Num  Description       Executable
>>  * 1    <null>            /home/pedro/gdb/mygit/build/gdb/gdbserver/../gdb
>>  (top-gdb)
>>
>> Note ".../gdb/gdb" vs ".../gdb/gdbserver/../gdb".
>>
>> I tried your new series at
>> <https://sourceware.org/ml/gdb-patches/2013-08/msg00837.html>, and
>> seems there's still some inconsistency:
>>
>>  (gdb) file ./gdb
>>  Reading symbols from /home/pedro/gdb/mygit/build/gdb/./gdb...done.
>>  Setting up the environment for debugging gdb.
>>  (top-gdb) info inferiors
>>    Num  Description       Executable
>>  * 1    <null>            /home/pedro/gdb/mygit/build/gdb/./gdb
>>  (top-gdb) info files
>>  Symbols from "/home/pedro/gdb/mygit/build/gdb/./gdb".
>>  Local exec file:
>>          `/home/pedro/gdb/mygit/build/gdb/./gdb', file type elf64-x86-64.
>>
>> This one's consistent now, but then this one's odd:
>>
>>  (top-gdb) file gdbserver/../gdb
>>  Load new symbol table from "/home/pedro/gdb/mygit/build/gdb/./gdb"? (y or n) y
>>  Reading symbols from /home/pedro/gdb/mygit/build/gdb/./gdb...done.
>>
>>  (top-gdb) info inferiors
>>    Num  Description       Executable
>>  * 1    <null>            /home/pedro/gdb/mygit/build/gdb/gdbserver/../gdb
>>  (top-gdb)
>>
>> Hmm.  It seems to only happen after having loaded "./gdb" first.  This
>> sounds like related to the filename handling in the gdb/bfd cache?  GDB
>> finds reuses the same bfd (as gdbserver/../gdb is the same file as
>> the previous ./gdb), and then we printing the filename that had
>> been associated with the bfd before, instead of the one that was
>> specified in the second "file" ?
> 
> Hi.  Some random thoughts.
> 
> If the user wants to debug foo/bar/../baz, I don't mind gdb printing
> it as foo/bar/../baz.
> Other's might of course.

I don't mind either.  Opening foo/bar/../baz is not the
same as opening foo/baz, if bar is a symlink.  That's not usually
true when talking about "cd", though.

Maybe what we'll find out we need in the future is a knob users
can turn on to say "also print me the realpath in addition if it
differs from what I specified".

> 
> Removing ./ from paths is easy and reasonable enough.

Yeah, I think we should.

> 
> If gdb is debugging a new file I would have expected the old file to
> be gone from bfd's cache.

Well, or not, a valid caching policy may be to monitor memory/resource
usage, and as long as resources are available, assume the same file
will be reopened soon.  I don't really know the policy here though,
it may just be the original file is only closed after the new one
is opened.

But, the issue should trigger even if not closing the old file.
Let me check...

 (gdb) add-inferior -exec ./gdbserver/../gdb
 Added inferior 2
 Reading symbols from /home/pedro/gdb/mygit/build/gdb/./gdbserver/../gdb...done.
 Setting up the environment for debugging gdb.
 (top-gdb) add-inferior -exec ./gdb
 Added inferior 3
 Reading symbols from /home/pedro/gdb/mygit/build/gdb/./gdbserver/../gdb...done.
 (top-gdb) info inferiors
   Num  Description       Executable
   3    <null>            /home/pedro/gdb/mygit/build/gdb/./gdb
   2    <null>            /home/pedro/gdb/mygit/build/gdb/./gdbserver/../gdb
 * 1    <null>

Note it shows "gdbserver/../gdb" for inferior 3 too.

vs (new run from scratch):

 (gdb) add-inferior -exec ./gdb
 Added inferior 2
 Reading symbols from /home/pedro/gdb/mygit/build/gdb/./gdb...done.
 Setting up the environment for debugging gdb.
 (top-gdb) add-inferior -exec ./gdbserver/../gdb
 Added inferior 3
 Reading symbols from /home/pedro/gdb/mygit/build/gdb/./gdb...done.
 Setting up the environment for debugging gdb.
 (top-gdb) info inferiors
   Num  Description       Executable
   3    <null>            /home/pedro/gdb/mygit/build/gdb/./gdbserver/../gdb
   2    <null>            /home/pedro/gdb/mygit/build/gdb/./gdb
 * 1    <null>

Note it shows "./gdb" for inferior 2 too.

> [The problem may be in the sequencing.]

Yep, whatever is used first is reused.

> We do need to be consistent with which flavor of a path we use.

-- 
Pedro Alves

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

[Yufeng Zhang]

On 09/04/13 17:53, Jan Kratochvil wrote:
> On Wed, 04 Sep 2013 18:46:07 +0200, Yufeng Zhang wrote:
>> On 08/26/13 19:21, Jan Kratochvil wrote:
>>> 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.
>>
>> I wonder if the tests shall be skipped in the remote environment
>> where gdb has no control over argv[0].
>
> The testcase runs gdbserver (even in gdbserver mode) so it has control over
> argv[0].  The testcase PASSes for me both with gdbserver and with gdbserver in
> extended mode running on localhost (Fedora Rawhide x86_64).
>
> If one runs gdbserver on remote host with different filesystem I believe the
> testcase still should work as I test there only the latest filename component
> (and latest directory component).
>
> Could you post your FAIL gdb.log or do you have just theoretical objectives?

I am running the test on a simulator implementing the gdb remote stub. 
  I think it is related with the way the simulator is spawned; it seems 
like only the file name 'argv0-symlink-filelink' is passed to the simulator:

spawn <simulator listening on 9784> argv0-symlink-filelink
target remote localhost:9784
Remote debugging using localhost:9784
0x0000000000400180 in _start ()
(gdb) continue
Continuing.

Breakpoint 1, main (argc=1, argv=0x20414fe0) at 
/work/src/binutils/gdb/testsuite/gdb.base/argv0-symlink.c:21
21        return 0;
(gdb) print argv[0]
$1 = 0x412fa8 "argv0-symlink-filelink"
(gdb) FAIL: gdb.base/argv0-symlink.exp: kept file symbolic link name


I'll check my test config.

Thanks,
Yufeng

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

[Jan Kratochvil]

On Wed, 04 Sep 2013 12:48:41 +0200, Pedro Alves wrote:
> It seems this patch introduces some output inconsistency
> (only tried mainline):
> 
>  $ ./gdb
>  ...
>  (gdb) file ./gdb
>  Reading symbols from /home/pedro/gdb/mygit/build/gdb/gdb...done.
>  Setting up the environment for debugging gdb.
>  (top-gdb) info inferiors
>    Num  Description       Executable
>  * 1    <null>            /home/pedro/gdb/mygit/build/gdb/./gdb
>  (top-gdb)
> 
> Note "gdb/gdb" vs "gdb/./gdb".
> 
>  (top-gdb) file gdbserver/../gdb
>  Load new symbol table from "/home/pedro/gdb/mygit/build/gdb/gdb"? (y or n) y
>  Reading symbols from /home/pedro/gdb/mygit/build/gdb/gdb...done.
>  (top-gdb) info inferiors
>    Num  Description       Executable
>  * 1    <null>            /home/pedro/gdb/mygit/build/gdb/gdbserver/../gdb
>  (top-gdb)
> 
> Note ".../gdb/gdb" vs ".../gdb/gdbserver/../gdb".

That is correct, you have specified different "file" parameter, therefore
those inferiors will be executed with different argv[0].

Just the name is displayed in absolute form (with CWD prepended) - argv[0]
will be supplied exactly as given by the user (relatively in your case).
The other possibility would be to really display argv[0]:
   * 1    <null>            ./gdb
+
   * 1    <null>            gdbserver/../gdb
But I think the current output above is preferred.


> I tried your new series at
> <https://sourceware.org/ml/gdb-patches/2013-08/msg00837.html>, and
> seems there's still some inconsistency:

I have found the series does not make much sense, going to rework it first.

When bfd->filename no longer contains the canonical form then we could use it
as its original filename, couldn't we?  No, we can't because it can be shared
from cache so it may be original filename of an unrelated file.

As long as bfd's are shared (based on their canonical name) it does not make
sense to put to bfd->filename anything besides the canonical name.

But I think objfile->name can store the original (non-canonical) filename.

exec_bfd does not have its objfile - it has symfile_objfile which does not
have to be equal and argv[0] should IMO correspond rather to exec_bfd than to
correspond to symfile_objfile.  But we have already exec_filename for exec_bfd
so that is no longer a problem.

Going to code new series how it will work, thanks for the feedback.


In reality xfullpath() (canonical directories, filename intact) was a perfect
match as it resolved all relative references to directories while it kept
argv[0] in original form (as inferiors generally care only about filename in
argv[0] and not about directories in argv[0]; except for GDB relocatability
although there it does not matter if the path is canonicalized or not).
Just so far I still do not think it is worth it reintroducing xfullpath.


Jan