[PATCH] Fix "start" for D, Rust, etc

[PATCH] Fix "start" for D, Rust, etc

[Tom Tromey]

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.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30116
---
 gdb/dwarf2/cooked-index.c                    | 50 +++++++++++++-------
 gdb/dwarf2/cooked-index.h                    | 18 +++++--
 gdb/dwarf2/read.c                            | 13 ++++-
 gdb/testsuite/gdb.dlang/dlang-start.exp      | 38 +++++++++++++++
 gdb/testsuite/gdb.dlang/simple.d             | 17 +++++++
 gdb/testsuite/gdb.dwarf2/main-subprogram.exp |  3 +-
 gdb/testsuite/gdb.rust/rust-start.exp        | 38 +++++++++++++++
 7 files changed, 154 insertions(+), 23 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dlang/dlang-start.exp
 create mode 100644 gdb/testsuite/gdb.dlang/simple.d
 create mode 100644 gdb/testsuite/gdb.rust/rust-start.exp

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index 3d23a65ad8f..d465028add4 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -48,6 +48,16 @@ to_string (cooked_index_flag flags)
 
 /* See cooked-index.h.  */
 
+bool
+language_requires_canonicalization (enum language lang)
+{
+  return (lang == language_ada
+	  || lang == language_c
+	  || lang == language_cplus);
+}
+
+/* See cooked-index.h.  */
+
 int
 cooked_index_entry::compare (const char *stra, const char *strb,
 			     comparison_mode mode)
@@ -162,10 +172,12 @@ test_compare ()
 /* See cooked-index.h.  */
 
 const char *
-cooked_index_entry::full_name (struct obstack *storage) const
+cooked_index_entry::full_name (struct obstack *storage, bool for_main) const
 {
+  const char *local_name = for_main ? name : canonical;
+
   if ((flags & IS_LINKAGE) != 0 || parent_entry == nullptr)
-    return canonical;
+    return local_name;
 
   const char *sep = nullptr;
   switch (per_cu->lang ())
@@ -182,11 +194,11 @@ cooked_index_entry::full_name (struct obstack *storage) const
       break;
 
     default:
-      return canonical;
+      return local_name;
     }
 
-  parent_entry->write_scope (storage, sep);
-  obstack_grow0 (storage, canonical, strlen (canonical));
+  parent_entry->write_scope (storage, sep, for_main);
+  obstack_grow0 (storage, local_name, strlen (local_name));
   return (const char *) obstack_finish (storage);
 }
 
@@ -194,11 +206,13 @@ cooked_index_entry::full_name (struct obstack *storage) const
 
 void
 cooked_index_entry::write_scope (struct obstack *storage,
-				 const char *sep) const
+				 const char *sep,
+				 bool for_main) const
 {
   if (parent_entry != nullptr)
-    parent_entry->write_scope (storage, sep);
-  obstack_grow (storage, canonical, strlen (canonical));
+    parent_entry->write_scope (storage, sep, for_main);
+  const char *local_name = for_main ? name : canonical;
+  obstack_grow (storage, local_name, strlen (local_name));
   obstack_grow (storage, sep, strlen (sep));
 }
 
@@ -218,10 +232,6 @@ cooked_index_shard::add (sect_offset die_offset, enum dwarf_tag tag,
      implicit "main" discovery.  */
   if ((flags & IS_MAIN) != 0)
     m_main = result;
-  else if (per_cu->lang () != language_ada
-	   && m_main == nullptr
-	   && strcmp (name, "main") == 0)
-    m_main = result;
 
   return result;
 }
