[RFC 0/7] [gdb/symtab, cc-with-dwz] Fix gdb.cp/breakpoint-locs.exp

[RFC 0/7] [gdb/symtab, cc-with-dwz] Fix gdb.cp/breakpoint-locs.exp

[Tom de Vries]

When running test-case gdb.cp/breakpoint-locs.exp with target board
cc-with-dwz, we occasionally run into:
...
FAIL: gdb.cp/breakpoint-locs.exp: break N1::C1::baz
...
due to the breakpoint having one instead of two locations.

This is PR symtab/30728.  This series fixes it.

The first five patches are preparation, the fix is in the last two patches.

The fix consists of adding entries to the cooked index for
DW_TAG_inlined_subroutine DIEs, with correct qualified names.

The fix is split up in two patches, as follows:
- the first fixes the target board unix case, and
- the second fixes the target board cc-with-dwz case.

Currently resolving deferred entries is done in the parallel for that
computes the cooked index.  The last patch moves resolving of deferred entries
out of the parallel for.

We can add two optimizations to move some of that computation back into the
parallel for:
- resolve intra-shard dependencies at the end of generating the shard, and
- don't defer backward intra-shard dependencies.

Both of these require to keep track of the range of DIEs that was indexed
in the shard, because currently the parent tracking cannot distuinguish
between the "no parent" and "don't know" cases.

I'd like to have some feedback on the current series before I proceed with
these optimizations, hence this is an RFC at this point.

Tested on x86_64-linux, with target boards unix, cc-with-dwz and
cc-with-dwz-m.

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

Tom de Vries (7):
  [gdb/symtab] Factor out m_die_range_map and m_deferred_entries usage
  [gdb/symtab] Check effect in parent_map::set_parent
  [gdb/symtab] Handle nullptr parent in parent_map::set_parent
  [gdb/symtab] Add parent_map::dump
  [gdb/symtab] Add debug_handle_deferred_entries
  [gdb/symtab] Add DW_TAG_inlined_subroutine entries in the cooked index
    for c++
  [gdb/symtab] Resolve deferred entries, inter-shard case

 gdb/addrmap.c             |  19 ++++--
 gdb/addrmap.h             |   7 ++-
 gdb/dwarf2/cooked-index.c |  60 +++++++++++++++++++
 gdb/dwarf2/cooked-index.h | 114 +++++++++++++++++++++++++++++++++++-
 gdb/dwarf2/read.c         | 120 +++++++++++++++++++++++++-------------
 5 files changed, 269 insertions(+), 51 deletions(-)


base-commit: b73ffa23bf6ed7f48ce67881d97b4111ce3b8181
-- 
2.35.3

[RFC 1/7] [gdb/symtab] Factor out m_die_range_map and m_deferred_entries usage

[Tom de Vries]

Factor out all usage of cooked_indexer::m_die_range_map and
cooked_indexer::m_deferred_entries in new member functions:
- find_parent,
- set_parent,
- defer_entry, and
- handle_deferred_entries,
and factor out class parent_map.

Tested on x86_64-linux.
---
 gdb/dwarf2/cooked-index.h | 22 ++++++++++++++++++
 gdb/dwarf2/read.c         | 48 +++++++++++++++++++++++++++++----------
 2 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 5aacb321c91..39809b488ca 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -239,6 +239,28 @@ struct cooked_index_entry : public allocate_on_obstack
 		    bool for_name) const;
 };
 
+class parent_map
+{
+public:
+  /* Find the parent of DIE LOOKUP.  */
+  const cooked_index_entry *find_parent (CORE_ADDR lookup)
+  {
+    void *obj = m_parent_map.find (lookup);
+    return static_cast<const cooked_index_entry *> (obj);
+  }
+
+  /* Set the parent of DIES in range [START, END] to PARENT_ENTRY.  */
+  void set_parent (CORE_ADDR start, CORE_ADDR end,
+		   const cooked_index_entry *parent_entry)
+  {
+    m_parent_map.set_empty (start, end, (void *)parent_entry);
+  }
+
+private:
+  /* An addrmap that maps from section offsets to cooked_index_entry *.  */
+  addrmap_mutable m_parent_map;
+};
+
 class cooked_index;
 
 /* An index of interesting DIEs.  This is "cooked", in contrast to a
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index eb4cb9ba72e..1e2c7f87088 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -4797,7 +4797,20 @@ class cooked_indexer
   /* An addrmap that maps from section offsets (see the form_addr
      method) to newly-created entries.  See m_deferred_entries to
      understand this.  */
-  addrmap_mutable m_die_range_map;
+  parent_map m_die_range_map;
+
+  /* Find the parent of DIE LOOKUP.  */
+  const cooked_index_entry *find_parent (CORE_ADDR lookup)
+  {
+    return m_die_range_map.find_parent (lookup);
+  }
+
+  /* Set the parent of DIES in range [START, END] to PARENT_ENTRY.  */
+  void set_parent (CORE_ADDR start, CORE_ADDR end,
+		   const cooked_index_entry *parent_entry)
+  {
+    m_die_range_map.set_parent (start, end, parent_entry);
+  }
 
   /* A single deferred entry.  */
   struct deferred_entry
@@ -4817,6 +4830,24 @@ class cooked_indexer
      we'll know the containing context of all the DIEs that we might
      have scanned.  This vector stores these deferred entries.  */
   std::vector<deferred_entry> m_deferred_entries;
