From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2126) id BD4943858D32; Sat, 18 Feb 2023 23:22:15 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org BD4943858D32 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1676762535; bh=YeYiv3KWrV2Ldve/zRAAqg7jwpOiChdmDUTue/PwICE=; h=From:To:Subject:Date:From; b=w84HERhof58Zyk5zxNi1yXZCKzdT75ZbGizGIcBcWlDxxaSyOFRnQLys/6StKjtnu FAZ0yD7dCQKoY34gBQ9Rb1U0jFcn2fk4aY32oP7cIHAxc2aYersaxzrJbn5UFFpb8v +IKBzOTb2FPu6lQSnVwl/Kv63vjry5EspE3VW5Bo= Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Tom Tromey To: gdb-cvs@sourceware.org Subject: [binutils-gdb] Fix "start" for D, Rust, etc X-Act-Checkin: binutils-gdb X-Git-Author: Tom Tromey X-Git-Refname: refs/heads/master X-Git-Oldrev: e8eca7a6b602290bb3f50728432d524577ade727 X-Git-Newrev: 47fe57c92810c7302bb80eafdec6f4345bcc69c8 Message-Id: <20230218232215.BD4943858D32@sourceware.org> Date: Sat, 18 Feb 2023 23:22:15 +0000 (GMT) List-Id: https://sourceware.org/git/gitweb.cgi?p=3Dbinutils-gdb.git;h=3D47fe57c92810= c7302bb80eafdec6f4345bcc69c8 commit 47fe57c92810c7302bb80eafdec6f4345bcc69c8 Author: Tom Tromey Date: Mon Feb 13 17:44:54 2023 -0700 Fix "start" for D, Rust, etc =20 The new DWARF indexer broke "start" for some languages. =20 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. =20 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". =20 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. =20 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. =20 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. =20 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. =20 This is kind of tricky and ugly, so I've tried to comment it sufficiently. =20 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. =20 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. =20 Reviewed-By: Andrew Burgess Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=3D30116 Diff: --- gdb/dwarf2/cooked-index.c | 50 ++++++++++++++++++------= ---- gdb/dwarf2/cooked-index.h | 23 ++++++++++--- 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 | 5 ++- gdb/testsuite/gdb.rust/rust-start.exp | 38 +++++++++++++++++++++ 7 files changed, 159 insertions(+), 25 deletions(-) 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) =20 /* See cooked-index.h. */ =20 +bool +language_requires_canonicalization (enum language lang) +{ + return (lang =3D=3D language_ada + || lang =3D=3D language_c + || lang =3D=3D 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. */ =20 const char * -cooked_index_entry::full_name (struct obstack *storage) const +cooked_index_entry::full_name (struct obstack *storage, bool for_main) con= st { + const char *local_name =3D for_main ? name : canonical; + if ((flags & IS_LINKAGE) !=3D 0 || parent_entry =3D=3D nullptr) - return canonical; + return local_name; =20 const char *sep =3D nullptr; switch (per_cu->lang ()) @@ -182,11 +194,11 @@ cooked_index_entry::full_name (struct obstack *storag= e) const break; =20 default: - return canonical; + return local_name; } =20 - 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); } =20 @@ -194,11 +206,13 @@ cooked_index_entry::full_name (struct obstack *storag= e) const =20 void cooked_index_entry::write_scope (struct obstack *storage, - const char *sep) const + const char *sep, + bool for_main) const { if (parent_entry !=3D 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 =3D for_main ? name : canonical; + obstack_grow (storage, local_name, strlen (local_name)); obstack_grow (storage, sep, strlen (sep)); } =20 @@ -218,10 +232,6 @@ cooked_index_shard::add (sect_offset die_offset, enum = dwarf_tag tag, implicit "main" discovery. */ if ((flags & IS_MAIN) !=3D 0) m_main =3D result; - else if (per_cu->lang () !=3D language_ada - && m_main =3D=3D nullptr - && strcmp (name, "main") =3D=3D 0) - m_main =3D result; =20 return result; } @@ -323,6 +333,8 @@ cooked_index_shard::do_finalize () =20 for (cooked_index_entry *entry : m_entries) { + /* Note that this code must be kept in sync with + language_requires_canonicalization. */ gdb_assert (entry->canonical =3D=3D nullptr); if ((entry->flags & IS_LINKAGE) !=3D 0) entry->canonical =3D entry->name; @@ -474,11 +486,15 @@ cooked_index::get_main () const for (const auto &index : m_vector) { const cooked_index_entry *entry =3D index->get_main (); - if (result =3D=3D nullptr - || ((result->flags & IS_MAIN) =3D=3D 0 - && entry !=3D nullptr - && (entry->flags & IS_MAIN) !=3D 0)) - result =3D 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 !=3D nullptr + && !language_requires_canonicalization (entry->per_cu->lang ())) + { + result =3D entry; + break; + } } =20 return result; diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h index 7fa78d5e87e..135f982d4b4 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); =20 std::string to_string (cooked_index_flag flags); =20 +/* 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,11 @@ struct cooked_index_entry : public allocate_on_obstack =20 /* 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 and also a related race (if + "main" computation is done during finalization). */ + const char *full_name (struct obstack *storage, bool for_main =3D false)= const; =20 /* Comparison modes for the 'compare' function. See the function for a description. */ @@ -220,7 +230,11 @@ struct cooked_index_entry : public allocate_on_obstack =20 private: =20 - void write_scope (struct obstack *storage, const char *sep) const; + /* A helper method for full_name. Emits the full scope of this + object, followed by the separator, to STORAGE. If this entry has + a parent, its write_scope method is called first. */ + void write_scope (struct obstack *storage, const char *sep, + bool for_name) const; }; =20 class cooked_index; @@ -325,8 +339,7 @@ private: auto_obstack m_storage; /* List of all entries. */ std::vector m_entries; - /* If we found "main" or an entry with 'is_main' set, store it - here. */ + /* If we found an entry with 'is_main' set, store it here. */ cooked_index_entry *m_main =3D nullptr; /* The addrmap. This maps address ranges to dwarf2_per_cu_data objects. */ diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 4fd46fd43f8..9ad1afa5d61 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -5167,8 +5167,17 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_= objfile) =20 const cooked_index_entry *main_entry =3D vec->get_main (); if (main_entry !=3D 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 =3D main_entry->per_cu->lang (); + gdb_assert (!language_requires_canonicalization (lang)); + const char *full_name =3D main_entry->full_name (&per_bfd->obstack, = true); + set_objfile_main_name (objfile, full_name, lang); + } =20 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.dl= ang/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 . + +# 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/sim= ple.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 . + +void main() { +} diff --git a/gdb/testsuite/gdb.dwarf2/main-subprogram.exp b/gdb/testsuite/g= db.dwarf2/main-subprogram.exp index 23f02df8513..9727dd4d725 100644 --- a/gdb/testsuite/gdb.dwarf2/main-subprogram.exp +++ b/gdb/testsuite/gdb.dwarf2/main-subprogram.exp @@ -27,8 +27,11 @@ Dwarf::assemble $asm_file { global srcfile =20 cu {} { + # Note we don't want C here as that requires canonicalization, + # so choose a language that isn't C and that gdb is unlikely + # to implement. 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 . + +# 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 rus= t}]} { + 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"