@@ -323,6 +333,8 @@ cooked_index_shard::do_finalize ()
 
   for (cooked_index_entry *entry : m_entries)
     {
+      /* Note that this code must be kept in sync with
+	 language_requires_canonicalization.  */
       gdb_assert (entry->canonical == nullptr);
       if ((entry->flags & IS_LINKAGE) != 0)
 	entry->canonical = entry->name;
@@ -474,11 +486,15 @@ cooked_index::get_main () const
   for (const auto &index : m_vector)
     {
       const cooked_index_entry *entry = index->get_main ();
-      if (result == nullptr
-	  || ((result->flags & IS_MAIN) == 0
-	      && entry != nullptr
-	      && (entry->flags & IS_MAIN) != 0))
-	result = entry;
+      /* Choose the first "main" we see.  The choice among several is
+	 arbitrary.  See the comment by the sole caller to understand
+	 the rationale for filtering by language.  */
+      if (entry != nullptr
+	  && !language_requires_canonicalization (entry->per_cu->lang ()))
+	{
+	  result = entry;
+	  break;
+	}
     }
 
   return result;
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 7fa78d5e87e..e90544f7906 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -58,6 +58,13 @@ DEF_ENUM_FLAGS_TYPE (enum cooked_index_flag_enum, cooked_index_flag);
 
 std::string to_string (cooked_index_flag flags);
 
+/* Return true if LANG requires canonicalization.  This is used
+   primarily to work around an issue computing the name of "main".
+   This function must be kept in sync with
+   cooked_index_shard::do_finalize.  */
+
+extern bool language_requires_canonicalization (enum language lang);
+
 /* A cooked_index_entry represents a single item in the index.  Note
    that two entries can be created for the same DIE -- one using the
    name, and another one using the linkage name, if any.
@@ -144,8 +151,12 @@ struct cooked_index_entry : public allocate_on_obstack
 
   /* Construct the fully-qualified name of this entry and return a
      pointer to it.  If allocation is needed, it will be done on
-     STORAGE.  */
-  const char *full_name (struct obstack *storage) const;
+     STORAGE.  FOR_MAIN is true if we are computing the name of the
+     "main" entry -- one marked DW_AT_main_subprogram.  This matters
+     for avoiding name canonicalization (see comments about this
+     elsewhere) and also a related race (if "main" computation is done
+     during finalization).  */
+  const char *full_name (struct obstack *storage, bool for_main = false) const;
 
   /* Comparison modes for the 'compare' function.  See the function
      for a description.  */
@@ -220,7 +231,8 @@ struct cooked_index_entry : public allocate_on_obstack
 
 private:
 
-  void write_scope (struct obstack *storage, const char *sep) const;
+  void write_scope (struct obstack *storage, const char *sep,
+		    bool for_name) const;
 };
 
 class cooked_index;
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 470ff125c5b..382603c2936 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -7167,8 +7167,17 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile)
 
   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 ());
+    {
+      /* We only do this for names not requiring canonicalization.  At
+	 this point in the process, names have not been canonicalized.
+	 However, currently, languages that require this step also do
+	 not use DW_AT_main_subprogram.  An assert is appropriate here
+	 because this filtering is done in get_main.  */
+      enum language lang = main_entry->per_cu->lang ();
+      gdb_assert (!language_requires_canonicalization (lang));
+      const char *full_name = main_entry->full_name (&per_bfd->obstack, true);
+      set_objfile_main_name (objfile, full_name, lang);
+    }
 
   dwarf_read_debug_printf ("Done building psymtabs of %s",
 			   objfile_name (objfile));
