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

[Keith Seitz <keiths at redhat dot com>]

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