This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] [7.6.1] Fix argv[0] symlink regression (PR 15415)
- From: Doug Evans <dje at google dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 28 Aug 2013 09:50:54 -0700
- Subject: Re: [patch] [7.6.1] Fix argv[0] symlink regression (PR 15415)
- Authentication-results: sourceware.org; auth=none
- References: <20130826182111 dot GA19509 at host2 dot jankratochvil dot net> <21019 dot 47767 dot 404597 dot 352962 at ruffy dot mtv dot corp dot google dot com> <20130827140915 dot GA17861 at host2 dot jankratochvil dot net>
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