diff --git a/gdb/testsuite/gdb.dlang/dlang-start.exp b/gdb/testsuite/gdb.dlang/dlang-start.exp
new file mode 100644
index 00000000000..fd4688b0635
--- /dev/null
+++ b/gdb/testsuite/gdb.dlang/dlang-start.exp
@@ -0,0 +1,38 @@
+# Copyright (C) 2023 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 "start" for D.
+
+load_lib d-support.exp
+require allow_d_tests
+
+# This testcase verifies the behavior of the `start' command, which
+# does not work when we use the gdb stub...
+require !use_gdb_stub
+
+standard_testfile simple.d
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug d}]} {
+    return -1
+}
+
+# Verify that "start" lands inside the right procedure.
+if {[gdb_start_cmd] < 0} {
+    unsupported "start failed"
+    return -1
+}
+
+gdb_test "" \
+    "main \\(\\) at .*simple.d.*" \
+    "start"
diff --git a/gdb/testsuite/gdb.dlang/simple.d b/gdb/testsuite/gdb.dlang/simple.d
new file mode 100644
index 00000000000..b00884b1b9f
--- /dev/null
+++ b/gdb/testsuite/gdb.dlang/simple.d
@@ -0,0 +1,17 @@
+// Copyright (C) 2023 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/>.
+
+void main() {
+}
diff --git a/gdb/testsuite/gdb.dwarf2/main-subprogram.exp b/gdb/testsuite/gdb.dwarf2/main-subprogram.exp
index 23f02df8513..149b7a801be 100644
--- a/gdb/testsuite/gdb.dwarf2/main-subprogram.exp
+++ b/gdb/testsuite/gdb.dwarf2/main-subprogram.exp
@@ -27,8 +27,9 @@ Dwarf::assemble $asm_file {
     global srcfile
 
     cu {} {
+	# Note we don't want C here as that requires canonicalization.
 	DW_TAG_compile_unit {
-                {DW_AT_language @DW_LANG_C}
+		{DW_AT_language @DW_LANG_PLI}
                 {DW_AT_name     $srcfile}
                 {DW_AT_comp_dir /tmp}
         } {
diff --git a/gdb/testsuite/gdb.rust/rust-start.exp b/gdb/testsuite/gdb.rust/rust-start.exp
new file mode 100644
index 00000000000..96ba2ae3ac8
--- /dev/null
+++ b/gdb/testsuite/gdb.rust/rust-start.exp
@@ -0,0 +1,38 @@
+# Copyright (C) 2023 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 "start" for Rust.
+
+load_lib rust-support.exp
+require allow_rust_tests
+
+# This testcase verifies the behavior of the `start' command, which
+# does not work when we use the gdb stub...
+require !use_gdb_stub
+
+standard_testfile simple.rs
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug rust}]} {
+    return -1
+}
+
+# Verify that "start" lands inside the right procedure.
+if {[gdb_start_cmd] < 0} {
+    unsupported "start failed"
+    return -1
+}
+
+gdb_test "" \
+    "simple::main \\(\\) at .*simple.rs.*" \
+    "start"
-- 
2.39.1

Re: [PATCH] Fix "start" for D, Rust, etc

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

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

BTW, I'd like to try to land this in time for GDB 13.

Tom

Re: [PATCH] Fix "start" for D, Rust, etc

[Andrew Burgess]

Tom Tromey <tom@tromey.com> writes:

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

After reading the description a couple of times, then looking at the
patch and reading it again, I feel I understand what's going on, except
for one thing....

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

... I don't understand what this last paragraph has to do with the
problem being solved here.

I have a couple of _really_ minor comment fixes that I think would help,
and one small code suggestion.  See inline below.

>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30116
> ---
>  gdb/dwarf2/cooked-index.c                    | 50 +++++++++++++-------
>  gdb/dwarf2/cooked-index.h                    | 18 +++++--
>  gdb/dwarf2/read.c                            | 13 ++++-
>  gdb/testsuite/gdb.dlang/dlang-start.exp      | 38 +++++++++++++++
>  gdb/testsuite/gdb.dlang/simple.d             | 17 +++++++
>  gdb/testsuite/gdb.dwarf2/main-subprogram.exp |  3 +-
>  gdb/testsuite/gdb.rust/rust-start.exp        | 38 +++++++++++++++
>  7 files changed, 154 insertions(+), 23 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.dlang/dlang-start.exp
>  create mode 100644 gdb/testsuite/gdb.dlang/simple.d
>  create mode 100644 gdb/testsuite/gdb.rust/rust-start.exp
>
> diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
> index 3d23a65ad8f..d465028add4 100644
> --- a/gdb/dwarf2/cooked-index.c
> +++ b/gdb/dwarf2/cooked-index.c
> @@ -48,6 +48,16 @@ to_string (cooked_index_flag flags)
>  
>  /* See cooked-index.h.  */
>  
> +bool
> +language_requires_canonicalization (enum language lang)
> +{
> +  return (lang == language_ada
> +	  || lang == language_c
> +	  || lang == language_cplus);
> +}
> +
> +/* See cooked-index.h.  */
> +
>  int
>  cooked_index_entry::compare (const char *stra, const char *strb,
>  			     comparison_mode mode)
> @@ -162,10 +172,12 @@ test_compare ()
>  /* See cooked-index.h.  */
>  
>  const char *
> -cooked_index_entry::full_name (struct obstack *storage) const
> +cooked_index_entry::full_name (struct obstack *storage, bool for_main) const
>  {
> +  const char *local_name = for_main ? name : canonical;
> +
>    if ((flags & IS_LINKAGE) != 0 || parent_entry == nullptr)
> -    return canonical;
> +    return local_name;
>  
>    const char *sep = nullptr;
>    switch (per_cu->lang ())
> @@ -182,11 +194,11 @@ cooked_index_entry::full_name (struct obstack *storage) const
>        break;
>  
>      default:
> -      return canonical;
> +      return local_name;
>      }
>  
> -  parent_entry->write_scope (storage, sep);
> -  obstack_grow0 (storage, canonical, strlen (canonical));
> +  parent_entry->write_scope (storage, sep, for_main);
> +  obstack_grow0 (storage, local_name, strlen (local_name));
>    return (const char *) obstack_finish (storage);
>  }
>  
> @@ -194,11 +206,13 @@ cooked_index_entry::full_name (struct obstack *storage) const
>  
>  void
>  cooked_index_entry::write_scope (struct obstack *storage,
> -				 const char *sep) const
> +				 const char *sep,
> +				 bool for_main) const

