diff mbox series

Use "::" as separator for Fortran in cooked index

Message ID 20250305003149.827350-1-tom@tromey.com
State New
Headers
Series Use "::" as separator for Fortran in cooked index |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed

Commit Message

Tom Tromey March 5, 2025, 12:31 a.m. UTC 
  This teaches cooked_index_entry::full_name that "::" is the separator
for Fortran.  I don't know enough Fortran to write a test case for
this.  However, a different series I am working on has a regression if
this patch is not applied.
---
 gdb/dwarf2/cooked-index.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Simon Marchi March 5, 2025, 3:44 p.m. UTC | #1 
On 3/4/25 7:31 PM, Tom Tromey wrote:
> This teaches cooked_index_entry::full_name that "::" is the separator
> for Fortran.  I don't know enough Fortran to write a test case for
> this.  However, a different series I am working on has a regression if
> this patch is not applied.
> ---
>  gdb/dwarf2/cooked-index.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
> index 21d9dab5f41..bbe14adcd56 100644
> --- a/gdb/dwarf2/cooked-index.c
> +++ b/gdb/dwarf2/cooked-index.c
> @@ -234,6 +234,7 @@ cooked_index_entry::full_name (struct obstack *storage, bool for_main,
>      {
>      case language_cplus:
>      case language_rust:
> +    case language_fortran:
>        sep = "::";
>        break;
>  
> -- 
> 2.46.1
> 

What does this ultimately impact?  Do you remember what the failure
looked like?

Simon
Tom Tromey March 5, 2025, 4:58 p.m. UTC | #2 
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> This teaches cooked_index_entry::full_name that "::" is the separator
>> for Fortran.  I don't know enough Fortran to write a test case for
>> this.  However, a different series I am working on has a regression if
>> this patch is not applied.

Simon> What does this ultimately impact?  Do you remember what the
Simon> failure looked like?

Not really but read on.

The bigger series changes symbol lookup so that gdb no longer uses the
idiom of "search existing compunits, then expand some units and look at
those".  Instead, all searches are done via expand_symtabs_matching,
with the final decision being made in the callback.

This is nice because it means that expanded symtabs that aren't useful
to the current search are never examined.  Also means that searches are
independent of CU expansion order.

However the drawback is that this exposes all the places where the index
and the full reader disagree.  And it turns out there's a number of
these.  With the patches these manifest as test failures because the
indexer no longer finds a symbol, so the searcher never sees the CU.

This was one of them, though I don't recall exactly which Fortran test
regressed.  I could find out if it's important.  Or just leave this
patch until I send the full series.  I was just trying to pre-send the
relatively independent patches so that the final series doesn't arrive
in a huge chunk.

Note that most of these "disagreement" bugs are probably already
reproducible if you set up a more complicated test involving multiple
CUs.

Tom
Simon Marchi March 5, 2025, 5:16 p.m. UTC | #3 
On 3/5/25 11:58 AM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
>>> This teaches cooked_index_entry::full_name that "::" is the separator
>>> for Fortran.  I don't know enough Fortran to write a test case for
>>> this.  However, a different series I am working on has a regression if
>>> this patch is not applied.
> 
> Simon> What does this ultimately impact?  Do you remember what the
> Simon> failure looked like?
> 
> Not really but read on.
> 
> The bigger series changes symbol lookup so that gdb no longer uses the
> idiom of "search existing compunits, then expand some units and look at
> those".  Instead, all searches are done via expand_symtabs_matching,
> with the final decision being made in the callback.
> 
> This is nice because it means that expanded symtabs that aren't useful
> to the current search are never examined.  Also means that searches are
> independent of CU expansion order.
> 
> However the drawback is that this exposes all the places where the index
> and the full reader disagree.  And it turns out there's a number of
> these.  With the patches these manifest as test failures because the
> indexer no longer finds a symbol, so the searcher never sees the CU.
> 
> This was one of them, though I don't recall exactly which Fortran test
> regressed.  I could find out if it's important.  Or just leave this
> patch until I send the full series.  I was just trying to pre-send the
> relatively independent patches so that the final series doesn't arrive
> in a huge chunk.
> 
> Note that most of these "disagreement" bugs are probably already
> reproducible if you set up a more complicated test involving multiple
> CUs.

Thanks for the explanation.  I'll just rehash it to make sure I
understand.  With that change, when you'll do a name lookup, we'll ask
the index "which CUs can potentially have a hit", expand the matched CUs
that aren't expanded already, and then search all matched CUs.  The
index will therefore act as the first filter of CUs to be searched.
Does that sound right?  If so, it does indeed sound nice to reduce the
"it works if the right CU is already expanded but doesn't work if not"
bugs.

I don't have any reason not to like this particular patch, so:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon
Tom Tromey March 5, 2025, 5:26 p.m. UTC | #4 
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> With that change, when you'll do a name lookup, we'll ask
Simon> the index "which CUs can potentially have a hit", expand the matched CUs
Simon> that aren't expanded already, and then search all matched CUs.  The
Simon> index will therefore act as the first filter of CUs to be searched.
Simon> Does that sound right?

Yes.

Tom
diff mbox series

Patch

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index 21d9dab5f41..bbe14adcd56 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -234,6 +234,7 @@  cooked_index_entry::full_name (struct obstack *storage, bool for_main,
     {
     case language_cplus:
     case language_rust:
+    case language_fortran:
       sep = "::";
       break;