diff mbox series

[v2,04/13,gdb/symtab] Factor out m_deferred_entries usage

Message ID 20231212173239.16793-5-tdevries@suse.de
State New
Headers
Series Fix gdb.cp/breakpoint-locs.exp |

Checks

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

Commit Message

Tom de Vries Dec. 12, 2023, 5:32 p.m. UTC 
  Factor out usage of cooked_indexer::m_deferred_entries in new member
functions defer_entry, handle_deferred_entries and resolve_deferred_entry.

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

Comments

Tom Tromey Dec. 12, 2023, 6:39 p.m. UTC | #1 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Factor out usage of cooked_indexer::m_deferred_entries in new member
Tom> functions defer_entry, handle_deferred_entries and resolve_deferred_entry.

I don't mind this, but when reading through the whole series, it seems
like the code has to now iterate over a lot of the entries trying to fix
up the parentage later.

I wonder then about just sticking this info directly into the
cooked_index_entry object, and then doing fixups directly on these in
the shard.

That is, instead of keeping separate "deferred" entries, just making
ordinary entries.  cooked_index_entry::parent_entry could be a union
holding either the parent (if known) or a CORE_ADDR; and then there
could be a new flag in cooked_index_flag_enum indicating which one is in
use.

Then I think parent_map::deferrred also would not be needed.

I'm still kind of mid-reading so my apologies if this doesn't really
make sense.

Tom
Tom de Vries Dec. 13, 2023, 8:46 a.m. UTC | #2 
On 12/12/23 19:39, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Factor out usage of cooked_indexer::m_deferred_entries in new member
> Tom> functions defer_entry, handle_deferred_entries and resolve_deferred_entry.
> 
> I don't mind this, but when reading through the whole series, it seems
> like the code has to now iterate over a lot of the entries trying to fix
> up the parentage later.
> 
> I wonder then about just sticking this info directly into the
> cooked_index_entry object, and then doing fixups directly on these in
> the shard.
> 
> That is, instead of keeping separate "deferred" entries, just making
> ordinary entries.  cooked_index_entry::parent_entry could be a union
> holding either the parent (if known) or a CORE_ADDR; and then there
> could be a new flag in cooked_index_flag_enum indicating which one is in
> use.
> 
> Then I think parent_map::deferrred also would not be needed.
> 
> I'm still kind of mid-reading so my apologies if this doesn't really
> make sense.

I've also considered something like this: rather than deferring creating 
entries, creating them immediately with some marker that work is left to 
be done.  My initial though there was to use parent_map::deferred as 
parent, and then keep lists of:
...
struct {
   CORE_ADDR get_parent_from_here;
   cooked_index_entry **patch_parent_here;
};
...

The union+flag approach would also work but doesn't offer a way to 
iterate over them quickly, which might matter for large shards.  Though 
we could do a side-table approach I suppose.

We could choose to not care about such lists (in which case you'd need 
the union+flag solution) and resolve these on demand, but then that'll 
have to take care of cycle detection, so atm I'm not convinced that's a 
good idea.

Thanks,
- Tom
Tom Tromey Dec. 13, 2023, 8:16 p.m. UTC | #3 
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> I've also considered something like this: rather than deferring
Tom> creating entries, creating them immediately with some marker that work
Tom> is left to be done.

Also worth noting, in many cases no extra work will need to be done.
In these cases the parent entry for a DIE range can be recorded directly.

Tom> The union+flag approach would also work but doesn't offer a way to
Tom> iterate over them quickly, which might matter for large shards.
Tom> Though we could do a side-table approach I suppose.

Instead of an addrmap I'm proposing a sequence of vectors, and replacing
splay-tree search with a binary search.  So the side thing is still
needed (it's these vectors), but the idea is that now the processing can
be parallelized; in fact stuck into the existing finalize stage.

Tom> We could choose to not care about such lists (in which case you'd need
Tom> the union+flag solution) and resolve these on demand, but then that'll
Tom> have to take care of cycle detection, so atm I'm not convinced that's
Tom> a good idea.

Doing it on demand means keeping the parentage information around
forever, or alternately re-reading abbrevs and then scanning DIEs at
lookup time.  I don't think we should do either of these.

Tom
diff mbox series

Patch

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 806cc5902b3..cf71d4c55ce 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -4641,6 +4641,32 @@  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_entry DE.  */
+  void defer_entry (const deferred_entry &de)
+  {
+    m_deferred_entries.push_back (de);
+  }
+
+  /* Create a cooked_index_entry corresponding to deferred_entry DE with
+     parent PARENT_ENTRY.  */
+  const cooked_index_entry *resolve_deferred_entry
+    (const deferred_entry &de, const cooked_index_entry *parent_entry)
+  {
+    return m_index_storage->add (de.die_offset, de.tag, de.flags, de.name,
+				 parent_entry, m_per_cu);
+  }
+
+  /* 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);
+	resolve_deferred_entry (entry, parent_entry);
+      }
+  }
 };
 
 /* Subroutine of dwarf2_build_psymtabs_hard to simplify it.
@@ -16371,7 +16397,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
@@ -16476,12 +16502,7 @@  cooked_indexer::make_index (cutu_reader *reader)
     return;
   index_dies (reader, reader->info_ptr, nullptr, false);
 
-  for (const auto &entry : m_deferred_entries)
-    {
-      const cooked_index_entry *parent = find_parent (entry.spec_offset);
-      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