The comment above this function says: 'See cooked_index.h', but there's
no comment there explaining any of the parameters... which is bit of a
shame.

>  {
>    if (parent_entry != nullptr)
> -    parent_entry->write_scope (storage, sep);
> -  obstack_grow (storage, canonical, strlen (canonical));
> +    parent_entry->write_scope (storage, sep, for_main);
> +  const char *local_name = for_main ? name : canonical;
> +  obstack_grow (storage, local_name, strlen (local_name));
>    obstack_grow (storage, sep, strlen (sep));
>  }
>  
> @@ -218,10 +232,6 @@ cooked_index_shard::add (sect_offset die_offset, enum dwarf_tag tag,
>       implicit "main" discovery.  */
>    if ((flags & IS_MAIN) != 0)
>      m_main = result;
> -  else if (per_cu->lang () != language_ada
> -	   && m_main == nullptr
> -	   && strcmp (name, "main") == 0)
> -    m_main = result;

The comment on m_main in cooked-index.h needs updating after this change
I think.

>  
>    return result;
>  }
> @@ -323,6 +333,8 @@ cooked_index_shard::do_finalize ()
>  
>    for (cooked_index_entry *entry : m_entries)
>      {
> +      /* Note that this code must be kept in sync with
> +	 language_requires_canonicalization.  */

Can I suggest that after the switch statement we include this assert:

  gdb_assert (entry->canonical == entry->name
              || language_requires_canonicalization (entry->per_cu->lang ()));

Then, if we ever add additional canonicalization and forget to keep
language_requires_canonicalization in sync, the assert will fire.


>        gdb_assert (entry->canonical == nullptr);
>        if ((entry->flags & IS_LINKAGE) != 0)
>  	entry->canonical = entry->name;
> @@ -474,11 +486,15 @@ cooked_index::get_main () const
>    for (const auto &index : m_vector)
>      {
>        const cooked_index_entry *entry = index->get_main ();
> -      if (result == nullptr
> -	  || ((result->flags & IS_MAIN) == 0
> -	      && entry != nullptr
> -	      && (entry->flags & IS_MAIN) != 0))
> -	result = entry;
> +      /* Choose the first "main" we see.  The choice among several is
> +	 arbitrary.  See the comment by the sole caller to understand
> +	 the rationale for filtering by language.  */
> +      if (entry != nullptr
> +	  && !language_requires_canonicalization (entry->per_cu->lang ()))
> +	{
> +	  result = entry;
> +	  break;
> +	}
>      }
>  
>    return result;
> diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
> index 7fa78d5e87e..e90544f7906 100644
> --- a/gdb/dwarf2/cooked-index.h
> +++ b/gdb/dwarf2/cooked-index.h
> @@ -58,6 +58,13 @@ DEF_ENUM_FLAGS_TYPE (enum cooked_index_flag_enum, cooked_index_flag);
>  
>  std::string to_string (cooked_index_flag flags);
>  
> +/* Return true if LANG requires canonicalization.  This is used
> +   primarily to work around an issue computing the name of "main".
> +   This function must be kept in sync with
> +   cooked_index_shard::do_finalize.  */
> +
> +extern bool language_requires_canonicalization (enum language lang);
> +
>  /* A cooked_index_entry represents a single item in the index.  Note
>     that two entries can be created for the same DIE -- one using the
>     name, and another one using the linkage name, if any.
> @@ -144,8 +151,12 @@ struct cooked_index_entry : public allocate_on_obstack
>  
>    /* Construct the fully-qualified name of this entry and return a
>       pointer to it.  If allocation is needed, it will be done on
> -     STORAGE.  */
> -  const char *full_name (struct obstack *storage) const;
> +     STORAGE.  FOR_MAIN is true if we are computing the name of the
> +     "main" entry -- one marked DW_AT_main_subprogram.  This matters
> +     for avoiding name canonicalization (see comments about this
> +     elsewhere) and also a related race (if "main" computation is done

Saying "see comments about this elsewhere" isn't very helpful.  Ideally
say where to go look.  Or repeat the reason here.  Or just drop that
sentence.

> +     during finalization).  */
> +  const char *full_name (struct obstack *storage, bool for_main = false) const;
>  
>    /* Comparison modes for the 'compare' function.  See the function
>       for a description.  */
> @@ -220,7 +231,8 @@ struct cooked_index_entry : public allocate_on_obstack
>  
>  private:
>  
> -  void write_scope (struct obstack *storage, const char *sep) const;
> +  void write_scope (struct obstack *storage, const char *sep,
> +		    bool for_name) const;
>  };
>  
>  class cooked_index;
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 470ff125c5b..382603c2936 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -7167,8 +7167,17 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile)
>  
>    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 ());
> +    {
> +      /* We only do this for names not requiring canonicalization.  At
> +	 this point in the process, names have not been canonicalized.

I think the comma after process can be dropped.

> +	 However, currently, languages that require this step also do
> +	 not use DW_AT_main_subprogram.  An assert is appropriate here
> +	 because this filtering is done in get_main.  */
> +      enum language lang = main_entry->per_cu->lang ();
> +      gdb_assert (!language_requires_canonicalization (lang));
> +      const char *full_name = main_entry->full_name (&per_bfd->obstack, true);
> +      set_objfile_main_name (objfile, full_name, lang);
> +    }
>  
>    dwarf_read_debug_printf ("Done building psymtabs of %s",
>  			   objfile_name (objfile));
> diff --git a/gdb/testsuite/gdb.dlang/dlang-start.exp b/gdb/testsuite/gdb.dlang/dlang-start.exp
> new file mode 100644
> index 00000000000..fd4688b0635
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dlang/dlang-start.exp
> @@ -0,0 +1,38 @@
> +# Copyright (C) 2023 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 "start" for D.
> +
> +load_lib d-support.exp
> +require allow_d_tests
> +
> +# This testcase verifies the behavior of the `start' command, which
> +# does not work when we use the gdb stub...
> +require !use_gdb_stub
> +
> +standard_testfile simple.d
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug d}]} {
> +    return -1
> +}
> +
> +# Verify that "start" lands inside the right procedure.
> +if {[gdb_start_cmd] < 0} {
> +    unsupported "start failed"
> +    return -1
> +}
> +
> +gdb_test "" \
> +    "main \\(\\) at .*simple.d.*" \
> +    "start"
> diff --git a/gdb/testsuite/gdb.dlang/simple.d b/gdb/testsuite/gdb.dlang/simple.d
> new file mode 100644
> index 00000000000..b00884b1b9f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dlang/simple.d
> @@ -0,0 +1,17 @@
> +// Copyright (C) 2023 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/>.
> +
> +void main() {
> +}
> diff --git a/gdb/testsuite/gdb.dwarf2/main-subprogram.exp b/gdb/testsuite/gdb.dwarf2/main-subprogram.exp
> index 23f02df8513..149b7a801be 100644
> --- a/gdb/testsuite/gdb.dwarf2/main-subprogram.exp
> +++ b/gdb/testsuite/gdb.dwarf2/main-subprogram.exp
> @@ -27,8 +27,9 @@ Dwarf::assemble $asm_file {
>      global srcfile
>  
>      cu {} {
> +	# Note we don't want C here as that requires canonicalization.
>  	DW_TAG_compile_unit {
> -                {DW_AT_language @DW_LANG_C}
> +		{DW_AT_language @DW_LANG_PLI}

I'm curious why PLI?  I guess it's to force GDB to select language
minimal, but this is so weird it feels like it's worth a comment.

I agree that the solution feels a little yuck, but I'm not sure we'd
want anything more complex until we're forced too (by some new language
that uses DW_AT_main_subprogram and requires canonicalization).  Plus I
was able to follow what's going on here, so I'd be happy to see this in
GDB.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew



>                  {DW_AT_name     $srcfile}
>                  {DW_AT_comp_dir /tmp}
>          } {
> diff --git a/gdb/testsuite/gdb.rust/rust-start.exp b/gdb/testsuite/gdb.rust/rust-start.exp
> new file mode 100644
> index 00000000000..96ba2ae3ac8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.rust/rust-start.exp
> @@ -0,0 +1,38 @@
> +# Copyright (C) 2023 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 "start" for Rust.
> +
> +load_lib rust-support.exp
> +require allow_rust_tests
> +
> +# This testcase verifies the behavior of the `start' command, which
> +# does not work when we use the gdb stub...
> +require !use_gdb_stub
> +
> +standard_testfile simple.rs
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug rust}]} {
> +    return -1
> +}
> +
> +# Verify that "start" lands inside the right procedure.
> +if {[gdb_start_cmd] < 0} {
> +    unsupported "start failed"
> +    return -1
> +}
> +
> +gdb_test "" \
> +    "simple::main \\(\\) at .*simple.rs.*" \
> +    "start"
> -- 
> 2.39.1

