diff mbox series

[01/13,gdb/symtab] Factor out m_die_range_map usage

Message ID 20231002125051.29911-2-tdevries@suse.de
State Superseded
Headers
Series Fix gdb.cp/breakpoint-locs.exp |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 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 Oct. 2, 2023, 12:50 p.m. UTC 
  Factor out usage of cooked_indexer::m_die_range_map into new class parent_map
with member functions find_parent and set_parent.

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

Comments

Alexandra Petlanova Hajkova Oct. 9, 2023, 8:33 p.m. UTC | #1 
On Mon, Oct 2, 2023 at 2:53 PM Tom de Vries via Gdb-patches <
gdb-patches@sourceware.org> wrote:

> Factor out usage of cooked_indexer::m_die_range_map into new class
> parent_map
> with member functions find_parent and set_parent.
>
> Tested on x86_64-linux.
> ---
>  gdb/dwarf2/cooked-index.h | 22 ++++++++++++++++++++++
>  gdb/dwarf2/read.c         | 23 +++++++++++++++++------
>  2 files changed, 39 insertions(+), 6 deletions(-)
>
>
> It seems breakpoint-locs.exp test does not fail anymore with this patch
applied on ppc64le Fedora Rawhide. But I wouldn't expect that
from just a factoring out. Is that a coincidence?
< Breakpoint 1 at 0x10010208: file
/root/build/gdb/testsuite/../../../binutils-g
db/gdb/testsuite/gdb.cp/breakpoint-locs.h, line 23.
< (gdb) FAIL: gdb.cp/breakpoint-locs.exp: break N1::C1::baz
< testcase
/root/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.cp/breakpoint-locs.exp
completed in 0 seconds
---
> Breakpoint 1 at 0x10010208: N1::C1::baz. (2 locations)
> (gdb) PASS: gdb.cp/breakpoint-locs.exp: break N1::C1::baz
> testcase
/root/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.cp/breakpoint-locs.exp
completed in 1 seconds
Tom de Vries Oct. 9, 2023, 9:43 p.m. UTC | #2 
On 10/9/23 22:33, Alexandra Petlanova Hajkova wrote:
> On Mon, Oct 2, 2023 at 2:53 PM Tom de Vries via Gdb-patches 
> <gdb-patches@sourceware.org <mailto:gdb-patches@sourceware.org>> wrote:
> 
>     Factor out usage of cooked_indexer::m_die_range_map into new class
>     parent_map
>     with member functions find_parent and set_parent.
> 
>     Tested on x86_64-linux.
>     ---
>       gdb/dwarf2/cooked-index.h | 22 ++++++++++++++++++++++
>       gdb/dwarf2/read.c         | 23 +++++++++++++++++------
>       2 files changed, 39 insertions(+), 6 deletions(-)
> 
> 
> It seems breakpoint-locs.exp test does not fail anymore with this patch 
> applied on ppc64le Fedora Rawhide. But I wouldn't expect that > from just a factoring out. Is that a coincidence?

Hi,

if you're talking about target board unix, then it's a failure I'm 
unaware of, and indeed I wouldn't expect this patch to change anything.

If you're talking about target board cc-with-dwz or cc-with-dwz-m, then 
you might be looking at nondeterministic behaviour of the cooked index, 
due to the race between 2 CUs to read a PU.  Using "maint set 
worker-threads 0" avoids that race.  For me on x86_64-linux that makes 
the failure reproducible.

Thanks,
- Tom


> < Breakpoint 1 at 0x10010208: file 
> /root/build/gdb/testsuite/../../../binutils-g
> db/gdb/testsuite/gdb.cp/breakpoint-locs.h, line 23.
> < (gdb) FAIL: gdb.cp/breakpoint-locs.exp: break N1::C1::baz
> < testcase 
> /root/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.cp/breakpoint-locs.exp completed in 0 seconds
> ---
>  > Breakpoint 1 at 0x10010208: N1::C1::baz. (2 locations)
>  > (gdb) PASS: gdb.cp/breakpoint-locs.exp: break N1::C1::baz
>  > testcase 
> /root/build/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.cp/breakpoint-locs.exp completed in 1 seconds
Tom Tromey Oct. 20, 2023, 7:50 p.m. UTC | #3 
>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Factor out usage of cooked_indexer::m_die_range_map into new class parent_map
Tom> with member functions find_parent and set_parent.

Tom> +  /* Find the parent of DIE LOOKUP.  */
Tom> +  const cooked_index_entry *find_parent (CORE_ADDR lookup) const
Tom> +  {

I sort of regret doing this stuff using CORE_ADDR.  I think the API
would be better if 'form_addr' was handled privately in the wrapper
class.

Tom
Tom de Vries Nov. 30, 2023, 12:55 p.m. UTC | #4 
On 10/20/23 21:50, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> Factor out usage of cooked_indexer::m_die_range_map into new class parent_map
> Tom> with member functions find_parent and set_parent.
> 
> Tom> +  /* Find the parent of DIE LOOKUP.  */
> Tom> +  const cooked_index_entry *find_parent (CORE_ADDR lookup) const
> Tom> +  {
> 
> I sort of regret doing this stuff using CORE_ADDR.  I think the API
> would be better if 'form_addr' was handled privately in the wrapper
> class.

This updated patch moves form_addr into the wrapper class, but doesn't 
make it private because there's still one remaining reference from 
outside the class.

WDYT?

Thanks,
- Tom
diff mbox series

Patch

diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 5aacb321c91..a029fcf2cc9 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) const
+  {
+    const 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 5bbc8e24cf9..17bc650a055 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) const
+  {
+    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
@@ -16356,8 +16369,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;
@@ -16479,7 +16491,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;
@@ -16652,8 +16664,7 @@  cooked_indexer::make_index (cutu_reader *reader)
 
   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);
+      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);
     }