+
+  /* Defer creating a cooked_index_entry corresponding to DEFERRED.  */
+  void defer_entry (deferred_entry de)
+  {
+    m_deferred_entries.push_back (de);
+  }
+
+  /* Create cooked_index_entries for the deferred entries.  */
+  void handle_deferred_entries ()
+  {
+    for (const auto &entry : m_deferred_entries)
+      {
+	const cooked_index_entry *parent_entry
+	  = find_parent (entry.spec_offset);
+	m_index_storage->add (entry.die_offset, entry.tag, entry.flags,
+			      entry.name, parent_entry, m_per_cu);
+      }
+  }
 };
 
 /* Subroutine of dwarf2_build_psymtabs_hard to simplify it.
@@ -16366,8 +16397,7 @@ cooked_indexer::scan_attributes (dwarf2_per_cu_data *scanning_per_cu,
 	  else if (*parent_entry == nullptr)
 	    {
 	      CORE_ADDR lookup = form_addr (origin_offset, origin_is_dwz);
-	      void *obj = m_die_range_map.find (lookup);
-	      *parent_entry = static_cast <cooked_index_entry *> (obj);
+	      *parent_entry = find_parent (lookup);
 	    }
 
 	  unsigned int bytes_read;
@@ -16489,7 +16519,7 @@ cooked_indexer::recurse (cutu_reader *reader,
 				   reader->cu->per_cu->is_dwz);
       CORE_ADDR end = form_addr (sect_offset (info_ptr - 1 - reader->buffer),
 				 reader->cu->per_cu->is_dwz);
-      m_die_range_map.set_empty (start, end, (void *) parent_entry);
+      set_parent (start, end, parent_entry);
     }
 
   return info_ptr;
@@ -16555,7 +16585,7 @@ cooked_indexer::index_dies (cutu_reader *reader,
       if (name != nullptr)
 	{
 	  if (defer != 0)
-	    m_deferred_entries.push_back ({
+	    defer_entry ({
 		this_die, name, defer, abbrev->tag, flags
 	      });
 	  else
@@ -16660,13 +16690,7 @@ cooked_indexer::make_index (cutu_reader *reader)
     return;
   index_dies (reader, reader->info_ptr, nullptr, false);
 
-  for (const auto &entry : m_deferred_entries)
-    {
-      void *obj = m_die_range_map.find (entry.spec_offset);
-      cooked_index_entry *parent = static_cast<cooked_index_entry *> (obj);
-      m_index_storage->add (entry.die_offset, entry.tag, entry.flags,
-			    entry.name, parent, m_per_cu);
-    }
+  handle_deferred_entries ();
 }
 
 /* An implementation of quick_symbol_functions for the cooked DWARF
-- 
2.35.3

[RFC 2/7] [gdb/symtab] Check effect in parent_map::set_parent

[Tom de Vries]

Set_parent uses m_die_range_map.set_empty to set the parent of a range.

If part of the range is already set, it remains the same.

If the entire range is already set, the set_empty has no effect, silently.

Fix this by verifying that calling set_empty has the desired effect on the
start and end points of the range.

Tested on x86_64-linux.
---
 gdb/dwarf2/cooked-index.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 39809b488ca..a921f25ecf2 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -254,6 +254,13 @@ class parent_map
 		   const cooked_index_entry *parent_entry)
   {
     m_parent_map.set_empty (start, end, (void *)parent_entry);
+
+    /* Assert that set_parent has the desired effect.  This is not trivial due
+       to how set_empty works.  If the range already has been set before, it
+       has no effect.  */
+    gdb_assert (m_parent_map.find (start) == parent_entry);
+    if (end != start)
+      gdb_assert (m_parent_map.find (end) == parent_entry);
   }
 
 private:
-- 
2.35.3

[RFC 3/7] [gdb/symtab] Handle nullptr parent in parent_map::set_parent

[Tom de Vries]

Set_parent uses m_die_range_map.set_empty, which doesn't allow
parent_entry == nullptr.

So it may be necessary to guard calls to set_parent with
"if (parent_entry != nullptr)".

Fix this by handling the parent_entry == nullptr case.

Tested on x86_64-linux.
---
 gdb/dwarf2/cooked-index.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index a921f25ecf2..23db5918878 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -253,7 +253,10 @@ class parent_map
   void set_parent (CORE_ADDR start, CORE_ADDR end,
 		   const cooked_index_entry *parent_entry)
   {
-    m_parent_map.set_empty (start, end, (void *)parent_entry);
+    /* Calling set_empty with nullptr is currently not allowed.  Note that we
+       still check the desired effect.  */
+    if (parent_entry != nullptr)
+      m_parent_map.set_empty (start, end, (void *)parent_entry);
 
     /* Assert that set_parent has the desired effect.  This is not trivial due
        to how set_empty works.  If the range already has been set before, it
-- 
2.35.3

[RFC 4/7] [gdb/symtab] Add parent_map::dump

[Tom de Vries]

When dumping m_die_range_map in handle_deferred_dies using:
...
    addrmap_dump (&m_die_range_map, gdb_stdlog, nullptr);
...
I get:
...
  0x00000000000000d4 0x355e600
  0x00000000000000de 0x355e630
  0x00000000000000f5 0x355e600
  0x0000000000000101 0x0
  0x00000000000001c2 0x355e7b0
  0x00000000000001cc 0x355e7e0
  0x00000000000001e3 0x355e7b0
  0x00000000000001ef 0x0
...
which doesn't make it clear which cooked_die_entries the values refer to.

Add a function parent_map::dump that passes an annotation function
to addrmap_dump such that we have instead:
...
  0x00000000000000d4 0x360d300 (0xd3)
  0x00000000000000de 0x360d330 (0xdd)
  0x00000000000000f5 0x360d300 (0xd3)
  0x0000000000000101 0x0
  0x00000000000001c2 0x360d4b0 (0x1c1)
  0x00000000000001cc 0x360d4e0 (0x1cb)
  0x00000000000001e3 0x360d4b0 (0x1c1)
  0x00000000000001ef 0x0
...

Tested on x86_64-linux.
---
 gdb/addrmap.c             | 19 +++++++++++++------
 gdb/addrmap.h             |  7 +++++--
 gdb/dwarf2/cooked-index.h | 17 +++++++++++++++++
 3 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/gdb/addrmap.c b/gdb/addrmap.c
index d16775d49d4..4c58427903b 100644
--- a/gdb/addrmap.c
+++ b/gdb/addrmap.c
@@ -361,7 +361,9 @@ addrmap_mutable::~addrmap_mutable ()
 /* See addrmap.h.  */
 
 void
-addrmap_dump (struct addrmap *map, struct ui_file *outfile, void *payload)
+addrmap_dump (struct addrmap *map, struct ui_file *outfile, void *payload,
+	      void (*annotate_value)(struct ui_file *outfile,
+				     const void *value))
 {
   /* True if the previously printed addrmap entry was for PAYLOAD.
      If so, we want to print the next one as well (since the next
@@ -380,11 +382,16 @@ addrmap_dump (struct addrmap *map, struct ui_file *outfile, void *payload)
       addr_str = "<ends here>";
 
     if (matches || previous_matched)
-      gdb_printf (outfile, "  %s%s %s\n",
-		  payload != nullptr ? "  " : "",
-		  core_addr_to_string (start_addr),
-		  addr_str);
-
+      {
+	gdb_printf (outfile, "  %s%s %s",
+		    payload != nullptr ? "  " : "",
+		    core_addr_to_string (start_addr),
+		    addr_str);
+	/* Annotate value.  */
+	if (annotate_value != nullptr)
+	  annotate_value (outfile, obj);
+	gdb_printf (outfile, "\n");
+      }
     previous_matched = matches;
 
     return 0;
diff --git a/gdb/addrmap.h b/gdb/addrmap.h
index e00dda6e711..f67cd32df1d 100644
--- a/gdb/addrmap.h
+++ b/gdb/addrmap.h
@@ -205,8 +205,11 @@ struct addrmap_mutable : public addrmap
 
 /* Dump the addrmap to OUTFILE.  If PAYLOAD is non-NULL, only dump any
    components that map to PAYLOAD.  (If PAYLOAD is NULL, the entire
-   map is dumped.)  */
+   map is dumped.)  If ANNOTATE_VALUE is non-nullptr, call it for each
+   value.  */
 void addrmap_dump (struct addrmap *map, struct ui_file *outfile,
-		   void *payload);
+		   void *payload,
+		   void (*annotate_value)(struct ui_file *outfile,
+					  const void *value) = nullptr);
 
 #endif /* ADDRMAP_H */
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 23db5918878..457ee819eac 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -266,6 +266,23 @@ class parent_map
       gdb_assert (m_parent_map.find (end) == parent_entry);
   }
 