Re: [PATCH] Fix "start" for D, Rust, etc

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

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

Andrew> ... I don't understand what this last paragraph has to do with the
Andrew> problem being solved here.

Yeah, it's kind of tangential.  Essay follows.

In an ideal world, gdb would scan the DWARF, and if it saw
DW_AT_main_subprogram, it would use that name.

That doesn't fully work because gdb requires canonicalization.  (Maybe
all debuggers do in some way or another, but gdb chooses to do it during
scanning, so it affects the user-visible startup time.)

gdb makes a second choice here, which is to use the "main" to set the
initial language and context.

So, if gdb scans the DWARF and waits for canonicalization in order to
use "main", then users will wait the entire time it takes... whereas
right now, we hide some of this by canonicalizing in a worker thread.

This patch preserves this feature by deciding to only handle a subset of
languages.

Another possibility would be to delay the search for "main".  When the
user runs "gdb program" and the prompt appears, there's no real need to
pre-emptively find "main".  Instead, it can be deferred until the first
command that actually requires it.

That's what this earlier patch did.  IIRC, I added a "#define" for
current_language that would compute it when first needed.  I think this
required some hacks as well (at the time I think Python initialization
wanted the language).

Another possible approach here would be to canonicalize just the "main"
entry and its parent entries when needed.  This is some annoying
bookkeeping but maybe not too bad.

