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

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

[Tom de Vries]

Hi,

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 preventing the creation of this type of partial symbol.

Build and reg-tested on x86_64-linux, native and with target board
cc-with-dwz.

This fixes the only remaining native vs cc-with-dwz regression:
...
FAIL: gdb.ada/maint_with_ada.exp: maintenance check-psymtabs
...

OK for trunk?

Thanks,
- Tom

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

gdb/ChangeLog:

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

	PR symtab/25256
	* dwarf2/read.c (add_partial_subprogram): Don't create partial symbol
	without pc info.

gdb/testsuite/ChangeLog:

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

	PR symtab/25256
	* gdb.base/check-psymtab.c: New test.
	* gdb.base/check-psymtab.exp: New file.

---
 gdb/dwarf2/read.c                        |  2 +-
 gdb/testsuite/gdb.base/check-psymtab.c   | 28 ++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/check-psymtab.exp | 27 +++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 1d4397dfab..299f532088 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -8445,7 +8445,7 @@ add_partial_subprogram (struct partial_die_info *pdi,
 	    }
         }
 
-      if (pdi->has_pc_info || (!pdi->is_external && pdi->may_be_inlined))
+      if (pdi->has_pc_info)
 	{
           if (!pdi->is_declaration)
 	    /* Ignore subprogram DIEs that do not have a name, they are
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"

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

[Tom Tromey]

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

thanks,
Tom

[committed][gdb/testsuite] Add test-case gdb.dwarf2/break-inline-psymtab.exp

[Tom de Vries]

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

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

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

I managed to construct a test-case like that, which:
- passes with master, and
- fails if I apply the "preventing the creation of this type of partial
  symbol" patch.

Since it seems there's no other test-case in the testsuite testing this
behaviour ... committed as obvious.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-testsuite-Add-test-case-gdb.dwarf2-break-inline-psymtab.exp.patch --]
[-- Type: text/x-patch, Size: 4493 bytes --]

[gdb/testsuite] Add test-case gdb.dwarf2/break-inline-psymtab.exp

Add a test-case that tests whether we can set a breakpoint on an inlined
inline function in CU for which the partial symtab has not yet been expanded.

Tested on x86_64-linux, with gcc 4.8.5, gcc-7.5.0, gcc-10.0.1, and clang
5.0.2.

gdb/testsuite/ChangeLog:

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

	* gdb.dwarf2/break-inline-psymtab-2.c: New test.
	* gdb.dwarf2/break-inline-psymtab.c: New test.
	* gdb.dwarf2/break-inline-psymtab.exp: New file.

---
 gdb/testsuite/gdb.dwarf2/break-inline-psymtab-2.c | 33 +++++++++++++++++++++
 gdb/testsuite/gdb.dwarf2/break-inline-psymtab.c   | 24 +++++++++++++++
 gdb/testsuite/gdb.dwarf2/break-inline-psymtab.exp | 36 +++++++++++++++++++++++
 3 files changed, 93 insertions(+)

diff --git a/gdb/testsuite/gdb.dwarf2/break-inline-psymtab-2.c b/gdb/testsuite/gdb.dwarf2/break-inline-psymtab-2.c
new file mode 100644
index 0000000000..38c69336f2
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/break-inline-psymtab-2.c
@@ -0,0 +1,33 @@
+/* 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/>.  */
+
+extern int foo (void);
+
+int a;
+
+static inline int
+bar (void)
+{
+  a = 2;
+  return 0;
+}
+
+int
+foo (void)
+{
+  return bar ();
+}
diff --git a/gdb/testsuite/gdb.dwarf2/break-inline-psymtab.c b/gdb/testsuite/gdb.dwarf2/break-inline-psymtab.c
new file mode 100644
index 0000000000..74cea4bf90
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/break-inline-psymtab.c
@@ -0,0 +1,24 @@
+/* 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/>.  */
+
+extern int foo (void);
+
+int
+main (void)
+{
+  return foo ();
+}
diff --git a/gdb/testsuite/gdb.dwarf2/break-inline-psymtab.exp b/gdb/testsuite/gdb.dwarf2/break-inline-psymtab.exp
new file mode 100644
index 0000000000..adbe8620aa
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/break-inline-psymtab.exp
@@ -0,0 +1,36 @@
+# Copyright 2019-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 break-inline-psymtab.c break-inline-psymtab-2.c
+
+set sources [list $srcfile $srcfile2]
+set opts {debug optimize=-O2}
+if { [prepare_for_testing "failed to prepare" ${testfile} $sources $opts] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+get_compiler_info
+get_debug_format
+if { [skip_inline_frame_tests] } {
+    return -1
+}
+
+# Set a break-point in inline function bar, in a CU for which the partial
+# symtab has not been expanded.
+gdb_breakpoint "bar" message

Re: [committed][gdb/testsuite] Add test-case gdb.dwarf2/break-inline-psymtab.exp

[Pedro Alves]

On 3/18/20 1:48 PM, Tom de Vries wrote:
> +set sources [list $srcfile $srcfile2]
> +set opts {debug optimize=-O2}
> +if { [prepare_for_testing "failed to prepare" ${testfile} $sources $opts] } {
> +    return -1
> +}

Is that -O2 needed to force inlining?

-O0 and __attribute__((always_inline)) would be better for that.

Thanks,
Pedro Alves

Re: [committed][gdb/testsuite] Add test-case gdb.dwarf2/break-inline-psymtab.exp

[Tom de Vries]

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

On 02-04-2020 16:24, Pedro Alves wrote:
> On 3/18/20 1:48 PM, Tom de Vries wrote:
>> +set sources [list $srcfile $srcfile2]
>> +set opts {debug optimize=-O2}
>> +if { [prepare_for_testing "failed to prepare" ${testfile} $sources $opts] } {
>> +    return -1
>> +}
> 
> Is that -O2 needed to force inlining?
> 
> -O0 and __attribute__((always_inline)) would be better for that.
> 

Ack, thanks for noticing, fixed in commit below, committed.

- Tom



[-- Attachment #2: 0001-gdb-testsuite-Don-t-use-O2-for-inlining-in-break-inline-psymtab.exp.patch --]
[-- Type: text/x-patch, Size: 1717 bytes --]

[gdb/testsuite] Don't use O2 for inlining in break-inline-psymtab.exp

In test-case gdb.dwarf2/break-inline-psymtab.exp we use O2 to enable inlining
of bar into foo in break-inline-psymtab-2.c.

Instead, enforce inlining using __attribute__((always_inline)), to avoid any
optimization-related test issues.

Tested on x86_64-linux.

gdb/testsuite/ChangeLog:

2020-04-02  Tom de Vries  <tdevries@suse.de>

	* gdb.dwarf2/break-inline-psymtab-2.c (bar): Add
	__attribute__((always_inline)).
	* gdb.dwarf2/break-inline-psymtab.exp: Don't use -O2.

---
 gdb/testsuite/gdb.dwarf2/break-inline-psymtab-2.c | 2 +-
 gdb/testsuite/gdb.dwarf2/break-inline-psymtab.exp | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.dwarf2/break-inline-psymtab-2.c b/gdb/testsuite/gdb.dwarf2/break-inline-psymtab-2.c
index 38c69336f2..b7fe485b3a 100644
--- a/gdb/testsuite/gdb.dwarf2/break-inline-psymtab-2.c
+++ b/gdb/testsuite/gdb.dwarf2/break-inline-psymtab-2.c
@@ -19,7 +19,7 @@ extern int foo (void);
 
 int a;
 
-static inline int
+static inline int __attribute__((always_inline))
 bar (void)
 {
   a = 2;
diff --git a/gdb/testsuite/gdb.dwarf2/break-inline-psymtab.exp b/gdb/testsuite/gdb.dwarf2/break-inline-psymtab.exp
index adbe8620aa..344d7da0d5 100644
--- a/gdb/testsuite/gdb.dwarf2/break-inline-psymtab.exp
+++ b/gdb/testsuite/gdb.dwarf2/break-inline-psymtab.exp
@@ -16,8 +16,7 @@
 standard_testfile break-inline-psymtab.c break-inline-psymtab-2.c
 
 set sources [list $srcfile $srcfile2]
-set opts {debug optimize=-O2}
-if { [prepare_for_testing "failed to prepare" ${testfile} $sources $opts] } {
+if { [prepare_for_testing "failed to prepare" ${testfile} $sources] } {
     return -1
 }
 

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

[Tom de Vries]

[-- 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"

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

[Tom de Vries]

On 18-03-2020 16:58, Tom de Vries wrote:
> 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?
> 

Ping.

Thanks,- Tom

> 0001-gdb-symtab-Fix-check-psymtab-failure-for-inline-function.patch
> 
> [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"
> 

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

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> This patch fixes things in the check itself instead.

Tom> OK for trunk?

Tom> gdb/ChangeLog:

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

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

Looks good to me, thanks.

I think the test suite changes are missing a ChangeLog entry.

Tom