[PATCH][gdb/symtab] Handle .gdb_index in ada language mode

[PATCH][gdb/symtab] Handle .gdb_index in ada language mode

[Tom de Vries]

Hi,

When running test-case gdb.base/with.exp with target board cc-with-gdb-index,
we have:
...
(gdb) PASS: gdb.base/with.exp: basics: show language
with language ada -- print g_s^M
'g_s' has unknown type; cast it to its declared type^M
(gdb) FAIL: gdb.base/with.exp: basics: with language ada -- print g_s
...

This is due to this bit in dw2_map_matching_symbols:
...
  if (dwarf2_per_objfile->index_table != nullptr)
    {
      /* Ada currently doesn't support .gdb_index (see PR24713).  We can get
	 here though if the current language is Ada for a non-Ada objfile
	 using GNU index.  As Ada does not look for non-Ada symbols this
	 function should just return.  */
      return;
    }
...

While the reasoning in the comment may be sound from language perspective, it
does introduce an inconsistency in gdb behaviour between:
- having a .gdb_index section, and
- having a .gdb_names section, or a partial symtab, or -readnow.

Fix the inconsistency by completing implementation of
dw2_map_matching_symbols.

Tested on x86_64-linux, both with native and target board
cc-with-debug-index.

Any comments?

Thanks,
- Tom

[gdb/symtab] Handle .gdb_index in ada language mode

gdb/ChangeLog:

2020-05-13  Tom de Vries  <tdevries@suse.de>

	PR symtab/25833
	* dwarf2/read.c (dw2_map_matching_symbols): Handle .gdb_index.

gdb/testsuite/ChangeLog:

2020-05-13  Tom de Vries  <tdevries@suse.de>

	PR symtab/25833
	* gdb.base/with-mf-inc.c: New test.
	* gdb.base/with-mf-main.c: New test.
	* gdb.base/with-mf.exp: New file.

---
 gdb/dwarf2/read.c                     | 52 +++++++++++++++++++++++++++++------
 gdb/testsuite/gdb.base/with-mf-inc.c  | 35 +++++++++++++++++++++++
 gdb/testsuite/gdb.base/with-mf-main.c | 28 +++++++++++++++++++
 gdb/testsuite/gdb.base/with-mf.exp    | 34 +++++++++++++++++++++++
 4 files changed, 141 insertions(+), 8 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 27bf40a898..5297c107a6 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -3649,6 +3649,20 @@ dw2_expand_symtabs_with_fullname (struct objfile *objfile,
     }
 }
 
