diff mbox series

gdb/dwarf: save DWARF version in dwarf2_loclist_baton, remove it from dwarf2_per_cu

Message ID 20250303212126.728156-1-simon.marchi@efficios.com
State New
Headers
Series gdb/dwarf: save DWARF version in dwarf2_loclist_baton, remove it from dwarf2_per_cu |

Checks

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

Commit Message

Simon Marchi March 3, 2025, 9:18 p.m. UTC 
  When running:

    $ make check TESTS="gdb.cp/cpexprs-debug-types.exp" RUNTESTFLAGS="--target_board=fission"

I get:

    (gdb) break -qualified main
    /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.h:295: internal-error: version: Assertion `m_dwarf_version != 0' failed.

The problem is that dwarf2_per_cu objects created in the
read_cutu_die_from_dwo code path never have their DWARF version set.  A
seemingly obvious solution would be to add a call to
dwarf2_per_cu::set_version in there (there's a patch in the referenced
PR that does that).  However, this comment in
read_comp_units_from_section is a bit scary:

      /* Init this asap, to avoid a data race in the set_version in
	 cutu_reader::cutu_reader (which may be run in parallel for the cooked
	 index case).  */
      this_cu->set_version (cu_header.version);

I don't know if a DWO file can be read while the cooked indexer runs, so
if it would be a problem here, but I prefer to be safe than sorry.  This
patch side-steps the problem by deleting the DWARF version from
dwarf2_per_cu.

The only users of dwarf2_per_cu::version are the loclists callbacks in
`loc.c`.  Add the DWARF version to dwarf2_loclist_baton and modify those
callbacks to get the version from there instead.  Initialize that new
field in fill_in_loclist_baton.

I like this approach because there is no version field that is possibly
unset now.

I wasn't keen on doing this at first because I thought it would waste
space, but the dwarf2_loclist_baton has 7 bytes of padding at the end
anyway, so we might as well use that.

Cc: Ricky Zhou <ricky@rzhou.org>
Cc: Tom de Vries <tdevries@suse.de>
Cc: Tom Tromey <tom@tromey.com>
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32309
Change-Id: I30d4ede7d67da5d80ff65c6122f5868e1098ec52
---
 gdb/dwarf2/loc.c  | 12 ++++++------
 gdb/dwarf2/loc.h  |  3 +++
 gdb/dwarf2/read.c | 27 ++++++++-------------------
 gdb/dwarf2/read.h | 21 ---------------------
 4 files changed, 17 insertions(+), 46 deletions(-)


base-commit: 50ca09324ae2132f9bb73324482e4c0f5eaeddd1

Comments

Tom Tromey March 10, 2025, 8:01 p.m. UTC | #1 
>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> I don't know if a DWO file can be read while the cooked indexer runs, so
Simon> if it would be a problem here, but I prefer to be safe than
Simon> sorry.

I'm not super expert on fission stuff but I think these can be read
during indexing.

Simon> This
Simon> patch side-steps the problem by deleting the DWARF version from
Simon> dwarf2_per_cu.

I like this a lot.  This always seemed like a weird redundancy to me,
but I wasn't sure it really could be removed.  But since it evidently
can be:

Approved-By: Tom Tromey <tom@tromey.com>

Tom
Simon Marchi March 10, 2025, 8:08 p.m. UTC | #2 
On 3/10/25 4:01 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> I don't know if a DWO file can be read while the cooked indexer runs, so
> Simon> if it would be a problem here, but I prefer to be safe than
> Simon> sorry.
> 
> I'm not super expert on fission stuff but I think these can be read
> during indexing.
> 
> Simon> This
> Simon> patch side-steps the problem by deleting the DWARF version from
> Simon> dwarf2_per_cu.
> 
> I like this a lot.  This always seemed like a weird redundancy to me,
> but I wasn't sure it really could be removed.  But since it evidently
> can be:
> 
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom

Thanks, pushed.

Simon
diff mbox series

Patch

diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index 398c2b1b3522..7c12c0d4d4dd 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -388,12 +388,12 @@  dwarf2_find_location_expression (const dwarf2_loclist_baton *baton,
       enum debug_loc_kind kind;
       const gdb_byte *new_ptr = NULL; /* init for gcc -Wall */
 
-      if (baton->per_cu->version () < 5 && baton->from_dwo)
+      if (baton->dwarf_version < 5 && baton->from_dwo)
 	kind = decode_debug_loc_dwo_addresses (baton->per_cu,
 					       baton->per_objfile,
 					       loc_ptr, buf_end, &new_ptr,
 					       &low, &high, byte_order);
-      else if (baton->per_cu->version () < 5)
+      else if (baton->dwarf_version < 5)
 	kind = decode_debug_loc_addresses (loc_ptr, buf_end, &new_ptr,
 					   &low, &high,
 					   byte_order, addr_size,
@@ -444,7 +444,7 @@  dwarf2_find_location_expression (const dwarf2_loclist_baton *baton,
 	  high = (unrelocated_addr) ((CORE_ADDR) high + (CORE_ADDR) base_address);
 	}
 
-      if (baton->per_cu->version () < 5)
+      if (baton->dwarf_version < 5)
 	{
 	  length = extract_unsigned_integer (loc_ptr, 2, byte_order);
 	  loc_ptr += 2;
@@ -3976,12 +3976,12 @@  loclist_describe_location (struct symbol *symbol, CORE_ADDR addr,
       enum debug_loc_kind kind;
       const gdb_byte *new_ptr = NULL; /* init for gcc -Wall */
 
-      if (dlbaton->per_cu->version () < 5 && dlbaton->from_dwo)
+      if (dlbaton->dwarf_version < 5 && dlbaton->from_dwo)
 	kind = decode_debug_loc_dwo_addresses (dlbaton->per_cu,
 					       per_objfile,
 					       loc_ptr, buf_end, &new_ptr,
 					       &low, &high, byte_order);
-      else if (dlbaton->per_cu->version () < 5)
+      else if (dlbaton->dwarf_version < 5)
 	kind = decode_debug_loc_addresses (loc_ptr, buf_end, &new_ptr,
 					   &low, &high,
 					   byte_order, addr_size,
@@ -4031,7 +4031,7 @@  loclist_describe_location (struct symbol *symbol, CORE_ADDR addr,
       CORE_ADDR low_reloc = per_objfile->relocate (low);
       CORE_ADDR high_reloc = per_objfile->relocate (high);
 
-      if (dlbaton->per_cu->version () < 5)
+      if (dlbaton->dwarf_version < 5)
 	 {
 	   length = extract_unsigned_integer (loc_ptr, 2, byte_order);
 	   loc_ptr += 2;
diff --git a/gdb/dwarf2/loc.h b/gdb/dwarf2/loc.h
index 5c7330e43a09..de9e14479eae 100644
--- a/gdb/dwarf2/loc.h
+++ b/gdb/dwarf2/loc.h
@@ -197,6 +197,9 @@  struct dwarf2_loclist_baton
   /* Non-zero if the location list lives in .debug_loc.dwo.
      The format of entries in this section are different.  */
   unsigned char from_dwo;
+
+  /* The version of DWARF this loclist comes from.  */
+  unsigned char dwarf_version;
 };
 
 /* The baton used when a dynamic property is an offset to a parent
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 29af3dbf24ba..afd3ce08762b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -3279,8 +3279,6 @@  cutu_reader::cutu_reader (dwarf2_per_cu *this_cu,
 	  /* Establish the type offset that can be used to lookup the type.  */
 	  sig_type->type_offset_in_section =
 	    this_cu->sect_off + to_underlying (sig_type->type_offset_in_tu);
-
-	  this_cu->set_version (cu->header.version);
 	}
       else
 	{
@@ -3291,7 +3289,6 @@  cutu_reader::cutu_reader (dwarf2_per_cu *this_cu,
 
 	  gdb_assert (this_cu->sect_off == cu->header.sect_off);
 	  this_cu->set_length (cu->header.get_length_with_initial ());
-	  this_cu->set_version (cu->header.version);
 	}
     }
 
@@ -4302,10 +4299,6 @@  read_comp_units_from_section (dwarf2_per_objfile *per_objfile,
 	}
 
       this_cu->is_dwz = is_dwz;
-      /* Init this asap, to avoid a data race in the set_version in
-	 cutu_reader::cutu_reader (which may be run in parallel for the cooked
-	 index case).  */
-      this_cu->set_version (cu_header.version);
 
       info_ptr = info_ptr + this_cu->length ();
       per_objfile->per_bfd->all_units.push_back (std::move (this_cu));
@@ -8367,17 +8360,13 @@  open_and_init_dwo_file (dwarf2_cu *cu, const char *dwo_name,
   create_cus_hash_table (per_objfile, cu, *dwo_file, dwo_file->sections.info,
 			 dwo_file->cus);
 
-  if (cu->per_cu->version () < 5)
-    {
-      create_debug_types_hash_table (per_objfile, dwo_file.get (),
-				     dwo_file->sections.types, dwo_file->tus);
-    }
+  if (cu->header.version < 5)
+    create_debug_types_hash_table (per_objfile, dwo_file.get (),
+				   dwo_file->sections.types, dwo_file->tus);
   else
-    {
-      create_debug_type_hash_table (per_objfile, dwo_file.get (),
-				    &dwo_file->sections.info, dwo_file->tus,
-				    rcuh_kind::COMPILE);
-    }
+    create_debug_type_hash_table (per_objfile, dwo_file.get (),
+				  &dwo_file->sections.info, dwo_file->tus,
+				  rcuh_kind::COMPILE);
 
   dwarf_read_debug_printf ("DWO file found: %s", dwo_name);
 
@@ -13823,8 +13812,7 @@  read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
   if (type->code () == TYPE_CODE_VOID
       && !type->is_stub ()
       && die->child == nullptr
-      && (cu->per_cu->version () == 2
-	  || cu->producer_is_gas_2_39 ()))
+      && (cu->header.version == 2 || cu->producer_is_gas_2_39 ()))
     {
       /* Work around PR gas/29517, pretend we have an DW_TAG_unspecified_type
 	 return type.  */
@@ -20707,6 +20695,7 @@  fill_in_loclist_baton (struct dwarf2_cu *cu,
   else
     baton->base_address = {};
   baton->from_dwo = cu->dwo_unit != NULL;
+  baton->dwarf_version = cu->header.version;
 }
 
 static void
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 43d0e5d2ebf9..f7df863a42f9 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -127,9 +127,6 @@  struct dwarf2_per_cu
 private:
   unsigned int m_length = 0;
 
-  /* DWARF standard version this data has been read from (such as 4 or 5).  */
-  unsigned char m_dwarf_version = 0;
-
 public:
   /* Non-zero if this CU is from .debug_types.
      Struct dwarf2_per_cu is contained in struct signatured_type iff
@@ -288,24 +285,6 @@  struct dwarf2_per_cu
       gdb_assert (m_length == length);
   }
 
-  /* Return DWARF version number of this CU.  */
-  short version () const
-  {
-    /* Make sure it's set already.  */
-    gdb_assert (m_dwarf_version != 0);
-    return m_dwarf_version;
-  }
-
-  void set_version (short version)
-  {
-    if (m_dwarf_version == 0)
-      /* Set if not set already.  */
-      m_dwarf_version = version;
-    else
-      /* If already set, verify that it's the same value.  */
-      gdb_assert (m_dwarf_version == version);
-  }
-
   dwarf_unit_type unit_type (bool strict_p = true) const
   {
     dwarf_unit_type ut = m_unit_type.load ();