[PATCH] Fix symtab/23853: symlinked default symtab

[PATCH] Fix symtab/23853: symlinked default symtab

[Keith Seitz]

This patch attempts to fix a bug dealing with setting breakpoints
in default symtabs that are symlinks.  For example:

(gdb) list
11	   GNU General Public License for more details.
12
13	   You should have received a copy of the GNU General Public License
14	   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
15
16	static int
17	foo (void)
18	{
19	  return 0; /* break here  */
20	}
(gdb)
21
22	int
23	main (void)
24	{
25	  return foo ();
26	}
(gdb) b 19
No line 19 in the current file.
Make breakpoint pending on future shared library load? (y or [n])

The problem here is that when convert_sals_line_offset sets the default
symtab, it immediately calls symtab_to_fullname, passing that fullname
to collect_symtabs_from_filename.

When we iterate over the compunit_symtabs in iterate_over_some_symtabs,
NAME is this the fullname from symtab_to_fullname while the symtab
filename is the symlink name (gotten from debuginfo).
compare_filenames_for_search is then called to ascertain a match between,
e.g., "symlink.c" and "/path/to/realsource.c".
The first comparison fails (compare_filenames_for_search returns early
with a simple strlen comparison).

Later in iterate_over_some_symtabs, symtab_to_fullname would be called
with the result of symtab_to_fullname, but only if the user has set
"basenames-may-differ."  This flag defaults to "off" because, as
iterate_over_some_symtabs says, realpath is very expensive to use.

However, there seems little reason not to check the fullname if it is
already known (non-NULL).  That's what this patch does.

gdb/ChangeLog

	PR symtab/23853
	* symtab.c (iterate_over_some_symtabs): If the symtab has a
	fullname, check whether it is a filename match, too.

gdb/testsuite/ChangeLog

	PR symtab/23853
	* gdb.base/symlink-sourcefile.c: New file.
	* gdb.base/symlink-sourcefile.exp: New file.
