From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19064 invoked by alias); 6 Sep 2013 18:29:13 -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 19055 invoked by uid 89); 6 Sep 2013 18:29:13 -0000 Received: from mail-ie0-f179.google.com (HELO mail-ie0-f179.google.com) (209.85.223.179) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 06 Sep 2013 18:29:13 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.7 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,NO_RELAYS autolearn=ham version=3.3.2 X-HELO: mail-ie0-f179.google.com Received: by mail-ie0-f179.google.com with SMTP id m16so6365689ieq.38 for ; Fri, 06 Sep 2013 11:29:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=V4mOPEoZZEdVZ/azgFGv8HsX8XattDrn62GYyqaBA0M=; b=EtoaxXJLHnSrh+u3SL7x2c6gQQXivu7+oaH9hZg3GGt0pZLa5lKHiHmFmBUvy7NMI1 M2ZX6pgSDmBX1N8VDgCURGq+E4fhvlAhdelkX3OwC+ch4NdCMtKr5KeWfpXgavqFLoGx 4ibALJ7ptyWgUwYR2ZAbAppzCbjzDBsV8GB9YNjY4n9DUEClt6D/lHCqNHhDfZrIzzQU pwVdBpMsWRJ/NdKGaas0aUD53ugx5WlKqU6PHSnJOzMymyDYJ0qt0lCUJ3Bc3nOYyL+x MUzMBW7iVkFz5yn1gO3vvlboE0O/zMWdut+504lG9RTLzy6q2FIG97N3HQZJ0V1q6vQb KVRg== X-Gm-Message-State: ALoCoQlRx7yNV6sDlzuwxVSvyH4SBRHim0dUO321ljgosaU5Fd632cdYWhKySY+Sl8fDnky4jxHFrFLLcI+Oy94qzQuCe6zD7fKxuPHmzdSuaVY35ifE4d2FOsFRgBLmPb8WIWQEzsxJlAmiTBDEcO6o+UEip+318QNFh0GRqr6adtwTPw5JFK/XW1+PrOlpVRvZ5DMlXiMHnmlsBY3rGnxS32MPYxptvg== MIME-Version: 1.0 X-Received: by 10.50.55.65 with SMTP id q1mr11019897igp.4.1378492150039; Fri, 06 Sep 2013 11:29:10 -0700 (PDT) Received: by 10.64.64.161 with HTTP; Fri, 6 Sep 2013 11:29:09 -0700 (PDT) In-Reply-To: <20130905131839.GA8618@host2.jankratochvil.net> References: <20130905131839.GA8618@host2.jankratochvil.net> Date: Fri, 06 Sep 2013 18:29:00 -0000 Message-ID: Subject: Re: [patchv2] Support .dwp with the name of symlinked binary file From: Doug Evans To: Jan Kratochvil Cc: gdb-patches Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes X-SW-Source: 2013-09/txt/msg00249.txt.bz2 On Thu, Sep 5, 2013 at 6:18 AM, Jan Kratochvil wrote: > Hi Doug, > > this patch obsoletes the series: > [patch 0/3] Support .dwp with the name of symlinked binary file > https://sourceware.org/ml/gdb-patches/2013-08/msg00837.html > Message-ID: <20130828160529.GA23977@host2.jankratochvil.net> > > 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. We *could* move towards not showing the user the file name from the bfd at all. [not suggesting we will, but we could] > As long as bfds are shared (based on their canonical name) it does not make > sense to put to bfd->filename anything besides the canonical name. > > So this patch solves the problem much easier. The problem is that is solves > it only for executables and not for shared libraries. Does it matter? Alas it does matter for shared libs, the same issues apply: lib/libshared.so ->(symlink) /foo/abc/def lib/libshared.so.dwp ->(symlink) /foo/ghi/jkl > Also currently GDB code seems to use bfd->filename and its objfile->name > interchangeably so I did not want to change either. One needn't address things all at once, but not wanting to change something isn't, in and of itself(!), a good reason. [I think we can agree that there are at least a few things in gdb that *need* changing. :-) We may not agree on what they are, but we should still evaluate each situation according to its merits.] Is there a real issue here? I'm not sure, but it seems so. >From the point of view of the user, realpath bugs me. [I'm not including stripping "./" in this, stripping that is reasonable enough. And foo/bar/../baz isn't worth the trouble, though I would be ok with an option where the user can turn canonicalization on.] ISTM realpath is mostly just an implementation detail in gdb. Or to turn it around, when *does* the user want realpath? If it is indeed rare that the user wants (or cares) about realpath, can we move towards not showing it to them? It may be that there are places where we don't have anything else besides bfd->filename, but we already wrap bfd in gdb_bfd (I'm all for abstracting away bfd as much as possible). We *could* save the file passed to gdb_bfd_open, and have gdb_bfd_filename() or some such. Also, it may be that not all objfiles have a bfd (I don't remember off hand, but whether it's true or not is irrelevant to my point), and if not all objfiles have a bfd we can't just remove objfile->name (assuming objfiles-without-bfds have a name). OTOH, if objfiles-without-bfds have a name, we could store it in a different place (objfile->foo_name), and thus still effectively have an implementation that only maintains one name, not two (setting aside bfd->filename which is more an implementation detail of the cache). [I don't want to add more file names, having two copies of something is already causing trouble. But I think this is a solvable problem.] If we choose to keep using objfile->name and bfd->filename without much thought put into a design we're just leaving the door open for more trouble. And if we record the original file name we can always get the realpath name, but if we only record the realpath name we can't get the original name back. [but again I'm not saying one needs to address all issues at once ... as long as we're "typing in the same direction" we'll eventually get there] Thoughts? > > > Jan > > > gdb/ > 2013-09-05 Jan Kratochvil > > * dwarf2read.c (open_and_init_dwp_file): Initialize dwp_name, dbfd and > cleanups. Try to find .dwp also before any symbolic links resolving. > > gdb/testsuite/ > 2013-09-05 Jan Kratochvil > > * gdb.base/dwp-symlink.c: New file. > * gdb.base/dwp-symlink.exp: New file. > > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index 4b30e6e..fab59ea 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -9643,14 +9643,25 @@ open_and_init_dwp_file (void) > { > struct objfile *objfile = dwarf2_per_objfile->objfile; > struct dwp_file *dwp_file; > - char *dwp_name; > - bfd *dbfd; > - struct cleanup *cleanups; > + char *dwp_name = NULL; > + bfd *dbfd = NULL; > + struct cleanup *cleanups = make_cleanup (null_cleanup, NULL); > > - dwp_name = xstrprintf ("%s.dwp", dwarf2_per_objfile->objfile->name); > - cleanups = make_cleanup (xfree, dwp_name); > - > - dbfd = open_dwp_file (dwp_name); > + if (objfile->obfd == exec_bfd && get_exec_file (0) != NULL) > + { > + /* Try to find .dwp for the binary file before any symbolic links > + resolving. */ > + dwp_name = xstrprintf ("%s.dwp", get_exec_file (1)); > + make_cleanup (xfree, dwp_name); > + dbfd = open_dwp_file (dwp_name); > + } > + if (dbfd == NULL) > + { > + /* Try to find .dwp for the binary file after gdb_realpath resolving. */ > + dwp_name = xstrprintf ("%s.dwp", objfile->name); > + make_cleanup (xfree, dwp_name); > + dbfd = open_dwp_file (dwp_name); > + } > if (dbfd == NULL) > { > if (dwarf2_read_debug) > diff --git a/gdb/testsuite/gdb.base/dwp-symlink.c b/gdb/testsuite/gdb.base/dwp-symlink.c > new file mode 100644 > index 0000000..5be12fb > --- /dev/null > +++ b/gdb/testsuite/gdb.base/dwp-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 . */ > + > +int > +main (int argc, char **argv) > +{ > + return 0; > +} > diff --git a/gdb/testsuite/gdb.base/dwp-symlink.exp b/gdb/testsuite/gdb.base/dwp-symlink.exp > new file mode 100644 > index 0000000..ad0522b > --- /dev/null > +++ b/gdb/testsuite/gdb.base/dwp-symlink.exp > @@ -0,0 +1,77 @@ > +# 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 . > + > +standard_testfile > + > +if [is_remote host] { > + untested "remote host" > + return 0 > +} > + > +file delete [standard_output_file ${testfile}.dwp] > +if [file exists [standard_output_file ${testfile}.dwp]] { > + unsupported "dwp file cannot be deleted" > + return 0 > +} > +if { [build_executable ${testfile}.exp ${testfile} ${srcfile}] == -1 } { > + return -1 > +} > +if ![file exists [standard_output_file ${testfile}.dwp]] { > + unsupported "testsuite run does not produce dwp files" > + return 0 > +} > + > +set thelink "${testfile}-thelink" > + > +file delete [standard_output_file ${thelink}] > +file delete [standard_output_file ${thelink}.dwp] > +# file link is only Tcl 8.4+. > +exec "ln" "-sf" "${testfile}" "[standard_output_file $thelink]" > +if ![file exists [standard_output_file $thelink]] { > + unsupported "host does not support symbolic links (binary symlink is missing)" > + return 0 > +} > +if [file exists [standard_output_file $thelink.dwp]] { > + unsupported "host does not support symbolic links (we tried to delete a file and it is still there)" > + return 0 > +} > + > +clean_restart "$testfile" > + > +gdb_test "ptype main" {type = int \(int, char \*\*\)} "binary default, dwp default" > + > +clean_restart "$thelink" > + > +gdb_test "ptype main" {type = int \(int, char \*\*\)} "binary symlink, dwp default" > + > +gdb_exit > +file rename [standard_output_file ${testfile}.dwp] [standard_output_file ${thelink}.dwp] > +if [file exists [standard_output_file ${testfile}.dwp]] { > + unsupported "host does not support symbolic links (binary symlink exists)" > + return 0 > +} > +if ![file exists [standard_output_file ${thelink}.dwp]] { > + unsupported "host does not support symbolic links (dwp symlink is missing)" > + return 0 > +} > + > +clean_restart "$testfile" > + > +# This case cannot work. > +gdb_test "ptype main" {type = int \(\)} "binary default, dwp at symlink" > + > +clean_restart "$thelink" > + > +gdb_test "ptype main" {type = int \(int, char \*\*\)} "binary symlink, dwp at symlink"