[Bug rust/30116] New: "start" fails for Rust with new DWARF indexer

[Bug rust/30116] New: "start" fails for Rust with new DWARF indexer

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30116

            Bug ID: 30116
           Summary: "start" fails for Rust with new DWARF indexer
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: rust
          Assignee: unassigned at sourceware dot org
          Reporter: tromey at sourceware dot org
  Target Milestone: ---

On irc, Iain Buclaw pointed out that the new DWARF indexer
broke "start" for D.  This is easily fixed, but more testing
showed that it also broke Rust.

It's not entirely trivial to fix.  The issue is that in
dwarf2_build_psymtabs_hard:

  const cooked_index_entry *main_entry = vec->get_main ();
  if (main_entry != nullptr)
    set_objfile_main_name (objfile, main_entry->name,
                           main_entry->per_cu->lang ());

... here we'd like to use the entry's full name.
But, the entry hasn't been canonicalized yet, so doing this
would actually crash.

Note though that Rust doesn't require canonicalization.
And, the languages that do require canonicalization don't
need this mechanism for finding "main".
So, I think at least for now we could have a hack to handle this.

Longer term, perhaps resurrecting an old patch of mine to
make "main"-finding more lazy (lookup on first use) would be
good.  This way the index could be "clean" and wait for the
finalization to be done, while not pausing the UI in the normal
case... the problem with the current code being that for
"gdb exe", gdb eagerly wants to set the current language, but
really this could be done on demand.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug rust/30116] "start" fails for Rust with new DWARF indexer

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30116

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |29366


Referenced Bugs:

https://sourceware.org/bugzilla/show_bug.cgi?id=29366
[Bug 29366] [meta] New DWARF indexer meta bug
-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug rust/30116] "start" fails for Rust with new DWARF indexer

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30116

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at sourceware dot org   |tromey at sourceware dot org

--- Comment #1 from Tom Tromey <tromey at sourceware dot org> ---
I have a patch for this, but haven't written a 'start' test for D yet.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug rust/30116] "start" fails for Rust with new DWARF indexer

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30116

--- Comment #2 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=47fe57c92810c7302bb80eafdec6f4345bcc69c8

commit 47fe57c92810c7302bb80eafdec6f4345bcc69c8
Author: Tom Tromey <tom@tromey.com>
Date:   Mon Feb 13 17:44:54 2023 -0700

    Fix "start" for D, Rust, etc

    The new DWARF indexer broke "start" for some languages.

    For D, it is broken because, while the code in cooked_index_shard::add
    specifically excludes Ada, it fails to exclude D.  This means that the
    C "main" will be detected as "main" here -- whereas what is intended
    is for the code in find_main_name to use d_main_name to find the name.

    The Rust compiler, on the other hand, uses DW_AT_main_subprogram.
    However, the code in dwarf2_build_psymtabs_hard fails to create a
    fully-qualified name, so the name always ends up as plain "main".

    For D and Ada, a very simple approach suffices: remove the check
    against "main" from cooked_index_shard::add.  This also has the
    benefit of slightly speeding up DWARF indexing.  I assume this
    approach will work for Pascal and Modula-2 as well, but I don't have a
    way to test those at present.

    For Rust, though, this is not sufficient.  And, computing the
    fully-qualified name in dwarf2_build_psymtabs_hard will crash, because
    cooked_index_entry::full_name uses the canonical name -- and that is
    not computed until after canonicalization.

    However, we don't want to wait for canonicalization to be done before
    computing the main name.  That would remove any benefit from doing
    canonicalization is the background.

    This patch solves this dilemma by noticing that languages using
    DW_AT_main_subprogram are, currently, disjoint from languages
    requiring canonicalization.  Because of this, we can add a parameter
    to full_name to let us avoid crashes, slowdowns, and races here.

    This is kind of tricky and ugly, so I've tried to comment it
    sufficiently.

    While doing this, I had to change gdb.dwarf2/main-subprogram.exp.  A
    different possibility here would be to ignore the canonicalization
    needs of C in this situation, because those only affect certain types.
    However, I chose this approach because the test case is artificial
    anyhow.

    A long time ago, in an earlier threading attempt, I changed the global
    current_language to be a function (hidden behind a macro) to let us
    attempt lazily computing the current language.  Perhaps this approach
    could still be made to work.  However, that also seemed rather tricky,
    more so than this patch.

    Reviewed-By: Andrew Burgess <aburgess@redhat.com>
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30116

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug rust/30116] "start" fails for Rust with new DWARF indexer

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30116

--- Comment #3 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The gdb-13-branch branch has been updated by Tom Tromey
<tromey@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=7f4307436fdab42da2b385040b90294f301ea55b

commit 7f4307436fdab42da2b385040b90294f301ea55b
Author: Tom Tromey <tom@tromey.com>
Date:   Mon Feb 13 17:44:54 2023 -0700

    Fix "start" for D, Rust, etc

    The new DWARF indexer broke "start" for some languages.

    For D, it is broken because, while the code in cooked_index_shard::add
    specifically excludes Ada, it fails to exclude D.  This means that the
    C "main" will be detected as "main" here -- whereas what is intended
    is for the code in find_main_name to use d_main_name to find the name.

    The Rust compiler, on the other hand, uses DW_AT_main_subprogram.
    However, the code in dwarf2_build_psymtabs_hard fails to create a
    fully-qualified name, so the name always ends up as plain "main".

    For D and Ada, a very simple approach suffices: remove the check
    against "main" from cooked_index_shard::add.  This also has the
    benefit of slightly speeding up DWARF indexing.  I assume this
    approach will work for Pascal and Modula-2 as well, but I don't have a
    way to test those at present.

    For Rust, though, this is not sufficient.  And, computing the
    fully-qualified name in dwarf2_build_psymtabs_hard will crash, because
    cooked_index_entry::full_name uses the canonical name -- and that is
    not computed until after canonicalization.

    However, we don't want to wait for canonicalization to be done before
    computing the main name.  That would remove any benefit from doing
    canonicalization is the background.

    This patch solves this dilemma by noticing that languages using
    DW_AT_main_subprogram are, currently, disjoint from languages
    requiring canonicalization.  Because of this, we can add a parameter
    to full_name to let us avoid crashes, slowdowns, and races here.

    This is kind of tricky and ugly, so I've tried to comment it
    sufficiently.

    While doing this, I had to change gdb.dwarf2/main-subprogram.exp.  A
    different possibility here would be to ignore the canonicalization
    needs of C in this situation, because those only affect certain types.
    However, I chose this approach because the test case is artificial
    anyhow.

    A long time ago, in an earlier threading attempt, I changed the global
    current_language to be a function (hidden behind a macro) to let us
    attempt lazily computing the current language.  Perhaps this approach
    could still be made to work.  However, that also seemed rather tricky,
    more so than this patch.

    Reviewed-By: Andrew Burgess <aburgess@redhat.com>
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30116
    (cherry picked from commit 47fe57c92810c7302bb80eafdec6f4345bcc69c8)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug rust/30116] "start" fails for Rust with new DWARF indexer

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30116

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.1
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #4 from Tom Tromey <tromey at sourceware dot org> ---
Fixed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.