+static void
+dw2_expand_symtabs_matching_symbol
+  (mapped_index_base &index,
+   const lookup_name_info &lookup_name_in,
+   gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher,
+   enum search_domain kind,
+   gdb::function_view<bool (offset_type)> match_callback);
+
+static void
+dw2_expand_symtabs_matching_one
+  (struct dwarf2_per_cu_data *per_cu,
+   gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher,
+   gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify);
+
 static void
 dw2_map_matching_symbols
   (struct objfile *objfile,
@@ -3661,19 +3675,41 @@ dw2_map_matching_symbols
   struct dwarf2_per_objfile *dwarf2_per_objfile
     = get_dwarf2_per_objfile (objfile);
 
+  const block_enum block_kind = global ? GLOBAL_BLOCK : STATIC_BLOCK;
+
   if (dwarf2_per_objfile->index_table != nullptr)
     {
       /* Ada currently doesn't support .gdb_index (see PR24713).  We can get
 	 here though if the current language is Ada for a non-Ada objfile
-	 using GNU index.  As Ada does not look for non-Ada symbols this
-	 function should just return.  */
-      return;
-    }
+	 using GNU index.  */
+      mapped_index &index = *dwarf2_per_objfile->index_table;
+
+      const char *match_name = name.ada ().lookup_name ().c_str ();
+      auto matcher = [&] (const char *symname)
+	{
+	  if (ordered_compare == nullptr)
+	    return true;
+	  return ordered_compare (symname, match_name) == 0;
+	};
+
+      dw2_expand_symtabs_matching_symbol (index, name, matcher, ALL_DOMAIN,
+					  [&] (offset_type namei)
+      {
+	struct dw2_symtab_iterator iter;
+	struct dwarf2_per_cu_data *per_cu;
 
-  /* We have -readnow: no .gdb_index, but no partial symtabs either.  So,
-     inline psym_map_matching_symbols here, assuming all partial symtabs have
-     been read in.  */
-  const int block_kind = global ? GLOBAL_BLOCK : STATIC_BLOCK;
+	dw2_symtab_iter_init (&iter, dwarf2_per_objfile, block_kind, domain,
+			      match_name);
+	while ((per_cu = dw2_symtab_iter_next (&iter)) != NULL)
+	  dw2_expand_symtabs_matching_one (per_cu, nullptr, nullptr);
+	return true;
+      });
+    }
+  else
+    {
+      /* We have -readnow: no .gdb_index, but no partial symtabs either.  So,
+	 proceed assuming all symtabs have been read in.  */
+    }
 
   for (compunit_symtab *cust : objfile->compunits ())
     {
diff --git a/gdb/testsuite/gdb.base/with-mf-inc.c b/gdb/testsuite/gdb.base/with-mf-inc.c
new file mode 100644
index 0000000000..491be7fba2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/with-mf-inc.c
@@ -0,0 +1,35 @@
+/* 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/>.  */
+
+/* Non-main part of with.c.  */
+
+int xxx1 = 123;
+
+struct S
+{
+  int a;
+  int b;
+  int c;
+};
+
+struct S g_s = {1, 2, 3};
+
+void
+inc ()
+{
+  g_s.a++;;
+}
diff --git a/gdb/testsuite/gdb.base/with-mf-main.c b/gdb/testsuite/gdb.base/with-mf-main.c
new file mode 100644
index 0000000000..b50d98f2f2
--- /dev/null
+++ b/gdb/testsuite/gdb.base/with-mf-main.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/>.  */
+
+/* Main part of with.c.  */
+
+extern void inc (void);
+
+int
+main ()
+{
+  inc ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/with-mf.exp b/gdb/testsuite/gdb.base/with-mf.exp
new file mode 100644
index 0000000000..6b3663c570
--- /dev/null
+++ b/gdb/testsuite/gdb.base/with-mf.exp
@@ -0,0 +1,34 @@
+# 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/>.
+
+# Test .gdb_index in ada language mode.
+
+standard_testfile with-mf-main.c with-mf-inc.c
+
+if {[prepare_for_testing "failed to prepare" $testfile "$srcfile $srcfile2" \
+	 debug]} {
+    return -1
+}
+
+if { [ensure_gdb_index $binfile] == -1 } {
+    return -1
+}
+
+clean_restart $binfile
+
+gdb_test "with language ada -- print g_s" \
+    " = \\(a => 1, b => 2, c => 3\\)"

Re: [PATCH][gdb/symtab] Handle .gdb_index in ada language mode

[Tom Tromey]

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

Tom> Fix the inconsistency by completing implementation of
Tom> dw2_map_matching_symbols.

Thanks for doing this.

Tom> Tested on x86_64-linux, both with native and target board
Tom> cc-with-debug-index.

Tom> Any comments?

This may be the only barrier to supporting Ada in .gdb_index.
So, it seems like the Ada check in dwarf2/index-write.c should probably
be removed as well...?

I'm not 100% sure whether it will work or not.
If you'd rather not try, that's fine by me.  I think this patch looks
ok.

Tom

Re: [PATCH][gdb/symtab] Handle .gdb_index in ada language mode

[Tom de Vries]

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

On 19-05-2020 22:32, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Fix the inconsistency by completing implementation of
> Tom> dw2_map_matching_symbols.
> 
> Thanks for doing this.
> 
> Tom> Tested on x86_64-linux, both with native and target board
> Tom> cc-with-debug-index.
> 
> Tom> Any comments?
> 
> This may be the only barrier to supporting Ada in .gdb_index.
> So, it seems like the Ada check in dwarf2/index-write.c should probably
> be removed as well...?
> 
> I'm not 100% sure whether it will work or not.

Using this patch, I managed to get it working.

The patch does the following:
- enable ada .gdb_index by removing the ada check in write_psymbols
- copy some ada-specific code from debug_names::insert to
  write_psymbols
- disable a workaround for gold/15646 in dw2_expand_marked_cus

As for the disabled workaround, I ran into trouble in
gdb.ada/access_tagged_param.exp, where setting a breakpoint on foo failed.

The index shows that there are two entries for foo, one variable, one
function:
...
[3733] foo:
        3 [global, variable]
        5 [global, function]
...
The workaround skips the function, so disabling the workaround allows
the test to pass (my guess atm is that the workaround is not precise
enough).

FWIW, if I use .debug_names instead, I see the same pattern:
...
[445] #0b887389 foo:
        <6> DW_TAG_subprogram DW_IDX_compile_unit=5 DW_IDX_GNU_external=1
        <2> DW_TAG_variable DW_IDX_compile_unit=3 DW_IDX_GNU_external=1
...

Thanks,
- Tom

[-- Attachment #2: 0002-try.patch --]
[-- Type: text/x-patch, Size: 3472 bytes --]

try

---
 gdb/dwarf2/index-write.c | 34 ++++++++++++++++++++++++++++++----
 gdb/dwarf2/read.c        | 12 ------------
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index eabfe5d682..3e83e3ae2c 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -543,18 +543,44 @@ write_psymbols (struct mapped_symtab *symtab,
   for (; count-- > 0; ++psymp)
     {
       struct partial_symbol *psym = *psymp;
+      const char *name = psym->ginfo.search_name ();
 
       if (psym->ginfo.language () == language_ada)
-	error (_("Ada is not currently supported by the index; "
-		 "use the DWARF 5 index instead"));
+	{
+	  /* We want to ensure that the Ada main function's name appears
+	     verbatim in the index.  However, this name will be of the
+	     form "_ada_mumble", and will be rewritten by ada_decode.
+	     So, recognize it specially here and add it to the index by
+	     hand.  */
+	  if (strcmp (main_name (), name) == 0)
+	    {
+	      gdb_index_symbol_kind kind = symbol_kind (psym);
+
+	      add_index_entry (symtab, name, is_static, kind, cu_index);
+	    }
+
+	  /* In order for the index to work when read back into gdb, it
+	     has to supply a funny form of the name: it should be the
+	     encoded name, with any suffixes stripped.  Using the
+	     ordinary encoded name will not work properly with the
+	     searching logic in find_name_components_bounds; nor will
+	     using the decoded name.  Furthermore, an Ada "verbatim"
+	     name (of the form "<MumBle>") must be entered without the
+	     angle brackets.  Note that the current index is unusual,
+	     see PR symtab/24820 for details.  */
+	  std::string decoded = ada_decode (name);
+	  if (decoded[0] == '<')
+	    name = (char *) strndup (decoded.c_str () + 1, decoded.length () - 2);
+	  else
+	    name = strdup (ada_encode (decoded.c_str ()));
+	}
 
       /* Only add a given psymbol once.  */
       if (psyms_seen.insert (psym).second)
 	{
 	  gdb_index_symbol_kind kind = symbol_kind (psym);
 
-	  add_index_entry (symtab, psym->ginfo.search_name (),
-			   is_static, kind, cu_index);
+	  add_index_entry (symtab, name, is_static, kind, cu_index);
 	}
     }
 }
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ded71f53b5..d25e8ecb8c 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -4514,7 +4514,6 @@ dw2_expand_marked_cus
    search_domain kind)
 {
   offset_type *vec, vec_len, vec_idx;
-  bool global_seen = false;
   mapped_index &index = *dwarf2_per_objfile->index_table;
 
   vec = (offset_type *) (index.constant_pool
@@ -4523,8 +4522,6 @@ dw2_expand_marked_cus
   for (vec_idx = 0; vec_idx < vec_len; ++vec_idx)
     {
       offset_type cu_index_and_attrs = MAYBE_SWAP (vec[vec_idx + 1]);
-      /* This value is only valid for index versions >= 7.  */
-      int is_static = GDB_INDEX_SYMBOL_STATIC_VALUE (cu_index_and_attrs);
       gdb_index_symbol_kind symbol_kind =
 	GDB_INDEX_SYMBOL_KIND_VALUE (cu_index_and_attrs);
       int cu_index = GDB_INDEX_CU_VALUE (cu_index_and_attrs);
@@ -4536,15 +4533,6 @@ dw2_expand_marked_cus
 	(index.version >= 7
 	 && symbol_kind != GDB_INDEX_SYMBOL_KIND_NONE);
 
-      /* Work around gold/15646.  */
-      if (attrs_valid)
-	{
-	  if (!is_static && global_seen)
-	    continue;
-	  if (!is_static)
-	    global_seen = true;
-	}
-
       /* Only check the symbol's kind if it has one.  */
       if (attrs_valid)
 	{

Re: [PATCH][gdb/symtab] Handle .gdb_index in ada language mode

[Tom Tromey]

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

Tom> Using this patch, I managed to get it working.

Wow, cool.

Tom> The patch does the following:
Tom> - enable ada .gdb_index by removing the ada check in write_psymbols
Tom> - copy some ada-specific code from debug_names::insert to
Tom>   write_psymbols
Tom> - disable a workaround for gold/15646 in dw2_expand_marked_cus

Tom> As for the disabled workaround, I ran into trouble in
Tom> gdb.ada/access_tagged_param.exp, where setting a breakpoint on foo failed.

Tom> The index shows that there are two entries for foo, one variable, one
Tom> function:
Tom> ...
Tom> [3733] foo:
Tom>         3 [global, variable]
Tom>         5 [global, function]
Tom> ...
Tom> The workaround skips the function, so disabling the workaround allows
Tom> the test to pass (my guess atm is that the workaround is not precise
Tom> enough).

Looking at the gold bug, I suspect what's needed is to rule out
duplicates that have the same kind.

I tend to think you should push your previous patch, and maybe we can
revisit this stuff later.

Tom> +	  std::string decoded = ada_decode (name);
Tom> +	  if (decoded[0] == '<')
Tom> +	    name = (char *) strndup (decoded.c_str () + 1, decoded.length () - 2);
Tom> +	  else
Tom> +	    name = strdup (ada_encode (decoded.c_str ()));
Tom> +	}

This leaks the string.

Tom

[PATCH][gdb/symtab] Make gold index workaround more precise

[Tom de Vries]

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

[ was: Re: [PATCH][gdb/symtab] Handle .gdb_index in ada language mode ]
On 22-05-2020 22:31, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Using this patch, I managed to get it working.
> 
> Wow, cool.
> 
> Tom> The patch does the following:
> Tom> - enable ada .gdb_index by removing the ada check in write_psymbols
> Tom> - copy some ada-specific code from debug_names::insert to
> Tom>   write_psymbols
> Tom> - disable a workaround for gold/15646 in dw2_expand_marked_cus
> 
> Tom> As for the disabled workaround, I ran into trouble in
> Tom> gdb.ada/access_tagged_param.exp, where setting a breakpoint on foo failed.
> 
> Tom> The index shows that there are two entries for foo, one variable, one
> Tom> function:
> Tom> ...
> Tom> [3733] foo:
> Tom>         3 [global, variable]
> Tom>         5 [global, function]
> Tom> ...
> Tom> The workaround skips the function, so disabling the workaround allows
> Tom> the test to pass (my guess atm is that the workaround is not precise
> Tom> enough).
> 
> Looking at the gold bug, I suspect what's needed is to rule out
> duplicates that have the same kind.
> 

I've:
- committed a target board gold-gdb-index
- committed a test-case for the workaround
  (gdb.base/gold-gdb-index.exp), and
- wrote a patch to make the duplicate check more precise (attached
  below).

Any comments?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-symtab-Make-gold-index-workaround-more-precise.patch --]
[-- Type: text/x-patch, Size: 3674 bytes --]

[gdb/symtab] Make gold index workaround more precise

There's a PR binutils/15646 - "gold-generated .gdb_index has duplicated
symbols that gdb-generated index doesn't", that causes gold to generate
duplicate symbols in the index.

F.i., a namespace N1 declared in a header file can be listed for two CUs that
include the header file:
...
[759] N1:
        2 [global type]
        3 [global type]
...

This causes a gdb performance problem: f.i. when attempting to set a
breakpoint on a non-existing function N1::misspelled, the symtab for both CUs
will be expanded.

Gdb contains a workaround for this, added in commit 8943b87476 "Work around
gold/15646", that skips duplicate global symbols in the index.

However, the workaround does not check for the symbol kind ("type" in the
example above).

Make the workaround more precise by taking symbol kind into account in the
check for duplicates.

Tested on x86_64-linux, with native and target board gold-gdb-index.

gdb/ChangeLog:

2020-05-26  Tom de Vries  <tdevries@suse.de>

	* dwarf2/read.c (NR_GDB_INDEX_SYMBOL_KINDS): Define.
	(struct dw2_symtab_iterator): Make seen_before a bool array.
	(dw2_symtab_iter_init): Update seen_before initialization.
	(dw2_symtab_iter_next, dw2_expand_marked_cus): Check for symbol_kind
	in duplicate check.

---
 gdb/dwarf2/read.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ec3844188e..4a93cad301 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -3344,6 +3344,8 @@ dw2_map_symtabs_matching_filename
   return false;
 }
 
+#define NR_GDB_INDEX_SYMBOL_KINDS (GDB_INDEX_SYMBOL_KIND_MASK + 1)
+
 /* Struct used to manage iterating over all CUs looking for a symbol.  */
 
 struct dw2_symtab_iterator
@@ -3366,7 +3368,7 @@ struct dw2_symtab_iterator
      If so we can ignore all further global instances.
      This is to work around gold/15646, inefficient gold-generated
      indices.  */
-  int global_seen;
+  bool global_seen[NR_GDB_INDEX_SYMBOL_KINDS];
 };
 
 /* Initialize the index symtab iterator ITER.  */
@@ -3382,7 +3384,8 @@ dw2_symtab_iter_init (struct dw2_symtab_iterator *iter,
   iter->block_index = block_index;
   iter->domain = domain;
   iter->next = 0;
-  iter->global_seen = 0;
+  for (unsigned i = 0; i < NR_GDB_INDEX_SYMBOL_KINDS; ++i)
+    iter->global_seen[i] = false;
 
   mapped_index *index = dwarf2_per_objfile->index_table.get ();
 
@@ -3448,10 +3451,10 @@ dw2_symtab_iter_next (struct dw2_symtab_iterator *iter)
 	    }
 
 	  /* Work around gold/15646.  */
-	  if (!is_static && iter->global_seen)
+	  if (!is_static && iter->global_seen[symbol_kind])
 	    continue;
 	  if (!is_static)
-	    iter->global_seen = 1;
+	    iter->global_seen[symbol_kind] = true;
 	}
 
       /* Only check the symbol's kind if it has one.  */
@@ -4514,9 +4517,12 @@ dw2_expand_marked_cus
    search_domain kind)
 {
   offset_type *vec, vec_len, vec_idx;
-  bool global_seen = false;
+  bool global_seen[NR_GDB_INDEX_SYMBOL_KINDS];
   mapped_index &index = *dwarf2_per_objfile->index_table;
 
+  for (unsigned i = 0; i < NR_GDB_INDEX_SYMBOL_KINDS; ++i)
+    global_seen[i] = false;
+
   vec = (offset_type *) (index.constant_pool
 			 + MAYBE_SWAP (index.symbol_table[idx].vec));
   vec_len = MAYBE_SWAP (vec[0]);
@@ -4539,10 +4545,10 @@ dw2_expand_marked_cus
       /* Work around gold/15646.  */
       if (attrs_valid)
 	{
-	  if (!is_static && global_seen)
+	  if (!is_static && global_seen[symbol_kind])
 	    continue;
 	  if (!is_static)
-	    global_seen = true;
+	    global_seen[symbol_kind] = true;
 	}
 
       /* Only check the symbol's kind if it has one.  */

Re: [PATCH][gdb/symtab] Make gold index workaround more precise

[Tom de Vries]

On 26-05-2020 15:01, Tom de Vries wrote:
> [ was: Re: [PATCH][gdb/symtab] Handle .gdb_index in ada language mode ]
> On 22-05-2020 22:31, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> Using this patch, I managed to get it working.
>>
>> Wow, cool.
>>
>> Tom> The patch does the following:
>> Tom> - enable ada .gdb_index by removing the ada check in write_psymbols
>> Tom> - copy some ada-specific code from debug_names::insert to
>> Tom>   write_psymbols
>> Tom> - disable a workaround for gold/15646 in dw2_expand_marked_cus
>>
>> Tom> As for the disabled workaround, I ran into trouble in
>> Tom> gdb.ada/access_tagged_param.exp, where setting a breakpoint on foo failed.
>>
>> Tom> The index shows that there are two entries for foo, one variable, one
>> Tom> function:
>> Tom> ...
>> Tom> [3733] foo:
>> Tom>         3 [global, variable]
>> Tom>         5 [global, function]
>> Tom> ...
>> Tom> The workaround skips the function, so disabling the workaround allows
>> Tom> the test to pass (my guess atm is that the workaround is not precise
>> Tom> enough).
>>
>> Looking at the gold bug, I suspect what's needed is to rule out
>> duplicates that have the same kind.
>>
> 
> I've:
> - committed a target board gold-gdb-index
> - committed a test-case for the workaround
>   (gdb.base/gold-gdb-index.exp), and
> - wrote a patch to make the duplicate check more precise (attached
>   below).
> 
> Any comments?

I tested this further in combination with the ada .gdb_index enabling,
and ran into failures in gdb.ada/bp_inlined_func.exp, which I managed to
fix by skipping the workaround when symbol_kind == function.

At this point, I wonder, should we just enable this workaround for
symbol_kind == type?

Thanks,
- Tom

Re: [PATCH][gdb/symtab] Make gold index workaround more precise

[Tom Tromey]

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

Tom> +  bool global_seen[NR_GDB_INDEX_SYMBOL_KINDS];

This can probably just use {} to initialize.

Tom> +  for (unsigned i = 0; i < NR_GDB_INDEX_SYMBOL_KINDS; ++i)
Tom> +    iter->global_seen[i] = false;

Then this wouldn't be needed.

Tom> +  bool global_seen[NR_GDB_INDEX_SYMBOL_KINDS];
Tom>    mapped_index &index = *dwarf2_per_objfile->index_table;
 
Tom> +  for (unsigned i = 0; i < NR_GDB_INDEX_SYMBOL_KINDS; ++i)
Tom> +    global_seen[i] = false;

Here too.

Tom

Re: [PATCH][gdb/symtab] Make gold index workaround more precise

[Tom Tromey]

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

Tom> I tested this further in combination with the ada .gdb_index enabling,
Tom> and ran into failures in gdb.ada/bp_inlined_func.exp, which I managed to
Tom> fix by skipping the workaround when symbol_kind == function.

Tom> At this point, I wonder, should we just enable this workaround for
Tom> symbol_kind == type?

I think that would make sense.

Tom

Re: [PATCH][gdb/symtab] Make gold index workaround more precise

[Tom de Vries]

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

On 27-05-2020 17:22, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> I tested this further in combination with the ada .gdb_index enabling,
> Tom> and ran into failures in gdb.ada/bp_inlined_func.exp, which I managed to
> Tom> fix by skipping the workaround when symbol_kind == function.
> 
> Tom> At this point, I wonder, should we just enable this workaround for
> Tom> symbol_kind == type?
> 
> I think that would make sense.
> 

I'll commit this patch then, once retesting has finished.

Thanks,
- Tom



[-- Attachment #2: 0002-gdb-symtab-Make-gold-index-workaround-more-precise.patch --]
[-- Type: text/x-patch, Size: 2279 bytes --]

[gdb/symtab] Make gold index workaround more precise

There's a PR gold/15646 - "gold-generated .gdb_index has duplicated
symbols that gdb-generated index doesn't", that causes gold to generate
duplicate symbols in the index.

F.i., a namespace N1 declared in a header file can be listed for two CUs that
include the header file:
...
[759] N1:
        2 [global type]
        3 [global type]
...

This causes a gdb performance problem: f.i. when attempting to set a
breakpoint on a non-existing function N1::misspelled, the symtab for both CUs
will be expanded.

Gdb contains a workaround for this, added in commit 8943b87476 "Work around
gold/15646", that skips duplicate global symbols in the index.

However, the workaround does not check for the symbol kind ("type" in the
example above).

Make the workaround more precise by limiting it to symbol kind "type".

Tested on x86_64-linux, with native and target board gold-gdb-index.

gdb/ChangeLog:

2020-05-26  Tom de Vries  <tdevries@suse.de>

	* dwarf2/read.c	(dw2_symtab_iter_next, dw2_expand_marked_cus): Limit
	PR gold/15646 workaround to symbol kind "type".

---
 gdb/dwarf2/read.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index a62224c0be..25f05fb993 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -3522,10 +3522,14 @@ dw2_symtab_iter_next (struct dw2_symtab_iterator *iter)
 	    }
 
 	  /* Work around gold/15646.  */
-	  if (!is_static && iter->global_seen)
-	    continue;
-	  if (!is_static)
-	    iter->global_seen = 1;
+	  if (!is_static
+	      && symbol_kind == GDB_INDEX_SYMBOL_KIND_TYPE)
+	    {
+	      if (iter->global_seen)
+		continue;
+
+	      iter->global_seen = 1;
+	    }
 	}
 
       /* Only check the symbol's kind if it has one.  */
