Re: [PATCH][gdb/symtab] Fix check-psymtab failure for inline function

[Tom de Vries <tdevries@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 1258 bytes --]

On 11-03-2020 01:08, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> When loading the executable in gdb, we create a partial symbol for foo, but
> Tom> after expansion into a full symbol table no actual symbol is created,
> Tom> resulting in a maint check-psymtab failure:
> Tom> ...
> Tom> (gdb) maint check-psymtab
> Tom> Static symbol `foo' only found in inline.c psymtab
> Tom> ...
> 
> Tom> Fix this by preventing the creation of this type of partial symbol.
> 
> With this patch, if there is a function which is only inlined (there is
> no "out-line" copy), will "break function" still work?
> 
> It seems to me that it wouldn't always, because there wouldn't be a
> partial symbol to match.  Maybe it might work accidentally if the inline
> function appears in a CU that is already expanded, like main's.  So the
> test would have to be an always-inline function that isn't used in
> main's CU.
> 
> If it does work, then I wonder how, since my mental model is that all
> the paths to expansion require a matching partial symbol.
> 
> If it doesn't work, then I think a different fix is needed.

Indeed, a different fix is needed.

This patch fixes things in the check itself instead.

OK for trunk?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-symtab-Fix-check-psymtab-failure-for-inline-function.patch --]
[-- Type: text/x-patch, Size: 5967 bytes --]

[gdb/symtab] Fix check-psymtab failure for inline function

Consider test-case inline.c, containing an inline function foo:
...
static inline int foo (void) { return 0; }
int main (void) { return foo (); }
...

And the test-case compiled with -O2 and debug info:
...
$ gcc -g inline.c -O2
...

This results in a DWARF entry for foo without pc info:
...
<1><114>: Abbrev Number: 4 (DW_TAG_subprogram)
   <115>   DW_AT_name        : foo
   <119>   DW_AT_decl_file   : 1
   <11a>   DW_AT_decl_line   : 2
   <11b>   DW_AT_prototyped  : 1
   <11b>   DW_AT_type        : <0x10d>
   <11f>   DW_AT_inline      : 3       (declared as inline and inlined)
...

When loading the executable in gdb, we create a partial symbol for foo, but
after expansion into a full symbol table no actual symbol is created,
resulting in a maint check-psymtab failure:
...
(gdb) maint check-psymtab
Static symbol `foo' only found in inline.c psymtab
...

Fix this by skipping this type of partial symbol during the check.

Note that we're not fixing this by not creating the partial symbol, because
this breaks setting a breakpoint on an inlined inline function in a CU for
which the partial symtab has not been expanded (test-case
gdb.dwarf2/break-inline-psymtab.exp).

Tested on x86_64-linux.

gdb/ChangeLog:

2020-03-18  Tom de Vries  <tdevries@suse.de>

	* psymtab.c (maintenance_check_psymtabs): Skip static LOC_BLOCK
	symbols without address.

---
 gdb/psymtab.c                            | 16 +++++++++-------
 gdb/testsuite/gdb.base/check-psymtab.c   | 28 ++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/check-psymtab.exp | 27 +++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 7 deletions(-)

diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index f77f6d5108..b65795d062 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -2104,7 +2104,7 @@ maintenance_check_psymtabs (const char *ignore, int from_tty)
   struct compunit_symtab *cust = NULL;
   const struct blockvector *bv;
   const struct block *b;
-  int length;
+  int i;
 
   for (objfile *objfile : current_program_space->objfiles ())
     for (partial_symtab *ps : require_partial_symbols (objfile, true))
@@ -2138,9 +2138,14 @@ maintenance_check_psymtabs (const char *ignore, int from_tty)
 	b = BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK);
 	partial_symbol **psym
 	  = &objfile->partial_symtabs->static_psymbols[ps->statics_offset];
-	length = ps->n_static_syms;
-	while (length--)
+	for (i = 0; i < ps->n_static_syms; psym++, i++)
 	  {
+	    /* Skip symbols for inlined functions without address.  These may
+	       or may not have a match in the full symtab.  */
+	    if ((*psym)->aclass == LOC_BLOCK
+		&& (*psym)->ginfo.value.address == 0)
+	      continue;
+
 	    sym = block_lookup_symbol (b, (*psym)->ginfo.search_name (),
 				       symbol_name_match_type::SEARCH_NAME,
 				       (*psym)->domain);
@@ -2152,12 +2157,10 @@ maintenance_check_psymtabs (const char *ignore, int from_tty)
 		puts_filtered (ps->filename);
 		printf_filtered (" psymtab\n");
 	      }
-	    psym++;
 	  }
 	b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
 	psym = &objfile->partial_symtabs->global_psymbols[ps->globals_offset];
-	length = ps->n_global_syms;
-	while (length--)
+	for (i = 0; i < ps->n_global_syms; psym++, i++)
 	  {
 	    sym = block_lookup_symbol (b, (*psym)->ginfo.search_name (),
 				       symbol_name_match_type::SEARCH_NAME,
@@ -2170,7 +2173,6 @@ maintenance_check_psymtabs (const char *ignore, int from_tty)
 		puts_filtered (ps->filename);
 		printf_filtered (" psymtab\n");
 	      }
-	    psym++;
 	  }
 	if (ps->raw_text_high () != 0
 	    && (ps->text_low (objfile) < BLOCK_START (b)
diff --git a/gdb/testsuite/gdb.base/check-psymtab.c b/gdb/testsuite/gdb.base/check-psymtab.c
new file mode 100644
index 0000000000..2c1e69955e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/check-psymtab.c
@@ -0,0 +1,28 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 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 inline int
+foo (void)
+{
+  return 0;
+}
+
+int
+main (void)
+{
+  return foo ();
+}
diff --git a/gdb/testsuite/gdb.base/check-psymtab.exp b/gdb/testsuite/gdb.base/check-psymtab.exp
new file mode 100644
index 0000000000..06438fc7c0
--- /dev/null
+++ b/gdb/testsuite/gdb.base/check-psymtab.exp
@@ -0,0 +1,27 @@
+# Copyright 2020 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 {[prepare_for_testing "failed to prepare" $testfile $srcfile \
+	 {debug optimize=-O2}]} {
+    return -1
+}
+
+gdb_test_no_output "maint expand-symtabs"
+
+# Check that we don't get:
+#   Static symbol `foo' only found in check-psymtab.c psymtab
+gdb_test_no_output "maint check-psymtab"