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

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

[Jan Kratochvil]

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.

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?

Also currently GDB code seems to use bfd->filename and its objfile->name
interchangeably so I did not want to change either.


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"

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

[Doug Evans]

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"

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

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> Also, it may be that not all objfiles have a bfd (I don't remember off
Doug> hand, but whether it's true or not is irrelevant to my point), and if
Doug> not all objfiles have a bfd we can't just remove objfile->name
Doug> (assuming objfiles-without-bfds have a name).

There are objfiles without a BFD.  E.g., Java makes one.  Maybe the JIT
stuff too.  These do still have a name -- objfiles are required to have
a name.

Doug> OTOH, if objfiles-without-bfds have a name, we could store it in a
Doug> different place (objfile->foo_name), and thus still effectively have
Doug> an implementation that only maintains one name, not two

I don't understand this.  It doesn't seem any different from the present
situation.

I think it is fine if you want to have different names for printing and
for canonical use.  It's also ok to change the BFD cache to work
differently (it realpaths since that is convenient and gets good caching
behavior; but you could store some other form of the path instead and
accept cache misses).

It's worth noting that sometimes it is useful to see the full path.
E.g., it can be a way to notice that gdb or the inferior is doing
something unexpected.

Tom

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

[Doug Evans]

On Mon, Sep 9, 2013 at 10:53 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Doug" == Doug Evans <dje@google.com> writes:
>
> Doug> Also, it may be that not all objfiles have a bfd (I don't remember off
> Doug> hand, but whether it's true or not is irrelevant to my point), and if
> Doug> not all objfiles have a bfd we can't just remove objfile->name
> Doug> (assuming objfiles-without-bfds have a name).
>
> There are objfiles without a BFD.  E.g., Java makes one.  Maybe the JIT
> stuff too.  These do still have a name -- objfiles are required to have
> a name.
>
> Doug> OTOH, if objfiles-without-bfds have a name, we could store it in a
> Doug> different place (objfile->foo_name), and thus still effectively have
> Doug> an implementation that only maintains one name, not two
>
> I don't understand this.  It doesn't seem any different from the present
> situation.

You cut out the part where one thought is to have gdb_bfd_open save the name.
If it did that, one *might* not want to keep objfile->name.
And then I was exploring what *might* happen if we did that.

> I think it is fine if you want to have different names for printing and
> for canonical use.

It's not just printing vs canonical use.

> It's also ok to change the BFD cache to work
> differently (it realpaths since that is convenient and gets good caching
> behavior; but you could store some other form of the path instead and
> accept cache misses).

I have a patch that goes that route.  We need something to keep gdb
working here.
It didn't seem likely that making less efficient use of the cache than
is possible would be acceptable, plus y'all are hacking on this
problem, so I've not pushed it upstream.

> It's worth noting that sometimes it is useful to see the full path.
> E.g., it can be a way to notice that gdb or the inferior is doing
> something unexpected.

Indeed.

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

[Jan Kratochvil]

On Thu, 05 Sep 2013 15:18:39 +0200, Jan Kratochvil wrote:
> 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.

obsoleted by:
	[patchv3 5/5] Support .dwp with the name of symlinked binary file
	https://sourceware.org/ml/gdb-patches/2013-09/msg00425.html
	Message-ID: <20130915193818.GE20411@host2.jankratochvil.net>

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

[Jan Kratochvil]

On Fri, 06 Sep 2013 20:29:09 +0200, Doug Evans wrote:
> On Thu, Sep 5, 2013 at 6:18 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> > 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.

The posted patches
	[patchv3 3/5] Code cleanup: Add objfile_name accessor
	[patchv3 4/5] Keep objfile original filename

therefore:
 * unify the two separate strings into one (the bfd one, canonical one).
 * keep backward compatibility all the current code uses the bfd string.
 * preserve the non-canonical string for special cases
   (accessing explicitly objfile->original_name)


> 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.]

I found the foo/bar/../baz paths unexpectedly annoying, after Pedro's
examples.


> 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?

ISTM people want the directories normalized against any "../".

I was giving a countercase where realpath gives worse result
	/home/user/file
->
	/.sdb5/home/user/file
but that happens rarely and the "../" normalization is a better benefit so
that one prefers to use gdb_realpath().


> 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.

IIUC you were proposing thin 'struct gdb_bfd *' wrapper around 'bfd *'.

In the patchset above I have found we do not IMO need non-canonical filename
from 'bfd *', it is enough to have non-canonical filename from 'objfile *'.


> 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.]

See the top paragraph.


> And if we record the original file name we can always
> get the realpath name,

Calling gdb_realpath() dynamically is very expensive.  Fortunately bfd's
canonical filename can be used for it / as a cache.


Jan

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

[Doug Evans]

Jan Kratochvil writes:
 > ISTM people want the directories normalized against any "../".

True.

OTOH, gdb can't know if/when there is an formal abstraction being implemented in symlinks (and thus when to terminate its realpath'ing).  I can imagine providing some means to handle this, but we don't have to add it until there's a compelling need.

 > IIUC you were proposing thin 'struct gdb_bfd *' wrapper around 'bfd *'.

It was a possibility to explore, nothing concrete.

 > In the patchset above I have found we do not IMO need non-canonical filename
 > from 'bfd *', it is enough to have non-canonical filename from 'objfile *'.

Sure.

 > > And if we record the original file name we can always
 > > get the realpath name,
 > 
 > Calling gdb_realpath() dynamically is very expensive.

I wasn't suggesting that (in and of itself). :-)
[modulo, if one were to go in that direction one would implement the realpath cache, and then one might decide whether to still head in that direction]