@@ -4627,12 +4631,14 @@ dw2_expand_marked_cus
 	 && symbol_kind != GDB_INDEX_SYMBOL_KIND_NONE);
 
       /* Work around gold/15646.  */
-      if (attrs_valid)
+      if (attrs_valid
+	  && !is_static
+	  && symbol_kind == GDB_INDEX_SYMBOL_KIND_TYPE)
 	{
-	  if (!is_static && global_seen)
+	  if (global_seen)
 	    continue;
-	  if (!is_static)
-	    global_seen = true;
+
+	  global_seen = true;
 	}
 
       /* Only check the symbol's kind if it has one.  */

Re: [PATCH][gdb/symtab] Handle .gdb_index in ada language mode

[Tom de Vries]

On 22-05-2020 22:31, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Using this patch, I managed to get it working.
> 
> Wow, cool.
> 
> Tom> The patch does the following:
> Tom> - enable ada .gdb_index by removing the ada check in write_psymbols
> Tom> - copy some ada-specific code from debug_names::insert to
> Tom>   write_psymbols
> Tom> - disable a workaround for gold/15646 in dw2_expand_marked_cus
> 
> Tom> As for the disabled workaround, I ran into trouble in
> Tom> gdb.ada/access_tagged_param.exp, where setting a breakpoint on foo failed.
> 
> Tom> The index shows that there are two entries for foo, one variable, one
> Tom> function:
> Tom> ...
> Tom> [3733] foo:
> Tom>         3 [global, variable]
> Tom>         5 [global, function]
> Tom> ...
> Tom> The workaround skips the function, so disabling the workaround allows
> Tom> the test to pass (my guess atm is that the workaround is not precise
> Tom> enough).
> 
> Looking at the gold bug, I suspect what's needed is to rule out
> duplicates that have the same kind.
> 