It's worth noting, though, that elsewhere I'm working on patches to move
more of the DWARF reader into worker threads.  There, the idea is to
start the DWARF reader immediately in dwarf2_has_info (in the
background).  If we had lazy current_language setting, this work would
make the user experience even faster startup times (sometimes).  With
the current shape of the patches, the difference is small for the "file"
case (it's more aimed at "attach"), because, once again, gdb has to wait
for the DWARF reader to dig up that DW_AT_main_subprogram.

Speeding up DWARF reading is pretty hard.  Partly this is because DWARF
is bad, but partly it's due to choices gdb makes.  However, since it's
regularly the slowest thing users encounter, IMO it's worth the extra
effort both to try to hide the costs, and also to try to think of
alternative solutions that perform well.

Andrew> I have a couple of _really_ minor comment fixes that I think would help,
Andrew> and one small code suggestion.  See inline below.

I'll fix all these up.

>> cu {} {
>> +	# Note we don't want C here as that requires canonicalization.
>> DW_TAG_compile_unit {
>> -                {DW_AT_language @DW_LANG_C}
>> +		{DW_AT_language @DW_LANG_PLI}

Andrew> I'm curious why PLI?  I guess it's to force GDB to select language
Andrew> minimal, but this is so weird it feels like it's worth a comment.

There's the "don't want C" comment but I will expand it.

Anyway I chose a language that gdb is unlikely to support and also that
made me laugh.  Normally of course this would be COBOL, but a GCC front
end for COBOL is going in so...

Thanks for the review.

Tom