From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12948 invoked by alias); 28 Aug 2013 16:50:59 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 12939 invoked by uid 89); 28 Aug 2013 16:50:59 -0000 Received: from mail-qa0-f74.google.com (HELO mail-qa0-f74.google.com) (209.85.216.74) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 28 Aug 2013 16:50:59 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.6 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RP_MATCHES_RCVD,SPF_SOFTFAIL autolearn=ham version=3.3.2 X-HELO: mail-qa0-f74.google.com Received: by mail-qa0-f74.google.com with SMTP id i13so464582qae.3 for ; Wed, 28 Aug 2013 09:50:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-gm-message-state:from:mime-version:content-type :content-transfer-encoding:message-id:date:to:cc:subject:in-reply-to :references; bh=2ISn4J8Pki3aWPq8tNQeixB+D/0TCMctENsEEi7xYFA=; b=aqOkutvP4uEMDY9pkLMiCE7cS0w6q4HlnkDwcrM8cbH0NQ0kcar/EvYOIhqlPHbYES LEU16gHUhBiPs+Pn0tNb85aYaw+TauocDvC6r+ltQ4ebM8aeBradQcQ9TjQyY5G7BQvS GZXG1zzxfiIbXr1N3iTtK2lDy/B1WHAtKLCz+tgDlk1neyHUgXyWpDUskL4TeEAPHXHb 9c4yTYBeRbCpOdGjPIl0cyMY0UucFen9Odl2SutFHuHPtMgiN5Y1PcrYJc5MagZ8HZAv 6hDVarb+IevsWt2cerLQuAlWdQ+b5H35ot1XZ2UQd2oXMBeNxYoSy1fms2D6FPA4nAzf BvEw== X-Gm-Message-State: ALoCoQnLJLmCFuwSoAg/ok/t/LVxLHWrOv8PQsRF92CJGuss0RINHksITsaKauaT++chqzp32+LZDEnkw2dGCvRuKh0tcxi00FWciRGkJ5l+Q1EkEyq/Y+Ofz+N62LAovcZ2TW0/uHxLxzlKlQm9lxeUm+093C4QDkRFcJPD5Xmz8TKyQ1qZITKH5dtTKwgNo8wHj5IqonpBQR5IPpoZA6bLnJXN/pqvFg== X-Received: by 10.236.189.167 with SMTP id c27mr10602202yhn.28.1377708655503; Wed, 28 Aug 2013 09:50:55 -0700 (PDT) Received: from corp2gmr1-1.hot.corp.google.com (corp2gmr1-1.hot.corp.google.com [172.24.189.92]) by gmr-mx.google.com with ESMTPS id a42si1657834yhj.6.1969.12.31.16.00.00 (version=TLSv1.1 cipher=AES128-SHA bits=128/128); Wed, 28 Aug 2013 09:50:55 -0700 (PDT) Received: from ruffy.mtv.corp.google.com (ruffy.mtv.corp.google.com [172.17.128.44]) by corp2gmr1-1.hot.corp.google.com (Postfix) with ESMTP id F1ACA31C264; Wed, 28 Aug 2013 09:50:54 -0700 (PDT) From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <21022.10862.436924.879667@ruffy.mtv.corp.google.com> Date: Wed, 28 Aug 2013 16:50:00 -0000 To: Jan Kratochvil Cc: gdb-patches@sourceware.org Subject: Re: [patch] [7.6.1] Fix argv[0] symlink regression (PR 15415) In-Reply-To: <20130827140915.GA17861@host2.jankratochvil.net> References: <20130826182111.GA19509@host2.jankratochvil.net> <21019.47767.404597.352962@ruffy.mtv.corp.google.com> <20130827140915.GA17861@host2.jankratochvil.net> X-SW-Source: 2013-08/txt/msg00842.txt.bz2 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 > > 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 > > PR gdb/15415 > * gdb.base/argv0-symlink.c: New file. > * gdb.base/argv0-symlink.exp: New file. LGTM