The workaround has been limited:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=f030440daa989ae3dadc1fa4342cfa16d690db3c
.

> I tend to think you should push your previous patch, and maybe we can
> revisit this stuff later.
> 
> Tom> +	  std::string decoded = ada_decode (name);
> Tom> +	  if (decoded[0] == '<')
> Tom> +	    name = (char *) strndup (decoded.c_str () + 1, decoded.length () - 2);
> Tom> +	  else
> Tom> +	    name = strdup (ada_encode (decoded.c_str ()));
> Tom> +	}
> 
> This leaks the string.

Fixed in a new version of the patch, submitted as part of patch series:
- "[PATCH, 1/2][gdb/symtab] Fix name lookup in dw2_map_matching_symbols"
  https://sourceware.org/pipermail/gdb-patches/2020-June/169213.html
- "[PATCH][gdb/symtab] Enable ada .gdb_index"
  https://sourceware.org/pipermail/gdb-patches/2020-June/169214.html

[ Note the missing ", 2/2" in subject of second patch. ]

Thanks,
- Tom

Re: [PATCH][gdb/symtab] Handle .gdb_index in ada language mode

[Tom de Vries]

On 02-06-2020 12:47, Tom de Vries wrote:
> On 22-05-2020 22:31, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>
>> Tom> Using this patch, I managed to get it working.
>>
>> Wow, cool.
>>
>> Tom> The patch does the following:
>> Tom> - enable ada .gdb_index by removing the ada check in write_psymbols
>> Tom> - copy some ada-specific code from debug_names::insert to
>> Tom>   write_psymbols
>> Tom> - disable a workaround for gold/15646 in dw2_expand_marked_cus
>>
>> Tom> As for the disabled workaround, I ran into trouble in
>> Tom> gdb.ada/access_tagged_param.exp, where setting a breakpoint on foo failed.
>>
>> Tom> The index shows that there are two entries for foo, one variable, one
>> Tom> function:
>> Tom> ...
>> Tom> [3733] foo:
>> Tom>         3 [global, variable]
>> Tom>         5 [global, function]
>> Tom> ...
>> Tom> The workaround skips the function, so disabling the workaround allows
>> Tom> the test to pass (my guess atm is that the workaround is not precise
>> Tom> enough).
>>
>> Looking at the gold bug, I suspect what's needed is to rule out
>> duplicates that have the same kind.
>>
> 
> The workaround has been limited:
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=f030440daa989ae3dadc1fa4342cfa16d690db3c
> .
> 
>> I tend to think you should push your previous patch, and maybe we can
>> revisit this stuff later.
>>
>> Tom> +	  std::string decoded = ada_decode (name);
>> Tom> +	  if (decoded[0] == '<')
>> Tom> +	    name = (char *) strndup (decoded.c_str () + 1, decoded.length () - 2);
>> Tom> +	  else
>> Tom> +	    name = strdup (ada_encode (decoded.c_str ()));
>> Tom> +	}
>>
>> This leaks the string.
> 
> Fixed in a new version of the patch, submitted as part of patch series:
> - "[PATCH, 1/2][gdb/symtab] Fix name lookup in dw2_map_matching_symbols"
>   https://sourceware.org/pipermail/gdb-patches/2020-June/169213.html
> - "[PATCH][gdb/symtab] Enable ada .gdb_index"
>   https://sourceware.org/pipermail/gdb-patches/2020-June/169214.html
> 
> [ Note the missing ", 2/2" in subject of second patch. ]

I've committed these two patches.

Thanks,
- Tom