[PATCH] [gdb/symtab] Handle PU in iterate_over_some_symtabs

[PATCH] [gdb/symtab] Handle PU in iterate_over_some_symtabs

[Tom de Vries]

When running test-case gdb.base/setshow.exp with target board cc-with-dwz I
run into:
...
(gdb) info line 1^M
Line 1 of "setshow.c" is at address 0x400527 <main> but contains no code.^M
Line 1 of "setshow.c" is at address 0x400527 <main> but contains no code.^M
(gdb) FAIL: gdb.base/setshow.exp: test_setshow_annotate: annotation_level 1
...
while the expected output is:
...
Line 1 of "setshow.c" is at address 0x400527 <main> but contains no code.
��setshow.c:1:0:beg:0x400527
...

The second line of the expected output is missing due to the first line of the
expected output being repeated, so the problem is that the "Line 1" line is
printed twice.

This happens because the PU imported by the CU reuses the filetab of the CU,
and both the CU and PU are visited by iterate_over_some_symtabs.

Fix this by skipping PUs in iterate_over_some_symtabs.

Tested on x86_64-linux, target boards unix, cc-with-dwz and cc-with-dwz-m.

PR symtab/30797
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30797
---
 gdb/symtab.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 0117a2a59d7..19d33020b87 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -565,6 +565,10 @@ iterate_over_some_symtabs (const char *name,
 
   for (cust = first; cust != NULL && cust != after_last; cust = cust->next)
     {
+      /* Skip included compunits.  */
+      if (cust->user != nullptr)
+	continue;
+
       for (symtab *s : cust->filetabs ())
 	{
 	  if (compare_filenames_for_search (s->filename, name))

base-commit: 959db212304e64751563bcaaacbdd28efd5016a7
-- 
2.35.3

Re: [PATCH] [gdb/symtab] Handle PU in iterate_over_some_symtabs

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> This happens because the PU imported by the CU reuses the filetab of the CU,
Tom> and both the CU and PU are visited by iterate_over_some_symtabs.

Tom> Fix this by skipping PUs in iterate_over_some_symtabs.

Looks good to me.
Approved-By: Tom Tromey <tom@tromey.com>

I wonder what would happen if we simply left included CUs off the linked list.
Is it ever worthwhile to search them?

Tom

Re: [PATCH] [gdb/symtab] Handle PU in iterate_over_some_symtabs

[Tom de Vries]

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

On 9/5/23 18:45, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> This happens because the PU imported by the CU reuses the filetab of the CU,
> Tom> and both the CU and PU are visited by iterate_over_some_symtabs.
> 
> Tom> Fix this by skipping PUs in iterate_over_some_symtabs.
> 
> Looks good to me.
> Approved-By: Tom Tromey <tom@tromey.com>
> 

Thanks for the review.

> I wonder what would happen if we simply left included CUs off the linked list.
> Is it ever worthwhile to search them?

I gave that a try (WIP patch attached).

That fixes this PR, and interestingly, it fixes another PR for which I 
have a similar, skip-PU patch pending ( 
https://sourceware.org/pipermail/gdb-patches/2023-August/201905.html ).

I've tested it with the cc-with-dwz target board and ran into a 
regression though:
...
FAIL: gdb.cp/m-static.exp: static const int initialized in class definition
FAIL: gdb.cp/m-static.exp: static const float initialized in class 
definition
...

Perhaps somewhere we assume that the PU is part off the linked list and 
we skip searching in the includes.

Thanks,
- Tom

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

From 8ea0a2072a19abbcdd04b2fcaf552f1dcca52d14 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Wed, 6 Sep 2023 08:50:20 +0200
Subject: [PATCH] try

---
 gdb/dwarf2/read.c |  7 ++++++-
 gdb/symfile.c     | 24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 98bedbc5d49..bc75639ad5d 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1775,6 +1775,8 @@ dw2_do_instantiate_symtab (dwarf2_per_cu_data *per_cu,
   per_objfile->age_comp_units ();
 }
 
+extern void remove_compunit_symtab_from_objfile (struct compunit_symtab *cu);
+
 /* Ensure that the symbols for PER_CU have been read in.  DWARF2_PER_OBJFILE is
    the per-objfile for which this symtab is instantiated.
 
@@ -6308,7 +6310,10 @@ recursively_compute_inclusions (std::vector<compunit_symtab *> *result,
 	{
 	  result->push_back (cust);
 	  if (cust->user == NULL)
-	    cust->user = immediate_parent;
+	    {
+	      cust->user = immediate_parent;
+	      remove_compunit_symtab_from_objfile (cust);
+	    }
 	}
     }
 
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 1b46ec45f2e..f67c4b7c0b6 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2895,6 +2895,30 @@ add_compunit_symtab_to_objfile (struct compunit_symtab *cu)
   cu->next = cu->objfile ()->compunit_symtabs;
   cu->objfile ()->compunit_symtabs = cu;
 }
+
+extern void remove_compunit_symtab_from_objfile (struct compunit_symtab *cu);
+
+void
+remove_compunit_symtab_from_objfile (struct compunit_symtab *cu)
+{
+  struct compunit_symtab *item, *prev_item;
+
+  prev_item = nullptr;
+  item = cu->objfile ()->compunit_symtabs;
+  while (item != cu && item != nullptr)
+    {
+      prev_item = item;
+      item = item->next;
+    }
+  gdb_assert (item != nullptr);
+
+  if (prev_item == nullptr)
+    cu->objfile ()->compunit_symtabs = item->next;
+  else
+    prev_item->next = item->next;
+
+  item->next = nullptr;
+}
 \f
 
 /* Reset all data structures in gdb which may contain references to

base-commit: a13e4c5c10d1a13d9128d033c9525810e876ac14
-- 
2.35.3

Re: [PATCH] [gdb/symtab] Handle PU in iterate_over_some_symtabs

[Tom de Vries]

On 9/6/23 09:31, Tom de Vries wrote:
> On 9/5/23 18:45, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries via Gdb-patches 
>>>>>>> <gdb-patches@sourceware.org> writes:
>>
>> Tom> This happens because the PU imported by the CU reuses the filetab 
>> of the CU,
>> Tom> and both the CU and PU are visited by iterate_over_some_symtabs.
>>
>> Tom> Fix this by skipping PUs in iterate_over_some_symtabs.
>>
>> Looks good to me.
>> Approved-By: Tom Tromey <tom@tromey.com>
>>
> 
> Thanks for the review.
> 
>> I wonder what would happen if we simply left included CUs off the 
>> linked list.
>> Is it ever worthwhile to search them?
> 
> I gave that a try (WIP patch attached).
> 
> That fixes this PR, and interestingly, it fixes another PR for which I 
> have a similar, skip-PU patch pending ( 
> https://sourceware.org/pipermail/gdb-patches/2023-August/201905.html ).
> 
> I've tested it with the cc-with-dwz target board and ran into a 
> regression though:
> ...
> FAIL: gdb.cp/m-static.exp: static const int initialized in class definition
> FAIL: gdb.cp/m-static.exp: static const float initialized in class 
> definition
> ...
> 
> Perhaps somewhere we assume that the PU is part off the linked list and 
> we skip searching in the includes.

I've looked into this.

For compunit_symtab::includes we have:
...
   /* If non-NULL, then this points to a NULL-terminated vector of 

      included compunits.  When searching the static or global 

      block of this compunit, the corresponding block of all 

      included compunits will also be searched.  Note that this 

      list must be flattened -- the symbol reader is responsible for 

      ensuring that this vector contains the transitive closure of all 

      included compunits.  */
   struct compunit_symtab **includes;
...

In lookup_symbol_in_objfile_symtabs we have a loop over the linked list:
...
  for (compunit_symtab *cust : objfile->compunits ())
...
but there's no provision to visit the includes.

Thanks,
- Tom

Re: [PATCH] [gdb/symtab] Handle PU in iterate_over_some_symtabs

[Tom de Vries]

On 9/6/23 11:04, Tom de Vries wrote:
> On 9/6/23 09:31, Tom de Vries wrote:
>> On 9/5/23 18:45, Tom Tromey wrote:
>>>>>>>> "Tom" == Tom de Vries via Gdb-patches 
>>>>>>>> <gdb-patches@sourceware.org> writes:
>>>
>>> Tom> This happens because the PU imported by the CU reuses the 
>>> filetab of the CU,
>>> Tom> and both the CU and PU are visited by iterate_over_some_symtabs.
>>>
>>> Tom> Fix this by skipping PUs in iterate_over_some_symtabs.
>>>
>>> Looks good to me.
>>> Approved-By: Tom Tromey <tom@tromey.com>
>>>
>>
>> Thanks for the review.
>>
>>> I wonder what would happen if we simply left included CUs off the 
>>> linked list.
>>> Is it ever worthwhile to search them?
>>
>> I gave that a try (WIP patch attached).
>>
>> That fixes this PR, and interestingly, it fixes another PR for which I 
>> have a similar, skip-PU patch pending ( 
>> https://sourceware.org/pipermail/gdb-patches/2023-August/201905.html ).
>>
>> I've tested it with the cc-with-dwz target board and ran into a 
>> regression though:
>> ...
>> FAIL: gdb.cp/m-static.exp: static const int initialized in class 
>> definition
>> FAIL: gdb.cp/m-static.exp: static const float initialized in class 
>> definition
>> ...
>>
>> Perhaps somewhere we assume that the PU is part off the linked list 
>> and we skip searching in the includes.
> 
> I've looked into this.
> 
> For compunit_symtab::includes we have:
> ...
>    /* If non-NULL, then this points to a NULL-terminated vector of
>       included compunits.  When searching the static or global
>       block of this compunit, the corresponding block of all
>       included compunits will also be searched.  Note that this
>       list must be flattened -- the symbol reader is responsible for
>       ensuring that this vector contains the transitive closure of all
>       included compunits.  */
>    struct compunit_symtab **includes;
> ...
> 
> In lookup_symbol_in_objfile_symtabs we have a loop over the linked list:
> ...
>   for (compunit_symtab *cust : objfile->compunits ())
> ...
> but there's no provision to visit the includes.
> 

I had some further thought, and submitted a review PR ( 
https://sourceware.org/bugzilla/show_bug.cgi?id=30826 ).

Thanks,
- Tom