Re: [patchv2] Support .dwp with the name of symlinked binary file

[Doug Evans <dje at google dot com>]

On Thu, Sep 5, 2013 at 6:18 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> 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  <jan.kratochvil@redhat.com>
>
>         * 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  <jan.kratochvil@redhat.com>
>
>         * 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 <http://www.gnu.org/licenses/>.  */
> +
> +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 <http://www.gnu.org/licenses/>.
> +
> +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"