+  /* Dump the parent map.  */
+  void dump ()
+  {
+    auto annotate_cooked_index_entry
+      = [] (struct ui_file *outfile, const void *value)
+      {
+	const cooked_index_entry *parent_entry
+	  = (const cooked_index_entry *)value;
+	if (parent_entry == nullptr)
+	  return;
+	gdb_printf (outfile, " (0x%" PRIx64 ")",
+		    to_underlying (parent_entry->die_offset));
+      };
+
+    addrmap_dump (&m_parent_map, gdb_stdlog, nullptr, annotate_cooked_index_entry);
+  }
+
 private:
   /* An addrmap that maps from section offsets to cooked_index_entry *.  */
   addrmap_mutable m_parent_map;
-- 
2.35.3

[RFC 5/7] [gdb/symtab] Add debug_handle_deferred_entries

[Tom de Vries]

Add a const bool debug_handle_deferred_entries, defaulting to false, that
allows us to easily debug the handling of deferred entries.

Example output:
...
Reading symbols from outputs/gdb.dwarf2/pr13961/pr13961...^M
  0x000000000000002b 0x3b51cf0 (0x2a)^M
  0x0000000000000044 0x0^M
Resolve deferred: 0x25 -> 0x2a: no parent^M
  0x000000000000002b 0x3b53410 (0x2a)^M
  0x0000000000000044 0x0^M
Resolve deferred: 0x25 -> 0x2a: no parent^M
  0x0000000000000109 0x7fce6c002f60 (0x108)^M
  0x0000000000000123 0x0^M
Resolve deferred: 0xfc -> 0x108: no parent^M
...