---
 gdb/ChangeLog                                 |  6 +++
 gdb/symtab.c                                  |  4 +-
 gdb/testsuite/ChangeLog                       |  6 +++
 gdb/testsuite/gdb.base/symlink-sourcefile.c   | 26 +++++++++++
 gdb/testsuite/gdb.base/symlink-sourcefile.exp | 44 +++++++++++++++++++
 5 files changed, 85 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/symlink-sourcefile.c
 create mode 100644 gdb/testsuite/gdb.base/symlink-sourcefile.exp

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index bc77fe85c7..557c72b203 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+YYYY-MM-DD  Keith Seitz  <keiths@redhat.com>
+
+	PR symtab/23853
+	* symtab.c (iterate_over_some_symtabs): If the symtab has a
+	fullname, check whether it is a filename match, too.
+
 2018-11-04  Tom Tromey  <tom@tromey.com>
 
 	* varobj.c (install_default_visualizer): Update.
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 7649908d9c..5b39db6d33 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -412,7 +412,9 @@ iterate_over_some_symtabs (const char *name,
     {
       ALL_COMPUNIT_FILETABS (cust, s)
 	{
-	  if (compare_filenames_for_search (s->filename, name))
+	  if (compare_filenames_for_search (s->filename, name)
+	      || (s->fullname != nullptr
+		  && compare_filenames_for_search (s->fullname, name)))
 	    {
 	      if (callback (s))
 		return true;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 0df75aa54f..4befa4f4c9 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+YYYY-MM-DD  Keith Seitz  <keiths@redhat.com>
+
+	PR symtab/23853
+	* gdb.base/symlink-sourcefile.c: New file.
+	* gdb.base/symlink-sourcefile.exp: New file.
+
 2018-11-01  Joel Brobecker  <brobecker@adacore.com>
 
 	* gdb.ada/watch_minus_l: New testcase.
diff --git a/gdb/testsuite/gdb.base/symlink-sourcefile.c b/gdb/testsuite/gdb.base/symlink-sourcefile.c
new file mode 100644
index 0000000000..2cf0d0e2fe
--- /dev/null
+++ b/gdb/testsuite/gdb.base/symlink-sourcefile.c
@@ -0,0 +1,26 @@
+/* Copyright 2018 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/>.  */
+
+static int
+foo (void)
+{
+  return 0; /* break here  */
+}
+
+int
+main (void)
+{
+  return foo ();
+}
diff --git a/gdb/testsuite/gdb.base/symlink-sourcefile.exp b/gdb/testsuite/gdb.base/symlink-sourcefile.exp
new file mode 100644
index 0000000000..084be07151
--- /dev/null
+++ b/gdb/testsuite/gdb.base/symlink-sourcefile.exp
@@ -0,0 +1,44 @@
+# Copyright 2018 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/>.
+
+# Test GDB when source files are symlinks.
+standard_testfile
+
+set test "file symbolic link name"
+set linksrc "link-${srcfile}"
+set srcfilelink [standard_output_file $linksrc]
+
+
+# Remove any existing symlink and build executable using the
+# symlink.
+remote_file host delete $srcfilelink
+set status [remote_exec host \
+		"ln -sf $srcdir/$subdir/$srcfile $srcfilelink"]
+if {[lindex $status 0] != 0} {
+    unsupported "$test (host does not support symbolic links)"
+    return 0
+}
+
+if {[prepare_for_testing $testfile $testfile $srcfilelink]} {
+    return -1
+}
+
+if {![runto_main]} {
+    untested "could not run to main"
+    return -1
+}
+
+gdb_breakpoint [gdb_get_line_number "break here" $srcfile] message
+gdb_continue_to_breakpoint "run to breakpoint marker"
-- 
2.17.2

Re: [PATCH] Fix symtab/23853: symlinked default symtab

[Pedro Alves]

On 11/05/2018 09:19 PM, Keith Seitz wrote:
> This patch attempts to fix a bug dealing with setting breakpoints
> in default symtabs that are symlinks.  For example:
> 
> (gdb) list
> 11	   GNU General Public License for more details.
> 12
> 13	   You should have received a copy of the GNU General Public License
> 14	   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> 15
> 16	static int
> 17	foo (void)
> 18	{
> 19	  return 0; /* break here  */
> 20	}
> (gdb)
> 21
> 22	int
> 23	main (void)
> 24	{
> 25	  return foo ();
> 26	}
> (gdb) b 19
> No line 19 in the current file.
> Make breakpoint pending on future shared library load? (y or [n])
> 
> The problem here is that when convert_sals_line_offset sets the default

s/convert_sals_line_offset/create_sals_line_offset/

> symtab, it immediately calls symtab_to_fullname, passing that fullname
> to collect_symtabs_from_filename.
> 
> When we iterate over the compunit_symtabs in iterate_over_some_symtabs,
> NAME is this the fullname from symtab_to_fullname while the symtab
> filename is the symlink name (gotten from debuginfo).
> compare_filenames_for_search is then called to ascertain a match between,
> e.g., "symlink.c" and "/path/to/realsource.c".
> The first comparison fails (compare_filenames_for_search returns early
> with a simple strlen comparison).
> 
> Later in iterate_over_some_symtabs, symtab_to_fullname would be called
> with the result of symtab_to_fullname, but only if the user has set

This sentence doesn't seem to make sense (symtab_to_fullname twice)?

I think you meant something like this:

  Later in iterate_over_some_symtabs, the passed-in fullname would be compared
  with the symtab's fullname but only if the user has set

> "basenames-may-differ."  This flag defaults to "off" because, as
> iterate_over_some_symtabs says, realpath is very expensive to use.
> 
> However, there seems little reason not to check the fullname if it is
> already known (non-NULL).  That's what this patch does.
> 

My first reaction was, why are we doing that, passing down the
fullname instead of the symtab's filename, especially considering
basenames_may_differ.  Pedantically looks odd at first sight.

I suppose I don't see a great reason to not chech fullname either,
so I guess that's fine.

Actually, if create_sals_line_offset has a default symtab handy, why
does it need to iterate over symtabs?  Couldn't it just use the
default directly?

> gdb/ChangeLog
> 
> 	PR symtab/23853
> 	* symtab.c (iterate_over_some_symtabs): If the symtab has a
> 	fullname, check whether it is a filename match, too.
> 
> gdb/testsuite/ChangeLog
> 
> 	PR symtab/23853
> 	* gdb.base/symlink-sourcefile.c: New file.
> 	* gdb.base/symlink-sourcefile.exp: New file.
> ---
>  gdb/ChangeLog                                 |  6 +++
>  gdb/symtab.c                                  |  4 +-
>  gdb/testsuite/ChangeLog                       |  6 +++
>  gdb/testsuite/gdb.base/symlink-sourcefile.c   | 26 +++++++++++
>  gdb/testsuite/gdb.base/symlink-sourcefile.exp | 44 +++++++++++++++++++
>  5 files changed, 85 insertions(+), 1 deletion(-)
>  create mode 100644 gdb/testsuite/gdb.base/symlink-sourcefile.c
>  create mode 100644 gdb/testsuite/gdb.base/symlink-sourcefile.exp
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index bc77fe85c7..557c72b203 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,9 @@
> +YYYY-MM-DD  Keith Seitz  <keiths@redhat.com>
> +
> +	PR symtab/23853
> +	* symtab.c (iterate_over_some_symtabs): If the symtab has a
> +	fullname, check whether it is a filename match, too.
> +
>  2018-11-04  Tom Tromey  <tom@tromey.com>
>  
>  	* varobj.c (install_default_visualizer): Update.
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 7649908d9c..5b39db6d33 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -412,7 +412,9 @@ iterate_over_some_symtabs (const char *name,
>      {
>        ALL_COMPUNIT_FILETABS (cust, s)
>  	{
> -	  if (compare_filenames_for_search (s->filename, name))
> +	  if (compare_filenames_for_search (s->filename, name)
> +	      || (s->fullname != nullptr
> +		  && compare_filenames_for_search (s->fullname, name)))

This may use a comment.  Otherwise a reader is wondering why we're comparing
the fullname, when basenames_may_differ is false, which pedantically looks
odd at first sight.

Also, I'm looking at this wondering about redundant checks and
efficiency.

I.e., if s->fullname is not NULL and the new check fails above, then
this one below:

	  if (compare_filenames_for_search (symtab_to_fullname (s), name))
	    {

... is guaranteed to fail too, AFAICS.  That would suggest tweaking
that one to:

	  if (s->fullname == nullptr 
              || compare_filenames_for_search (symtab_to_fullname (s), name))
	    {

then.  Right?

I noticed that psym_map_symtabs_matching_filename uses the same
search algorithm, which looks like should be changed to match.
Maybe dw2_map_symtabs_matching_filename and 
skiplist_entry::do_skip_file_p too.  And others.  Not sure.
I just grepped for basenames_may_differ and found these.

> +
> +# Test GDB when source files are symlinks.

Please mention the specific case of the default symtab somewhere.
Either here, or at the gdb_breakpoint line below.  That's important
for the test, right?

> +standard_testfile
> +
> +set test "file symbolic link name"
> +set linksrc "link-${srcfile}"
> +set srcfilelink [standard_output_file $linksrc]
> +
> +
> +# Remove any existing symlink and build executable using the
> +# symlink.
> +remote_file host delete $srcfilelink
> +set status [remote_exec host \
> +		"ln -sf $srcdir/$subdir/$srcfile $srcfilelink"]
> +if {[lindex $status 0] != 0} {
> +    unsupported "$test (host does not support symbolic links)"
> +    return 0
> +}
> +
> +if {[prepare_for_testing $testfile $testfile $srcfilelink]} {
> +    return -1
> +}
> +
> +if {![runto_main]} {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +gdb_breakpoint [gdb_get_line_number "break here" $srcfile] message
> +gdb_continue_to_breakpoint "run to breakpoint marker"
>  
Thanks,
Pedro Alves

Re: [PATCH] Fix symtab/23853: symlinked default symtab

[Keith Seitz]

On 11/28/18 11:17 AM, Pedro Alves wrote:> On 11/05/2018 09:19 PM, Keith Seitz wrote:
>> The problem here is that when convert_sals_line_offset sets the default
> 
> s/convert_sals_line_offset/create_sals_line_offset/

Fixed.

>> Later in iterate_over_some_symtabs, symtab_to_fullname would be called
>> with the result of symtab_to_fullname, but only if the user has set
> 
> This sentence doesn't seem to make sense (symtab_to_fullname twice)?
> 
> I think you meant something like this:
> 
>   Later in iterate_over_some_symtabs, the passed-in fullname would be compared
>   with the symtab's fullname but only if the user has set

I've rewritten/simplified this patch, so this sentence is gone. Which is
a form of fixed. :-)

>> "basenames-may-differ."  This flag defaults to "off" because, as
>> iterate_over_some_symtabs says, realpath is very expensive to use.
>>
>> However, there seems little reason not to check the fullname if it is
>> already known (non-NULL).  That's what this patch does.
>>
> 
> My first reaction was, why are we doing that, passing down the
> fullname instead of the symtab's filename, especially considering
> basenames_may_differ.  Pedantically looks odd at first sight.

Yes, indeed it does, and after some investigation, I'm convinced enough
to submit this revision using this.  The symtab_to_filename/fullname
thing was introduced in a series in 2007 dealing with absolute filenames.
In fact, the ChangeLog entry for this particular change says that it is
using "symtab_to_filename_for_display," even though the patch doesn't
include such a function. [It is simply using symtab_to_filename.]

Clearly, this was introduced for dealing with displaying symtab filenames,
not searching for them.  Since we already have the actual debug info filename
in the symtab, we can look them up using that, and, as expected, it works.

> Actually, if create_sals_line_offset has a default symtab handy, why
> does it need to iterate over symtabs?  Couldn't it just use the
> default directly?

That's a good question. Short answer: For developers that do elaborate
#if..#endif and multiple compilations on a single source file. [See
gdb.base/expand-pysmtabs.exp as an example.] Without the iteration,
this test fails.
 
This does /not/ address the previous patch's "compare fullname if it
is available" approach, which I still think is correct, but I would need
to find a way to trigger that in a test, and I've not done that.

>> +
>> +# Test GDB when source files are symlinks.
> 
> Please mention the specific case of the default symtab somewhere.
> Either here, or at the gdb_breakpoint line below.  That's important
> for the test, right?

Done.

Keith

Differences from last revision:
- Simplify patch -- remove fullname lookups and changes to
  iterate_over_some_symtabs
- Fixed typos in commit log
- Add more/better description of test to .exp

-----

This patch attempts to fix a bug dealing with setting breakpoints
in default symtabs that are symlinks.  For example:

(gdb) list
11	   GNU General Public License for more details.
12
13	   You should have received a copy of the GNU General Public License
14	   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
15
16	static int
17	foo (void)
18	{
19	  return 0; /* break here  */
20	}
(gdb)
21
22	int
23	main (void)
24	{
25	  return foo ();
26	}
(gdb) b 19
No line 19 in the current file.
Make breakpoint pending on future shared library load? (y or [n])

The problem here is that when create_sals_line_offset sets the default
symtab, it immediately calls symtab_to_fullname, passing that fullname
to collect_symtabs_from_filename to find all matching symtabs.  This
fails because we end up looking for a symtab with the name of the
actual file on disk (which is different in this case because of the
symlink) instead of the one stored in the debug info.

Since we already have the lookup name of the default symtab, use it
instead of the fullname. [This fullname thing was originally added
in 2007 in a series dealing with *displaying* absolute file names.
Clearly, this instance has nothing to do with the display of file names.]

gdb/ChangeLog

	PR symtab/23853
	* linespec.c (create_sals_line_offset): Search for the default
	symtab's filename instead of its fullname.

gdb/testsuite/ChangeLog

	PR symtab/23853
	* gdb.base/symlink-sourcefile.c: New file.
	* gdb.base/symlink-sourcefile.exp: New file.
---
 gdb/linespec.c                                |  6 +--
 gdb/testsuite/gdb.base/symlink-sourcefile.c   | 26 +++++++++++
 gdb/testsuite/gdb.base/symlink-sourcefile.exp | 45 +++++++++++++++++++
 3 files changed, 73 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/symlink-sourcefile.c
 create mode 100644 gdb/testsuite/gdb.base/symlink-sourcefile.exp

diff --git a/gdb/linespec.c b/gdb/linespec.c
index cedbd39bd7..e902b11c8e 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -2102,16 +2102,14 @@ create_sals_line_offset (struct linespec_state *self,
   if (ls->file_symtabs->size () == 1
       && ls->file_symtabs->front () == nullptr)
     {
-      const char *fullname;
-
       set_current_program_space (self->program_space);
 
       /* Make sure we have at least a default source line.  */
       set_default_source_symtab_and_line ();
       initialize_defaults (&self->default_symtab, &self->default_line);
-      fullname = symtab_to_fullname (self->default_symtab);
       *ls->file_symtabs
-	= collect_symtabs_from_filename (fullname, self->search_pspace);
+	= collect_symtabs_from_filename (self->default_symtab->filename,
+					 self->search_pspace);
       use_default = 1;
     }
 
diff --git a/gdb/testsuite/gdb.base/symlink-sourcefile.c b/gdb/testsuite/gdb.base/symlink-sourcefile.c
new file mode 100644
index 0000000000..12cd96892a
--- /dev/null
+++ b/gdb/testsuite/gdb.base/symlink-sourcefile.c
@@ -0,0 +1,26 @@
+/* Copyright 2019 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/>.  */
+
+static int
+foo (void)
+{
+  return 0; /* break here  */
+}
+
+int
+main (void)
+{
+  return foo ();
+}
diff --git a/gdb/testsuite/gdb.base/symlink-sourcefile.exp b/gdb/testsuite/gdb.base/symlink-sourcefile.exp
new file mode 100644
index 0000000000..ae43934ff4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/symlink-sourcefile.exp
@@ -0,0 +1,45 @@
+# Copyright 2019 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/>.
+
+# Test that GDB can find the default symtab when it is a symbolic link.
+standard_testfile
+
+set test "file symbolic link name"
+set linksrc "link-${srcfile}"
+set srcfilelink [standard_output_file $linksrc]
+
+
+# Remove any existing symlink and build executable using a
+# symbolic link to the actual source file.
+remote_file host delete $srcfilelink
+set status [remote_exec host \
+		"ln -sf $srcdir/$subdir/$srcfile $srcfilelink"]
+if {[lindex $status 0] != 0} {
+    unsupported "$test (host does not support symbolic links)"
+    return 0
+}
+
+if {[prepare_for_testing $testfile $testfile $srcfilelink]} {
+    return -1
+}
+
+if {![runto_main]} {
+    untested "could not run to main"
+    return -1
+}
+
+# Using a line number ensures that the default symtab is used.
+gdb_breakpoint [gdb_get_line_number "break here" $srcfile] message
+gdb_continue_to_breakpoint "run to breakpoint marker"
-- 
2.20.1

Re: [PATCH] Fix symtab/23853: symlinked default symtab

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> Differences from last revision:
Keith> - Simplify patch -- remove fullname lookups and changes to
Keith>   iterate_over_some_symtabs
Keith> - Fixed typos in commit log
Keith> - Add more/better description of test to .exp
[...]

I read this and it looks reasonable to me.
Please check it in.

thanks for doing this,
Tom

Re: [PATCH] Fix symtab/23853: symlinked default symtab

[Keith Seitz]

On 2/21/19 2:15 PM, Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
> 
> Keith> Differences from last revision:
> Keith> - Simplify patch -- remove fullname lookups and changes to
> Keith>   iterate_over_some_symtabs
> Keith> - Fixed typos in commit log
> Keith> - Add more/better description of test to .exp
> [...]
> 
> I read this and it looks reasonable to me.
> Please check it in.

Thank you for reviewing. I've pushed this (and the missing
ChangeLog entries in a follow-up commit).

/me dreams of getting rid of ChangeLogs

Keith