[gdb/symtab] Handle PU in iterate_over_some_symtabs

  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(+)


base-commit: 959db212304e64751563bcaaacbdd28efd5016a7
  

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

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
  

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
  

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