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

[Doug Evans <dje@google.com>]

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