Tested on x86_64-linux.
---
 gdb/dwarf2/read.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 1e2c7f87088..40248890c38 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -4840,10 +4840,27 @@ class cooked_indexer
   /* Create cooked_index_entries for the deferred entries.  */
   void handle_deferred_entries ()
   {
+    const bool debug_handle_deferred_entries = false;
+
+    if (debug_handle_deferred_entries)
+      m_die_range_map.dump ();
+
     for (const auto &entry : m_deferred_entries)
       {
 	const cooked_index_entry *parent_entry
 	  = find_parent (entry.spec_offset);
+	if (debug_handle_deferred_entries)
+	  {
+	    gdb_printf (gdb_stdlog,
+			"Resolve deferred: 0x%" PRIx64 " -> 0x%lx: ",
+			to_underlying (entry.die_offset),
+			entry.spec_offset);
+	    if (parent_entry == nullptr)
+	      gdb_printf (gdb_stdlog, "no parent\n");
+	    else
+	      gdb_printf (gdb_stdlog, "0x%" PRIx64 "\n",
+			  to_underlying (parent_entry->die_offset));
+	  }
 	m_index_storage->add (entry.die_offset, entry.tag, entry.flags,
 			      entry.name, parent_entry, m_per_cu);
       }
-- 
2.35.3

[RFC 6/7] [gdb/symtab] Add DW_TAG_inlined_subroutine entries in the cooked index for c++

[Tom de Vries]

Test-case gdb.cp/breakpoint-locs.exp has two DW_TAG_inlined_subroutine entries
at 0x140 and 0x227 representing N1::C1::baz.  The two are similar, and the
first one is:
...
 <1><d3>: Abbrev Number: 2 (DW_TAG_namespace)
    <d4>   DW_AT_name        : N1
 <2><dd>: Abbrev Number: 3 (DW_TAG_class_type)
    <de>   DW_AT_name        : C1
 <3><e8>: Abbrev Number: 4 (DW_TAG_subprogram)
    <e9>   DW_AT_external    : 1
    <e9>   DW_AT_name        : baz
    <ef>   DW_AT_linkage_name: _ZN2N12C13bazEv
    <f3>   DW_AT_accessibility: 1       (public)
    <f4>   DW_AT_declaration : 1
 <1><125>: Abbrev Number: 8 (DW_TAG_subprogram)
    <126>   DW_AT_specification: <0xf5>
    <12a>   DW_AT_low_pc      : 0x4004d7
    <132>   DW_AT_high_pc     : 0x10
    <13a>   DW_AT_frame_base  : 1 byte block: 9c        (DW_OP_call_frame_cfa)
    <13c>   DW_AT_GNU_all_call_sites: 1
 <2><140>: Abbrev Number: 9 (DW_TAG_inlined_subroutine)
    <141>   DW_AT_abstract_origin: <0x173>
    <145>   DW_AT_low_pc      : 0x4004db
    <14d>   DW_AT_high_pc     : 0x9
 <1><173>: Abbrev Number: 12 (DW_TAG_subprogram)
    <174>   DW_AT_specification: <0xe8>
    <178>   DW_AT_inline      : 3       (declared as inline and inlined)
...
but the cooked index doesn't add an entry for it because in
cooked_indexer::index_dies we don't recurse into the children of a
DW_TAG_subprogram for c++:
...
	    case DW_TAG_subprogram:
	      if ((m_language == language_fortran
		   || m_language == language_ada)
		  && this_entry != nullptr)
		{
		  info_ptr = recurse (reader, info_ptr, this_entry, true);
...

Doing so yields the following entries:
...
    [6] ((cooked_index_entry *) 0x7faa98005370)
    name:       baz
    canonical:  baz
    qualified:  N1::foo::baz
    DWARF tag:  DW_TAG_inlined_subroutine
    flags:      0x0 []
    DIE offset: 0x140
    parent:     ((cooked_index_entry *) 0x7faa98005310) [foo]
  ...
    [20] ((cooked_index_entry *) 0x7faa80002fc0)
    name:       baz
    canonical:  baz
    qualified:  N1::bar::baz
    DWARF tag:  DW_TAG_inlined_subroutine
    flags:      0x0 []
    DIE offset: 0x227
    parent:     ((cooked_index_entry *) 0x7faa80002f60) [bar]
...

The entries have incorrect qualified names N1::foo::baz and N1::bar::baz, due
to an incorrect parent field.  Indeed in DIE terms the parent is foo/bar, but
we use the parent field to track encapsulating scopes, so fix this by ignoring
the actual DIE parent.

That gives us the qualified name baz in both cases.  Adding some debugging in
handle_deferred_dies shows us why:
...
  0x00000000000000d4 0x36b5f10 (0xd3)
  0x00000000000000de 0x36b5f40 (0xdd)
  0x00000000000000f5 0x36b5f10 (0xd3)
  0x0000000000000101 0x0
  0x0000000000000126 0x36b5fd0 (0x125)
  0x0000000000000173 0x0
  0x0000000000000174 0x36b6060 (0x173)
  0x0000000000000187 0x0
Resolve deferred: 0x140 -> 0x173: no parent
...

We try to find the find the parent of 0x140, by looking up the parent of 0x173
in m_die_range_map, which is nullptr, in other words no parent.

Fix this by recording more parent relations in m_die_range_map, such that we
have:
...
  0x00000000000000d4 0x33882e0 (0xd3)
  0x00000000000000de 0x3388310 (0xdd)
  0x00000000000000f5 0x33882e0 (0xd3)
  0x0000000000000101 0x0
  0x0000000000000125 0x33882e0 (0xd3)
  0x0000000000000126 0x33883a0 (0x125)
  0x0000000000000173 0x3388310 (0xdd)
  0x0000000000000174 0x3388430 (0x173)
  0x0000000000000187 0x0
Resolve deferred: 0x140 -> 0x173: 0xdd
...

This gives us the desired outcome of N1::C1::baz for both DIEs.

Tested on x86_64-linux.
---
 gdb/dwarf2/read.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 40248890c38..5ce0786e5bd 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16582,6 +16582,8 @@ cooked_indexer::index_dies (cutu_reader *reader,
       cooked_index_flag flags = IS_STATIC;
       sect_offset sibling {};
       const cooked_index_entry *this_parent_entry = parent_entry;
+      if (abbrev->tag == DW_TAG_inlined_subroutine)
+	this_parent_entry = nullptr;
       info_ptr = scan_attributes (reader->cu->per_cu, reader, info_ptr,
 				  info_ptr, abbrev, &name, &linkage_name,
 				  &flags, &sibling, &this_parent_entry,
@@ -16606,9 +16608,13 @@ cooked_indexer::index_dies (cutu_reader *reader,
 		this_die, name, defer, abbrev->tag, flags
 	      });
 	  else
-	    this_entry = m_index_storage->add (this_die, abbrev->tag, flags,
-					       name, this_parent_entry,
-					       m_per_cu);
+	    {
+	      CORE_ADDR addr = form_addr (this_die, reader->cu->per_cu->is_dwz);
+	      set_parent (addr, addr, this_parent_entry);
+	      this_entry = m_index_storage->add (this_die, abbrev->tag, flags,
+						 name, this_parent_entry,
+						 m_per_cu);
+	    }
 	}
 
       if (linkage_name != nullptr)
@@ -16669,7 +16675,8 @@ cooked_indexer::index_dies (cutu_reader *reader,
 
 	    case DW_TAG_subprogram:
 	      if ((m_language == language_fortran
-		   || m_language == language_ada)
+		   || m_language == language_ada
+		   || m_language == language_cplus)
 		  && this_entry != nullptr)
 		{
 		  info_ptr = recurse (reader, info_ptr, this_entry, true);
-- 
2.35.3

[RFC 7/7] [gdb/symtab] Resolve deferred entries, inter-shard case

[Tom de Vries]

When running test-case gdb.cp/breakpoint-locs.exp with target board
cc-with-dwz, we occasionally run into:
...
FAIL: gdb.cp/breakpoint-locs.exp: break N1::C1::baz
...
due to the breakpoint having one instead of two locations.

By using "maint set worker-threads 0" we can make the test-case fail reliably.

In the failing case the inlined_subroutine DIE at 0x1cf has an incorrect
parent, and the one at 0x148 has the correct parent:
...
    [7] ((cooked_index_entry *) 0x7fdf800030b0)
    name:       baz
    canonical:  baz
    qualified:  N1::C1::baz
    DWARF tag:  DW_TAG_inlined_subroutine
    flags:      0x0 []
    DIE offset: 0x148
    parent:     ((cooked_index_entry *) 0x7fdf80002ed0) [C1]
  ...
    [20] ((cooked_index_entry *) 0x7fdf84005340)
    name:       baz
    canonical:  baz
    qualified:  baz
    DWARF tag:  DW_TAG_inlined_subroutine
    flags:      0x0 []
    DIE offset: 0x1cf
    parent:     ((cooked_index_entry *) 0)
...
and in the passing case it's the other way around.

The corresponding dwarf for 0x148 is this:
...
 <0><b>: Abbrev Number: 34 (DW_TAG_partial_unit)
 <1><14>: Abbrev Number: 37 (DW_TAG_namespace)
    <15>   DW_AT_name        : N1
 <2><19>: Abbrev Number: 36 (DW_TAG_class_type)
    <1a>   DW_AT_name        : C1
 <3><20>: Abbrev Number: 32 (DW_TAG_subprogram)
    <21>   DW_AT_external    : 1
    <21>   DW_AT_name        : baz
    <27>   DW_AT_linkage_name: _ZN2N12C13bazEv
    <2b>   DW_AT_accessibility: 1       (public)
    <2c>   DW_AT_declaration : 1
 <1><2e>: Abbrev Number: 35 (DW_TAG_subprogram)
    <2f>   DW_AT_specification: <0x20>
    <30>   DW_AT_inline      : 3        (declared as inline and inlined)
 <0><ee>: Abbrev Number: 16 (DW_TAG_compile_unit)
    <f3>   DW_AT_language    : 4        (C++)
    <f4>   DW_AT_name        : gdb.cp/breakpoint-locs.cc
 <1><13b>: Abbrev Number: 25 (DW_TAG_subprogram)
    <13c>   DW_AT_specification: <0x115>
    <13d>   DW_AT_low_pc      : 0x4004d7
    <145>   DW_AT_high_pc     : 16
    <146>   DW_AT_frame_base  : 1 byte block: 9c        (DW_OP_call_frame_cfa)
    <148>   DW_AT_GNU_all_call_sites: 1
 <2><148>: Abbrev Number: 24 (DW_TAG_inlined_subroutine)
    <149>   DW_AT_abstract_origin: <0x2e>
    <14d>   DW_AT_low_pc      : 0x4004db
    <155>   DW_AT_high_pc     : 9
    <156>   DW_AT_call_file   : 1
    <157>   DW_AT_call_line   : 22
...
and for 0x1cf it's similar but in a CU with name gdb.cp/breakpoint-locs-2.cc.

In the target board unix case, the test-case passes because both CUs are
expanded, because each CU contains the equivalent of 0x2e.

But in the dwz case, dwz factored out that DIE into the PU, so that mechanism
no longer works, and instead we rely on the DW_TAG_inlined_subroutine DIEs to
trigger the expansion, which requires that the parent of each DIE is correct.

[ Note that also in the partial symtab we relied on the
DW_TAG_inlined_subroutine DIEs, see commit f9b5d5ea18a ("[gdb/symtab] Fix
missing breakpoint location for inlined function"). ]

By switching on debug_handle_deferred_entries, we get:
...
  0x0000000000000015 0x2708cf0 (0x14)
  0x000000000000001a 0x2708d20 (0x19)
  0x000000000000002d 0x2708cf0 (0x14)
  0x000000000000002e 0x2708d20 (0x19)
  0x000000000000002f 0x2708d50 (0x2e)
  0x0000000000000039 0x0
  0x000000000000010f 0x2708e10 (0x10e)
  0x0000000000000121 0x0
  0x000000000000013b 0x2708e10 (0x10e)
  0x000000000000013c 0x2708ea0 (0x13b)
  0x0000000000000148 0x2708d20 (0x19)
  0x0000000000000149 0x2708ea0 (0x13b)
  0x000000000000016d 0x0
  0x000000000000019a 0x2708f60 (0x199)
  0x00000000000001ac 0x0
  0x00000000000001c2 0x2708f60 (0x199)
  0x00000000000001c3 0x2708ff0 (0x1c2)
  0x00000000000001f4 0x0
...

So, 0x148 has parent 0x19, in other words N1::C1, and 0x1cf, in other words bar.
The parent of 0x1cf is incorrect, but it was never used.  Instead, at the point
the DIE was indexed, find_parent was called, which returned nullptr, and that
was used instead.  Which is also incorrect.

The problem is that m_die_range_map only has the scope of a parsing a single CU.
For the first CU, that includes the PU as well, but not for the second.

[ Then there's the problem that find_parent cannot distinguish between the
"no parent" and "don't know" cases, which means that getting the incorrect
parent happens silently. ]

We need to generically solve the case of inter-CU dependencies, and that includes
inter-shard dependencies as well.

So we:
- mark deferred entries in m_die_range_map using dummy cooked_index_entry
  parent_map::defered, to prevent incorrect entries for deferred dies,
- defer all inter-CU dependencies (and consequently both 0x148 and 0x1cf are
  deferred),
- move m_die_range_map and m_deferred_dies to cooked_index_shard, and
- move handle_deferred_dies to the cooked_index, where it is called in the
  constructor, and update it to handle the intra-shard case,
such that we get:
...
  0x0000000000000015 0x4623d10 (0x14)
  0x000000000000001a 0x4623d40 (0x19)
  0x000000000000002d 0x4623d10 (0x14)
  0x000000000000002e 0x4623d40 (0x19)
  0x000000000000002f 0x4623d70 (0x2e)
  0x0000000000000039 0x0
  0x000000000000010f 0x4623e30 (0x10e)
  0x0000000000000121 0x0
  0x000000000000013b 0x4623e30 (0x10e)
  0x000000000000013c 0x4623ec0 (0x13b)
  0x0000000000000148 0xffffffffffffffff (deferred)
  0x0000000000000149 0x4623ec0 (0x13b)
  0x000000000000016d 0x0
  0x000000000000019a 0x4623f50 (0x199)
  0x00000000000001ac 0x0
  0x00000000000001c2 0x4623f50 (0x199)
  0x00000000000001c3 0x4623fe0 (0x1c2)
  0x00000000000001cf 0xffffffffffffffff (deferred)
  0x00000000000001d0 0x4623fe0 (0x1c2)
  0x00000000000001f4 0x0
Resolve deferred: 0x148 -> 0x2e: 0x19
Resolve deferred: 0x1cf -> 0x2e: 0x19
...

Note that handling units from the .debug_info section alongside units from the
.debug_types section requires us to extend form_addr to take is_debug_types
into account.

Tested on x86_64-linux.

PR symtab/30728
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30728
---
 gdb/dwarf2/cooked-index.c |  60 +++++++++++++++++++
 gdb/dwarf2/cooked-index.h |  65 ++++++++++++++++++++-
 gdb/dwarf2/read.c         | 118 +++++++++++++++++---------------------
 3 files changed, 177 insertions(+), 66 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index 351f0016402..9cc9ce7fb3e 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -228,6 +228,12 @@ cooked_index_entry::write_scope (struct obstack *storage,
 
 /* See cooked-index.h.  */
 
+cooked_index_entry parent_map::deferred((sect_offset)0, (dwarf_tag)0,
+					(cooked_index_flag)0, nullptr,
+					nullptr, nullptr);
+
+/* See cooked-index.h.  */
+
 const cooked_index_entry *
 cooked_index_shard::add (sect_offset die_offset, enum dwarf_tag tag,
 			 cooked_index_flag flags, const char *name,
@@ -446,6 +452,8 @@ cooked_index_shard::wait (bool allow_quit) const
 cooked_index::cooked_index (vec_type &&vec)
   : m_vector (std::move (vec))
 {
+  handle_deferred_entries ();
+
   for (auto &idx : m_vector)
     idx->finalize ();
 
@@ -650,6 +658,58 @@ cooked_index::maybe_write_index (dwarf2_per_bfd *per_bfd,
   global_index_cache.store (per_bfd, ctx);
 }
 
+/* See cooked-index.h.  */
+
+void
+cooked_index::handle_deferred_entries ()
+{
+  const bool debug_handle_deferred_entries = false;
+  if (debug_handle_deferred_entries)
+    for (auto &shard : m_vector)
+      shard->m_die_range_map->dump ();
+
+  for (auto &shard : m_vector)
+    for (const auto &entry : *shard->m_deferred_entries)
+      {
+	const cooked_index_entry *parent_entry = nullptr;
+	for (auto &shard_2 : m_vector)
+	  {
+	    auto res = shard_2->find_parent (entry.spec_offset);
+	    if (res != nullptr)
+	      {
+		/* We currently assume that no derrered entry is
+		   dependent on another deferred entry.  If that turns
+		   out to be not the case, detect it here.  */
+		gdb_assert (res != &parent_map::deferred);
+		parent_entry = res;
+		break;
+	      }
+	  }
+	if (debug_handle_deferred_entries)
+	  {
+	    gdb_printf (gdb_stdlog,
+			"Resolve deferred: 0x%" PRIx64 " -> 0x%lx: ",
+			to_underlying (entry.die_offset),
+			entry.spec_offset);
+	    if (parent_entry == nullptr)
+	      gdb_printf (gdb_stdlog, "no parent\n");
+	    else
+	      gdb_printf (gdb_stdlog, "0x%" PRIx64 "\n",
+			  to_underlying (parent_entry->die_offset));
+	  }
+	shard->add (entry.die_offset, entry.tag, entry.flags,
+		    entry.name, parent_entry, entry.per_cu);
+      }
+
+  for (auto &shard : m_vector)
+    {
+      delete shard->m_die_range_map;
+      shard->m_die_range_map = nullptr;
+      delete shard->m_deferred_entries;
+      shard->m_deferred_entries = nullptr;
+    }
+}
+
 /* Wait for all the index cache entries to be written before gdb
    exits.  */
 static void
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 457ee819eac..d71688cc382 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -242,6 +242,10 @@ struct cooked_index_entry : public allocate_on_obstack
 class parent_map
 {
 public:
+  /* A dummy cooked_index_entry to mark that computing the parent has been
+     deferred.  */
+  static cooked_index_entry deferred;
+
   /* Find the parent of DIE LOOKUP.  */
   const cooked_index_entry *find_parent (CORE_ADDR lookup)
   {
@@ -276,6 +280,13 @@ class parent_map
 	  = (const cooked_index_entry *)value;
 	if (parent_entry == nullptr)
 	  return;
+
+	if (parent_entry == &parent_map::deferred)
+	  {
+	    gdb_printf (outfile, " (deferred)");
+	    return;
+	  }
+
 	gdb_printf (outfile, " (0x%" PRIx64 ")",
 		    to_underlying (parent_entry->die_offset));
       };
@@ -300,7 +311,11 @@ class cooked_index;
 class cooked_index_shard
 {
 public:
-  cooked_index_shard () = default;
+  cooked_index_shard () {
+    m_die_range_map = new parent_map;
+    m_deferred_entries = new std::vector<deferred_entry>;
+  }
+
   DISABLE_COPY_AND_ASSIGN (cooked_index_shard);
 
   /* Create a new cooked_index_entry and register it with this object.
@@ -344,6 +359,37 @@ class cooked_index_shard
      for completion, will be returned.  */
   range find (const std::string &name, bool completing) const;
 
+  /* Find the parent of DIE LOOKUP.  */
+  const cooked_index_entry *
+  find_parent (CORE_ADDR lookup)
+  {
+    return m_die_range_map->find_parent (lookup);
+  }
+
+  /* Set the parent of DIES in range [START, END] to PARENT_ENTRY.  */
+  void set_parent (CORE_ADDR start, CORE_ADDR end,
+		   const cooked_index_entry *parent_entry)
+  {
+    m_die_range_map->set_parent (start, end, parent_entry);
+  }
+
+  /* A single deferred entry.  See m_deferred_entries.  */
+  struct deferred_entry
+  {
+    sect_offset die_offset;
+    const char *name;
+    CORE_ADDR spec_offset;
+    dwarf_tag tag;
+    cooked_index_flag flags;
+    dwarf2_per_cu_data *per_cu;
+  };
+
+  /* Defer creating a cooked_index_entry corresponding to DEFERRED.  */
+  void defer_entry (deferred_entry de)
+  {
+    m_deferred_entries->push_back (de);
+  }
+
 private:
 
   /* Return the entry that is believed to represent the program's
@@ -401,6 +447,20 @@ class cooked_index_shard
      that the 'get' method is never called on this future, only
      'wait'.  */
   gdb::future<void> m_future;
+
+  /* An addrmap that maps from section offsets (see the form_addr
+     method) to newly-created entries.  See m_deferred_entries to
+     understand this.  */
+  parent_map *m_die_range_map;
+
+  /* The generated DWARF can sometimes have the declaration for a
+     method in a class (or perhaps namespace) scope, with the
+     definition appearing outside this scope... just one of the many
+     bad things about DWARF.  In order to handle this situation, we
+     defer certain entries until the end of scanning, at which point
+     we'll know the containing context of all the DIEs that we might
+     have scanned.  This vector stores these deferred entries.  */
+  std::vector<deferred_entry> *m_deferred_entries;
 };
 
 /* The main index of DIEs.  The parallel DIE indexers create
@@ -484,6 +544,9 @@ class cooked_index : public dwarf_scanner_base
 
 private:
 
+  /* Create cooked_index_entries for the deferred entries.  */
+  void handle_deferred_entries ();
+
   /* Maybe write the index to the index cache.  */
   void maybe_write_index (dwarf2_per_bfd *per_bfd,
 			  const index_cache_store_context &);
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 5ce0786e5bd..8bb0530506b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -4670,6 +4670,25 @@ class cooked_index_storage
     return &m_addrmap;
   }
 
+  /* Find the parent of DIE LOOKUP.  */
+  const cooked_index_entry *find_parent (CORE_ADDR lookup)
+  {
+    return m_index->find_parent (lookup);
+  }
+
+  /* Set the parent of DIES in range [START, END] to PARENT_ENTRY.  */
+  void set_parent (CORE_ADDR start, CORE_ADDR end,
+		   const cooked_index_entry *parent_entry)
+  {
+    m_index->set_parent (start, end, parent_entry);
+  }
+
+  /* Defer creating a cooked_index_entry corresponding to DEFERRED.  */
+  void defer_entry (cooked_index_shard::deferred_entry de)
+  {
+    m_index->defer_entry (de);
+  }
+
 private:
 
   /* Hash function for a cutu_reader.  */
@@ -4722,11 +4741,13 @@ class cooked_indexer
 
   /* A helper function to turn a section offset into an address that
      can be used in an addrmap.  */
-  CORE_ADDR form_addr (sect_offset offset, bool is_dwz)
+  CORE_ADDR form_addr (sect_offset offset, bool is_dwz, bool is_debug_types)
   {
     CORE_ADDR value = to_underlying (offset);
     if (is_dwz)
       value |= ((CORE_ADDR) 1) << (8 * sizeof (CORE_ADDR) - 1);
+    if (is_debug_types)
+      value |= ((CORE_ADDR) 1) << (8 * sizeof (CORE_ADDR) - 2);
     return value;
   }
 
@@ -4802,68 +4823,20 @@ class cooked_indexer
   /* Find the parent of DIE LOOKUP.  */
   const cooked_index_entry *find_parent (CORE_ADDR lookup)
   {
-    return m_die_range_map.find_parent (lookup);
+    return m_index_storage->find_parent (lookup);
   }
 
   /* Set the parent of DIES in range [START, END] to PARENT_ENTRY.  */
   void set_parent (CORE_ADDR start, CORE_ADDR end,
 		   const cooked_index_entry *parent_entry)
   {
-    m_die_range_map.set_parent (start, end, parent_entry);
+    m_index_storage->set_parent (start, end, parent_entry);
   }
 
-  /* A single deferred entry.  */
-  struct deferred_entry
-  {
-    sect_offset die_offset;
-    const char *name;
-    CORE_ADDR spec_offset;
-    dwarf_tag tag;
-    cooked_index_flag flags;
-  };
-
-  /* The generated DWARF can sometimes have the declaration for a
-     method in a class (or perhaps namespace) scope, with the
-     definition appearing outside this scope... just one of the many
-     bad things about DWARF.  In order to handle this situation, we
-     defer certain entries until the end of scanning, at which point
-     we'll know the containing context of all the DIEs that we might
-     have scanned.  This vector stores these deferred entries.  */
-  std::vector<deferred_entry> m_deferred_entries;
-
   /* Defer creating a cooked_index_entry corresponding to DEFERRED.  */
-  void defer_entry (deferred_entry de)
+  void defer_entry (cooked_index_shard::deferred_entry de)
   {
-    m_deferred_entries.push_back (de);
-  }
-
-  /* Create cooked_index_entries for the deferred entries.  */
-  void handle_deferred_entries ()
-  {
-    const bool debug_handle_deferred_entries = false;
-
-    if (debug_handle_deferred_entries)
-      m_die_range_map.dump ();
-
-    for (const auto &entry : m_deferred_entries)
-      {
-	const cooked_index_entry *parent_entry
-	  = find_parent (entry.spec_offset);
-	if (debug_handle_deferred_entries)
-	  {
-	    gdb_printf (gdb_stdlog,
-			"Resolve deferred: 0x%" PRIx64 " -> 0x%lx: ",
-			to_underlying (entry.die_offset),
-			entry.spec_offset);
-	    if (parent_entry == nullptr)
-	      gdb_printf (gdb_stdlog, "no parent\n");
-	    else
-	      gdb_printf (gdb_stdlog, "0x%" PRIx64 "\n",
-			  to_underlying (parent_entry->die_offset));
-	  }
-	m_index_storage->add (entry.die_offset, entry.tag, entry.flags,
-			      entry.name, parent_entry, m_per_cu);
-      }
+    m_index_storage->defer_entry (de);
   }
 };
 
@@ -16407,13 +16380,22 @@ cooked_indexer::scan_attributes (dwarf2_per_cu_data *scanning_per_cu,
 	  const gdb_byte *new_info_ptr = (new_reader->buffer
 					  + to_underlying (origin_offset));
 
-	  if (new_reader->cu == reader->cu
-	      && new_info_ptr > watermark_ptr
+	  if ((new_reader->cu != reader->cu
+	       || (new_reader->cu == reader->cu
+		   && new_info_ptr > watermark_ptr))
 	      && *parent_entry == nullptr)
-	    *maybe_defer = form_addr (origin_offset, origin_is_dwz);
+	    {
+	      gdb_assert (reader->cu->per_cu->is_debug_types
+			  == new_reader->cu->per_cu->is_debug_types);
+	      *maybe_defer = form_addr (origin_offset, origin_is_dwz,
+					reader->cu->per_cu->is_debug_types);
+	    }
 	  else if (*parent_entry == nullptr)
 	    {
-	      CORE_ADDR lookup = form_addr (origin_offset, origin_is_dwz);
+	      gdb_assert (reader->cu->per_cu->is_debug_types
+			  == new_reader->cu->per_cu->is_debug_types);
+	      CORE_ADDR lookup = form_addr (origin_offset, origin_is_dwz,
+					    reader->cu->per_cu->is_debug_types);
 	      *parent_entry = find_parent (lookup);
 	    }
 
@@ -16533,9 +16515,11 @@ cooked_indexer::recurse (cutu_reader *reader,
       /* Both start and end are inclusive, so use both "+ 1" and "- 1" to
 	 limit the range to the children of parent_entry.  */
       CORE_ADDR start = form_addr (parent_entry->die_offset + 1,
-				   reader->cu->per_cu->is_dwz);
+				   reader->cu->per_cu->is_dwz,
+				   reader->cu->per_cu->is_debug_types);
       CORE_ADDR end = form_addr (sect_offset (info_ptr - 1 - reader->buffer),
-				 reader->cu->per_cu->is_dwz);
+				 reader->cu->per_cu->is_dwz,
+				 reader->cu->per_cu->is_debug_types);
       set_parent (start, end, parent_entry);
     }
 
@@ -16604,12 +16588,18 @@ cooked_indexer::index_dies (cutu_reader *reader,
       if (name != nullptr)
 	{
 	  if (defer != 0)
-	    defer_entry ({
-		this_die, name, defer, abbrev->tag, flags
-	      });
+	    {
+	      CORE_ADDR addr = form_addr (this_die, reader->cu->per_cu->is_dwz,
+					  reader->cu->per_cu->is_debug_types);
+	      set_parent (addr, addr, &parent_map::deferred);
+	      defer_entry ({
+		  this_die, name, defer, abbrev->tag, flags, m_per_cu
+		});
+	    }
 	  else
 	    {
-	      CORE_ADDR addr = form_addr (this_die, reader->cu->per_cu->is_dwz);
+	      CORE_ADDR addr = form_addr (this_die, reader->cu->per_cu->is_dwz,
+					  reader->cu->per_cu->is_debug_types);
 	      set_parent (addr, addr, this_parent_entry);
 	      this_entry = m_index_storage->add (this_die, abbrev->tag, flags,
 						 name, this_parent_entry,
@@ -16713,8 +16703,6 @@ cooked_indexer::make_index (cutu_reader *reader)
   if (!reader->comp_unit_die->has_children)
     return;
   index_dies (reader, reader->info_ptr, nullptr, false);
-
-  handle_deferred_entries ();
 }
 
 /* An implementation of quick_symbol_functions for the cooked DWARF
-- 
2.35.3

Re: [RFC 0/7] [gdb/symtab, cc-with-dwz] Fix gdb.cp/breakpoint-locs.exp

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Currently resolving deferred entries is done in the parallel for that
Tom> computes the cooked index.  The last patch moves resolving of deferred entries
Tom> out of the parallel for.

Tom> We can add two optimizations to move some of that computation back into the
Tom> parallel for:
Tom> - resolve intra-shard dependencies at the end of generating the shard, and
Tom> - don't defer backward intra-shard dependencies.

Tom> Both of these require to keep track of the range of DIEs that was indexed
Tom> in the shard, because currently the parent tracking cannot distuinguish
Tom> between the "no parent" and "don't know" cases.

Right now, we let "CUs" race on reading a PU -- whichever thread happens
to need a PU first does the reading and the other ones just, IIRC, mark
the dependency and move on.

But rather than move these data structures into the shards, would it be
possible to have all CUs wait for the PU to be read and keep a reference
to it?  Then the name computation can still be done during scanning.

The benefit of this is that it means that only the bad case has to
really pay for the overhead.  That is, dwz is uncommon and efforts to
support it shouldn't penalize normal DWARF reading.

Insert a rant here about how DWARF is awful... there is no real reason
any of this has to work this way or be so difficult.

Tom

Re: [RFC 0/7] [gdb/symtab, cc-with-dwz] Fix gdb.cp/breakpoint-locs.exp

[Tom de Vries]

On 9/8/23 20:56, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> Currently resolving deferred entries is done in the parallel for that
> Tom> computes the cooked index.  The last patch moves resolving of deferred entries
> Tom> out of the parallel for.
> 
> Tom> We can add two optimizations to move some of that computation back into the
> Tom> parallel for:
> Tom> - resolve intra-shard dependencies at the end of generating the shard, and
> Tom> - don't defer backward intra-shard dependencies.
> 
> Tom> Both of these require to keep track of the range of DIEs that was indexed
> Tom> in the shard, because currently the parent tracking cannot distuinguish
> Tom> between the "no parent" and "don't know" cases.
> 
> Right now, we let "CUs" race on reading a PU -- whichever thread happens
> to need a PU first does the reading

Correct, and the entries read from the PU become part of the CU that 
wins the race.

Easier to spot if we add:
...
diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index d6531260c86..59b377751de 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -592,6 +592,8 @@ cooked_index::dump (gdbarch *arch) const
        else
  	gdb_printf ("    parent:     ((cooked_index_entry *) 0)\n");

+      gdb_printf ("    CU:         %s\n", sect_offset_str 
(entry->per_cu->sect_off));
+
        gdb_printf ("\n");
      }

...

Then we have (for test-case gdb.cp/breakpoint-locs.exp and target board 
cc-with-dwz):
...
     [13] ((cooked_index_entry *) 0x7f6cfc005280)
     name:       C1
     canonical:  C1
     qualified:  N1::C1
     DWARF tag:  DW_TAG_class_type
     flags:      0x0 []
     DIE offset: 0x19
     parent:     ((cooked_index_entry *) 0x7f6cfc005250) [N1]
     CU:         0x16e
...
Note the discrepancy between the DIE offset and the CU.

For now I'm assuming that this is intentional.  While not fully 
reflecting the dwarfs meaning, it's a way to ensure that we only read 
and store the PU once.  The side-effect of this is that we can no longer 
tell in how many CUs a PU is included (or more concretely, how many CUs 
contain a certain type), which is why for this test-case we need the fix 
to handle DW_TAG_inlined_subroutine DIEs (because they sticks around in 
the CU and cannot be moved to the PU).  We had the exact same issue with 
partial symtabs.

[ Note btw that cc-with-debug-types does it differently.  Even though 
the definition of the class C1 is moved to a TU in .debug_types, both 
CUs keep an incomplete declaration of the class, referencing the TU, 
which results in two entries in the cooked index.  Which is why the 
test-case passes for cc-with-debug-types. ]

> and the other ones just, IIRC, mark
> the dependency and move on.
> 

Yes, though AFAICT that dependency is not used in any way in the 
expansion process.

> But rather than move these data structures into the shards, would it be
> possible to have all CUs wait for the PU to be read and keep a reference
> to it?  Then the name computation can still be done during scanning.
> 

I'm a bit confused about which problem you're trying to solve.

We can remove the import race, and have the cooked index entries from 
the PU have the PU as per_cu, and track reverse dependencies, and when 
expanding the PU trigger the expansion of the closure of all including 
CU/PUs, fully reflecting the meaning of the dwarf.

AFAIU, that would mean we no longer need the DW_TAG_inlined_subroutine 
bit for this particular test-case (but perhaps for others, I'm not sure).

But that does not address the parent entry calculation.  In the simplest 
case, the parent of an entry is the DIE parent, and it's trivial to 
handle.  In case the DIE parent is ignored, and the parent is derived by 
following abstract_origin chains, and we end up with a parent that is 
not processed yet, we simply have to defer.  And if the parent is in 
another shard, we have to defer until after the parallel for (or 
alternatively, wait for other shards to be done, though I'm not sure 
that level of complexity is worth it).

The problem of incorrect parent entry calculation is not very visible 
atm.  If the parent entry calculation is incorrect, it quietly defaults 
to no parent.  And most of the time, that doesn't result in incorrect 
gdb behaviour.  This test-case is the exception.  Which we can work 
around by handling PUs differently.  But we'll still quietly calculate 
incorrect parent entries, unless we fix the generic case, as this patch 
series proposes to do.

> The benefit of this is that it means that only the bad case has to
> really pay for the overhead.  That is, dwz is uncommon and efforts to
> support it shouldn't penalize normal DWARF reading.
> 

I don't understand how the patch series I'm currently proposing 
penalizes normal DWARF reading, at least not once the two optimizations 
I proposed in the cover letter are implemented (see "two optimizations" 
above).

Thanks,
- Tom

> Insert a rant here about how DWARF is awful... there is no real reason
> any of this has to work this way or be so difficult.
> 
> Tom

Re: [RFC 0/7] [gdb/symtab, cc-with-dwz] Fix gdb.cp/breakpoint-locs.exp

[Tom de Vries]

On 9/12/23 17:23, Tom de Vries wrote:
> But that does not address the parent entry calculation.  In the simplest 
> case, the parent of an entry is the DIE parent, and it's trivial to 
> handle.  In case the DIE parent is ignored, and the parent is derived by 
> following abstract_origin chains, and we end up with a parent that is 
> not processed yet, we simply have to defer.  And if the parent is in 
> another shard, we have to defer until after the parallel for (or 
> alternatively, wait for other shards to be done, though I'm not sure 
> that level of complexity is worth it).
> 
> The problem of incorrect parent entry calculation is not very visible 
> atm.  If the parent entry calculation is incorrect, it quietly defaults 
> to no parent.  And most of the time, that doesn't result in incorrect 
> gdb behaviour.  This test-case is the exception.  Which we can work 
> around by handling PUs differently.  But we'll still quietly calculate 
> incorrect parent entries, unless we fix the generic case, as this patch 
> series proposes to do.

I managed to write a test-case that demonstrates this problem 
independently of the gdb.cp/breakpoint-locs.exp test-case, filed here ( 
https://sourceware.org/bugzilla/show_bug.cgi?id=30846 ).

Thanks,
- Tom

Re: [RFC 0/7] [gdb/symtab, cc-with-dwz] Fix gdb.cp/breakpoint-locs.exp

[Tom de Vries]

On 9/13/23 16:37, Tom de Vries via Gdb-patches wrote:
> On 9/12/23 17:23, Tom de Vries wrote:
>> But that does not address the parent entry calculation.  In the 
>> simplest case, the parent of an entry is the DIE parent, and it's 
>> trivial to handle.  In case the DIE parent is ignored, and the parent 
>> is derived by following abstract_origin chains, and we end up with a 
>> parent that is not processed yet, we simply have to defer.  And if the 
>> parent is in another shard, we have to defer until after the parallel 
>> for (or alternatively, wait for other shards to be done, though I'm 
>> not sure that level of complexity is worth it).
>>
>> The problem of incorrect parent entry calculation is not very visible 
>> atm.  If the parent entry calculation is incorrect, it quietly 
>> defaults to no parent.  And most of the time, that doesn't result in 
>> incorrect gdb behaviour.  This test-case is the exception.  Which we 
>> can work around by handling PUs differently.  But we'll still quietly 
>> calculate incorrect parent entries, unless we fix the generic case, as 
>> this patch series proposes to do.
> 
> I managed to write a test-case that demonstrates this problem 
> independently of the gdb.cp/breakpoint-locs.exp test-case, filed here ( 
> https://sourceware.org/bugzilla/show_bug.cgi?id=30846 ).
> 

I've submitted a patch series to fix this ( 
https://sourceware.org/pipermail/gdb-patches/2023-October/202882.html ).

Thanks,
- Tom