[v2,1/4] DWARF 5 support: Handle dwo_id

Message ID 20190906215249.161246-1-tamur@google.com
State New, archived
Headers

Commit Message

Terekhov, Mikhail via Gdb-patches Sept. 6, 2019, 9:52 p.m. UTC
  On Tue, Aug 27, 2019 at 10:15 PM Simon Marchi <simark@simark.ca> wrote:
> Thanks for splitting your patches.  In addition to having separate patches, please
> name each patch (git commit) to reflect what it does.  Otherwise we'll get 4 commits
> named "Increasing support for dwarf 5", not super clear.
Done. 

> In commit titles / commit messages / comments, can you please use "DWARF" instead of
> "dwarf", since that is the official spelling?
Done (but only in the lines I changed).

> Could you also precise (here and in the commit message) how you have tested this?  Which
> compiler, which version, which test case?
I compiled 1) synced master, and 2) that plus this patch. I used gcc as the  
compiler. I ran the gdb tests with "make check" and the same set of tests    
failed in each case. Until it comes to a point where gdb actually starts to  
handle 'hello world' compiled for DWARF 5, I don't know if I can do better.
 
> > +     DW_UT_skeleton, DW_UT_split_compile also contain a signature.  */
> >    ULONGEST signature;
>
> Does DW_UT_partial contain a signature?  In section 7.5.1.1 of the DWARF5 spec, it doesn't seem so.
My bad, corrected the comment. (Code needed no change).

> I think the code will be confusing if we keep the field named simply "signature", when it could
> actually mean "dwo_id".  Somebody reading the code with the DWARF 5 spec on the side will have some
> trouble making the correlation between both.  Maybe we should start using a union to describe that
> different fields are used with different kinds of units?
Please see the definition of struct dwo_unit, 'signature' is already used    
there. Renamed the field as "signature_or_dwo_id". If that is ok with you,    
I'd rather not introduce a union; the code is already complicated enough and  
I think it's not a good idea to mix such refactoring with behaviour changes  
in the same patch.
 
> There's a little discrepancy in this error message, it would show up as:
>
>   (is 1, should be DW_UT_type)
>
> Since we know it's an existing DW_UT value here, I'd suggest we print it as a
> string, or even both:
>
>   (is DW_UT_skeleton (0x4), should be DW_UT_type (0x2))
>
> We would just need a small utility function that converts numerical DW_UT value to string.
Done.

> Here, it's fine to not print the stringified version of the value, since it's an
> invalid value.  But could make the message appear like:
>
>   (is 0xff, should be one of: DW_UT_compile (0x1), DW_UT_skeleton (0x4), ...)
Done.

 > +lookup_signature (struct dwarf2_cu *cu, struct die_info* comp_unit_die)
>
> Again here, since the DWARF 5 standard talks about dwo_id, not signature, wouldn't it make
> more sense for us to use the term dwo_id?
 I renamed the function as lookup_dwo_id.

> Wouldn't it make sense here to check the DWARF version from the cu_header and
> take an action based on that?
Done.

> Even though we're unlikely to have a signature / dwo_id that is 0, I would
> prefer if we didn't use it as a special value to denote failure.  The function
> should return a boolean, and the signature / dwo_id would be returned as an
> output parameter.
Done.

> Here too, should we do something based on the CU's DWARF version?  I presume we don't want
> to accept a DW_AT_GNU_dwo_name attribute in a DWARF 5 CU.
 clang (and maybe others) still uses DW_AT_GNU_dwo_name in DWARF 5.

Thanks for the detailed review. I am attaching the updated commit message, Changelog and changes below.
---

* DW_UT_skeleton and DW_UT_split_compile compilation units have dwo ids
to match the compilation unit in the skeleton and .dwo files. The dwo_id is
in the header.

Tested with CC=/usr/bin/gcc against master and there was no increase in the
set of tests that fails.

This is part of an effort to support DWARF 5 in gdb.


gdb/ChangeLog:                                                                   
                                                                                 
        * gdb/dwarf2read.c (comp_unit_head): Rename field and update comment.    
        (dwarf2_dwo_name): New function declaration.                             
        (dwarf_unit_type_name): New function declaration.                        
        (create_signatured_type_table_from_debug_names): Reflect name change.    
        (read_comp_unit_head): Add support for new compilation units,            
        DW_UT_partial, DW_UT_skeleton, DW_UT_split_compile, DW_UT_split_type.    
        Particularly, DW_UT_skeleton and DW_UT_split_compile have dwo_id         
        in their header. Also clarify error messages.                            
        (create_debug_type_hash_table): Reflect field name change.               
        (read_cutu_die_from_dwo): Likewise.                                      
        (lookup_dwo_id): New function. Returns the dwo id of the given           
        compile unit.                                                            
        (lookup_dwo_unit): API change. Use the new lookup_dwo_id function.       
        (init_cutu_and_read_dies): Use the new dwarf2_dwo_name and lookup_dwo_id 
        functions.                                                               
        (read_comp_units_from_section): Reflect field name change.               
        (create_dwo_cu_reader): Use the added lookup_dwo_id function.            
        (dwarf2_dwo_name): Get the dwo name if present.                          
        (dwarf_unit_type_name): Convert DW_UT_* types to string for diagnostic   
        purposes.
---
 gdb/dwarf2read.c | 149 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 104 insertions(+), 45 deletions(-)
  

Comments

Simon Marchi Sept. 7, 2019, 8:30 p.m. UTC | #1
Hi Ali,

Thanks for the new version.

On 2019-09-06 5:52 p.m., Ali Tamur via gdb-patches wrote:
>> Could you also precise (here and in the commit message) how you have tested this?  Which
>> compiler, which version, which test case?
> I compiled 1) synced master, and 2) that plus this patch. I used gcc as the  
> compiler. I ran the gdb tests with "make check" and the same set of tests    
> failed in each case. Until it comes to a point where gdb actually starts to  
> handle 'hello world' compiled for DWARF 5, I don't know if I can do better.

Running the testsuite before and after the patch and comparing the results (as you did)
is indeed the recommended way, as the testsuite is not passing at 100% (especially given
the number of different possible configurations.

When you say you ran "make check", I understand it was without any special arguments, so
with DWARF 4 (gcc's default today) and non-split-DWARF?

Since you're touching the split-DWARF code, it might be a good idea to run the whole
testsuite with -gsplit-dwarf, with DWARF 4 (to verify there are no regressions) and
DWARF 5 (to check how things improve).

To illustrate, I ran this without your patch applied:

  $ make check TESTS="gdb.base/break.exp" RUNTESTFLAGS="CFLAGS_FOR_TARGET='-gdwarf-4 -gsplit-dwarf'"
  # of expected passes		105

  $ make check TESTS="gdb.base/break.exp" RUNTESTFLAGS="CFLAGS_FOR_TARGET='-gdwarf-5 -gsplit-dwarf'"
  # of expected passes		15
  # of unexpected failures	44

And this with your patch applied:

  $ make check TESTS="gdb.base/break.exp" RUNTESTFLAGS="CFLAGS_FOR_TARGET='-gdwarf-4 -gsplit-dwarf'"
  # of expected passes		105

  (good, no regressions with DWARF 4)

  $ make check TESTS="gdb.base/break.exp" RUNTESTFLAGS="CFLAGS_FOR_TARGET='-gdwarf-5 -gsplit-dwarf'"
  # of expected passes		60
  # of unexpected failures	45

  (not perfect, but the test went further before giving up, good news!)

This is just one test though.  Running the whole gdb.base directory (with TESTS="gdb.base/*.exp")
for each patch you add would give good coverage without being too time-consuming.  And maybe one
full run (just omit the TESTS variable) at the end to compare before/after your patch series.

And since DWARF 5 is relatively new stuff, results can vary greatly if using different versions of the
same compiler to run the tests.  So if you could mention in the commit message which gcc version the
tests were ran against, I think it would be useful.

>>> +     DW_UT_skeleton, DW_UT_split_compile also contain a signature.  */
>>>    ULONGEST signature;
>>
>> Does DW_UT_partial contain a signature?  In section 7.5.1.1 of the DWARF5 spec, it doesn't seem so.
> My bad, corrected the comment. (Code needed no change).
> 
>> I think the code will be confusing if we keep the field named simply "signature", when it could
>> actually mean "dwo_id".  Somebody reading the code with the DWARF 5 spec on the side will have some
>> trouble making the correlation between both.  Maybe we should start using a union to describe that
>> different fields are used with different kinds of units?
> Please see the definition of struct dwo_unit, 'signature' is already used    
> there. Renamed the field as "signature_or_dwo_id". If that is ok with you,    
> I'd rather not introduce a union; the code is already complicated enough and  
> I think it's not a good idea to mix such refactoring with behaviour changes  
> in the same patch.

I would say that the same change should be done in dwo_unit then.  But I totally
agree that we should not mix up cleanups with adding new features.  So here's
what I suggest: revert the field in comp_unit_head to be just `signature` for
now, it will avoid having too many unrelated changes in this patch (updated all
the users of that field).  We'll make a following cleanup patch to either rename
the field or introduce a union.

Sorry you went through the trouble of doing this rename only for me to suggest
you revert it, but as you say I think it's better to to mix refactoring with
behavior changes.

>> Here too, should we do something based on the CU's DWARF version?  I presume we don't want
>> to accept a DW_AT_GNU_dwo_name attribute in a DWARF 5 CU.
>  clang (and maybe others) still uses DW_AT_GNU_dwo_name in DWARF 5.

Ok.  Given that DWARF 5 introduces the feature to supersede DW_AT_GNU_dwo_name,
I would have thought compilers would not emit it when using DWARF 5.  But indeed,
clang does emit it (using today's master):

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000024 version = 0x0005 unit_type = e abbr_offset = 0x0000 addr_size = 0x08 DWO_id = 0xe23aa1245e312b00 (next unit at 0x00000028)

0x00000014: DW_TAG_compile_unit
              DW_AT_stmt_list	(0x00000000)
              DW_AT_str_offsets_base	(0x00000008)
              DW_AT_comp_dir	("/home/simark/build/binutils-gdb/gdb")
              DW_AT_GNU_pubnames	(true)
              DW_AT_GNU_dwo_name	("test.dwo")
              DW_AT_addr_base	(0x00000008)
              DW_AT_low_pc	(0x0000000000000000)
              DW_AT_high_pc	(0x0000000000000035)

Not that clang still uses DW_TAG_compile_unit (which is the DWARF 4 way of doing it,
I presume), unlike gcc 9.1.0 who uses DW_TAG_skeleton_unit + DW_AT_dwo_name:

0x00000014: DW_TAG_skeleton_unit
              DW_AT_low_pc	(0x0000000000000000)
              DW_AT_high_pc	(0x000000000000002a)
              DW_AT_stmt_list	(0x00000000)
              DW_AT_dwo_name	("test.dwo")
              DW_AT_comp_dir	("/home/simark/build/binutils-gdb/gdb")
              DW_AT_GNU_pubnames	(true)
              DW_AT_addr_base	(0x00000008)

When clang switches to use DW_TAG_skeleton_unit, I think they'll have to switch to using
DW_AT_dwo_name.  My understanding of DWARF 5 (section 3.1.2) is that this attribute is
mandatory in a DW_TAG_skeleton_unit.

It doesn't really impact us though, we have to support both.

> Thanks for the detailed review. I am attaching the updated commit message, Changelog and changes below.

Thanks, that looks nice, I added a few comments below, nothing major.

> +static const char *dwarf_unit_type_name(int unit_type);

Space before parenthesis.  Applies to the rest of the patch, for both
function signatures and calls.

> @@ -6390,18 +6397,27 @@ read_comp_unit_head (struct comp_unit_head *cu_header,
>        switch (cu_header->unit_type)
>  	{
>  	case DW_UT_compile:
> +	case DW_UT_partial:
> +	case DW_UT_skeleton:
> +	case DW_UT_split_compile:
>  	  if (section_kind != rcuh_kind::COMPILE)
>  	    error (_("Dwarf Error: wrong unit_type in compilation unit header "
> -		   "(is DW_UT_compile, should be DW_UT_type) [in module %s]"),
> -		   filename);
> +		   "(is %s, should be DW_UT_type) [in module %s]"),
> +		   dwarf_unit_type_name(cu_header->unit_type), filename);

Let's use %s and dwarf_unit_type_name to print DW_UT_type here, as you did below.

>  	  break;
>  	case DW_UT_type:
> +	case DW_UT_split_type:
>  	  section_kind = rcuh_kind::TYPE;
>  	  break;
>  	default:
>  	  error (_("Dwarf Error: wrong unit_type in compilation unit header "
> -		 "(is %d, should be %d or %d) [in module %s]"),
> -		 cu_header->unit_type, DW_UT_compile, DW_UT_type, filename);
> +		 "(is %#04x, should be one of: %s, %s, %s, %s or %s) "
> +		 "[in module %s]"), cu_header->unit_type,
> +		 dwarf_unit_type_name(DW_UT_compile),
> +		 dwarf_unit_type_name(DW_UT_skeleton),
> +		 dwarf_unit_type_name(DW_UT_split_compile),
> +		 dwarf_unit_type_name(DW_UT_type),
> +		 dwarf_unit_type_name(DW_UT_split_type), filename);

[snip]

> @@ -7297,47 +7319,58 @@ read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
>    return 1;
>  }
>  
> +/* Return the signature of the compile unit. In DWARF 4 and before, the
> +   signature is in the DW_AT_GNU_dwo_id attribute. In DWARF 5 and later, the
> +   signature is part of the header. Returns 0 if the signature is not found.  */

This comment needs to be updated (the last part).

> +static gdb::optional<ULONGEST>
> +lookup_dwo_id (struct dwarf2_cu *cu, struct die_info* comp_unit_die)
> +{
> +  if (cu->header.version >= 5)
> +    return cu->header.signature_or_dwo_id;
> +  struct attribute *attr;
> +  attr = dwarf2_attr (comp_unit_die, DW_AT_GNU_dwo_id, cu);
> +  if (!attr)

Use either of:

  if (attr == NULL)
  if (attr == nullptr)

This happens a few times in the patch.  In the opposite case, if (attr) should be
replaced with either of:

  if (attr != NULL)
  if (attr != nullptr)

Ref: https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Comparison_With_NULL_And_Zero

> +    return gdb::optional<ULONGEST>();
> +  return DW_UNSND (attr);
> +}
> +
>  /* Subroutine of init_cutu_and_read_dies to simplify it.
>     Look up the DWO unit specified by COMP_UNIT_DIE of THIS_CU.
>     Returns NULL if the specified DWO unit cannot be found.  */
>  
>  static struct dwo_unit *
> -lookup_dwo_unit (struct dwarf2_per_cu_data *this_cu,
> +lookup_dwo_unit (const struct die_reader_specs *reader,
>  		 struct die_info *comp_unit_die)

Is there a reason for this function to now take `reader` rather than `this_cu`?  At first
sight, I don't see why this change is needed or why it would help.

> @@ -22812,6 +22854,23 @@ dwarf_attr_name (unsigned attr)
>    return name;
>  }
>  
> +/* Convert a unit type to corresponding DW_UT name.  */
> +
> +static const char *
> +dwarf_unit_type_name(int unit_type) {
> +  switch (unit_type) {
> +    case 0x01: return "DW_UT_compile (0x01)";
> +    case 0x02: return "DW_UT_type (0x02)";
> +    case 0x03: return "DW_UT_partial (0x03)";
> +    case 0x04: return "DW_UT_skeleton (0x04)";
> +    case 0x05: return "DW_UT_split_compile (0x05)";
> +    case 0x06: return "DW_UT_split_type (0x06)";
> +    case 0x80: return "DW_UT_lo_user (0x80)";
> +    case 0xff: return "DW_UT_hi_user (0xff)";
> +    default: return nullptr;
> +  }
> +}

Format the switch-case like:

  switch (...)
    {
    case 1:
      break;
    break:
      break;
    }

Thanks,

Simon
  

Patch

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index fb888da7b8..e0697ff8c4 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -373,9 +373,12 @@  struct comp_unit_head
      This will be the first byte following the compilation unit header.  */
   cu_offset first_die_cu_offset;
 
-  /* 64-bit signature of this type unit - it is valid only for
-     UNIT_TYPE DW_UT_type.  */
-  ULONGEST signature;
+
+  /* 64-bit signature of this unit. For type units, it denotes the signature of
+     the type (DW_UT_type in DWARF 4, additionally DW_UT_split_type in DWARF 5).
+     Also used in DWARF 5, to denote the dwo id when the unit type is
+     DW_UT_skeleton or DW_UT_split_compile.  */
+  ULONGEST signature_or_dwo_id;
 
   /* For types, offset in the type's DIE of the type defined by this TU.  */
   cu_offset type_cu_offset_in_tu;
@@ -1579,6 +1582,8 @@  static struct attribute *dwarf2_attr_no_follow (struct die_info *,
 static const char *dwarf2_string_attr (struct die_info *die, unsigned int name,
                                        struct dwarf2_cu *cu);
 
+static const char *dwarf2_dwo_name (struct die_info *die, struct dwarf2_cu *cu);
+
 static int dwarf2_flag_true_p (struct die_info *die, unsigned name,
                                struct dwarf2_cu *cu);
 
@@ -1761,6 +1766,8 @@  static const char *dwarf_tag_name (unsigned int);
 
 static const char *dwarf_attr_name (unsigned int);
 
+static const char *dwarf_unit_type_name(int unit_type);
+
 static const char *dwarf_form_name (unsigned int);
 
 static const char *dwarf_bool_name (unsigned int);
@@ -3105,7 +3112,7 @@  create_signatured_type_table_from_debug_names
 
       sig_type = OBSTACK_ZALLOC (&objfile->objfile_obstack,
 				 struct signatured_type);
-      sig_type->signature = cu_header.signature;
+      sig_type->signature = cu_header.signature_or_dwo_id;
       sig_type->type_offset_in_tu = cu_header.type_cu_offset_in_tu;
       sig_type->per_cu.is_debug_types = 1;
       sig_type->per_cu.section = section;
@@ -6390,18 +6397,27 @@  read_comp_unit_head (struct comp_unit_head *cu_header,
       switch (cu_header->unit_type)
 	{
 	case DW_UT_compile:
+	case DW_UT_partial:
+	case DW_UT_skeleton:
+	case DW_UT_split_compile:
 	  if (section_kind != rcuh_kind::COMPILE)
 	    error (_("Dwarf Error: wrong unit_type in compilation unit header "
-		   "(is DW_UT_compile, should be DW_UT_type) [in module %s]"),
-		   filename);
+		   "(is %s, should be DW_UT_type) [in module %s]"),
+		   dwarf_unit_type_name(cu_header->unit_type), filename);
 	  break;
 	case DW_UT_type:
+	case DW_UT_split_type:
 	  section_kind = rcuh_kind::TYPE;
 	  break;
 	default:
 	  error (_("Dwarf Error: wrong unit_type in compilation unit header "
-		 "(is %d, should be %d or %d) [in module %s]"),
-		 cu_header->unit_type, DW_UT_compile, DW_UT_type, filename);
+		 "(is %#04x, should be one of: %s, %s, %s, %s or %s) "
+		 "[in module %s]"), cu_header->unit_type,
+		 dwarf_unit_type_name(DW_UT_compile),
+		 dwarf_unit_type_name(DW_UT_skeleton),
+		 dwarf_unit_type_name(DW_UT_split_compile),
+		 dwarf_unit_type_name(DW_UT_type),
+		 dwarf_unit_type_name(DW_UT_split_type), filename);
 	}
 
       cu_header->addr_size = read_1_byte (abfd, info_ptr);
@@ -6422,13 +6438,19 @@  read_comp_unit_head (struct comp_unit_head *cu_header,
 		    _("read_comp_unit_head: dwarf from non elf file"));
   cu_header->signed_addr_p = signed_addr;
 
-  if (section_kind == rcuh_kind::TYPE)
-    {
-      LONGEST type_offset;
+  bool header_has_signature_or_dwo_id = section_kind == rcuh_kind::TYPE
+    || cu_header->unit_type == DW_UT_skeleton
+    || cu_header->unit_type == DW_UT_split_compile;
 
-      cu_header->signature = read_8_bytes (abfd, info_ptr);
+  if (header_has_signature_or_dwo_id)
+    {
+      cu_header->signature_or_dwo_id = read_8_bytes (abfd, info_ptr);
       info_ptr += 8;
+    }
 
+  if (section_kind == rcuh_kind::TYPE)
+    {
+      LONGEST type_offset;
       type_offset = read_offset (abfd, info_ptr, cu_header, &bytes_read);
       info_ptr += bytes_read;
       cu_header->type_cu_offset_in_tu = (cu_offset) type_offset;
@@ -6694,7 +6716,7 @@  create_debug_type_hash_table (struct dwarf2_per_objfile *dwarf2_per_objfile,
       sect_offset sect_off = (sect_offset) (ptr - section->buffer);
 
       /* Initialize it due to a false compiler warning.  */
-      header.signature = -1;
+      header.signature_or_dwo_id = -1;
       header.type_cu_offset_in_tu = (cu_offset) -1;
 
       /* We need to read the type's signature in order to build the hash
@@ -6728,7 +6750,7 @@  create_debug_type_hash_table (struct dwarf2_per_objfile *dwarf2_per_objfile,
 	  dwo_tu = OBSTACK_ZALLOC (&objfile->objfile_obstack,
 				   struct dwo_unit);
 	  dwo_tu->dwo_file = dwo_file;
-	  dwo_tu->signature = header.signature;
+	  dwo_tu->signature = header.signature_or_dwo_id;
 	  dwo_tu->type_offset_in_tu = header.type_cu_offset_in_tu;
 	  dwo_tu->section = section;
 	  dwo_tu->sect_off = sect_off;
@@ -6741,7 +6763,7 @@  create_debug_type_hash_table (struct dwarf2_per_objfile *dwarf2_per_objfile,
 	  dwo_tu = NULL;
 	  sig_type = OBSTACK_ZALLOC (&objfile->objfile_obstack,
 				     struct signatured_type);
-	  sig_type->signature = header.signature;
+	  sig_type->signature = header.signature_or_dwo_id;
 	  sig_type->type_offset_in_tu = header.type_cu_offset_in_tu;
 	  sig_type->per_cu.dwarf2_per_objfile = dwarf2_per_objfile;
 	  sig_type->per_cu.is_debug_types = 1;
@@ -6776,14 +6798,14 @@  create_debug_type_hash_table (struct dwarf2_per_objfile *dwarf2_per_objfile,
 	  complaint (_("debug type entry at offset %s is duplicate to"
 		       " the entry at offset %s, signature %s"),
 		     sect_offset_str (sect_off), sect_offset_str (dup_sect_off),
-		     hex_string (header.signature));
+		     hex_string (header.signature_or_dwo_id));
 	}
       *slot = dwo_file ? (void *) dwo_tu : (void *) sig_type;
 
       if (dwarf_read_debug > 1)
 	fprintf_unfiltered (gdb_stdlog, "  offset %s, signature %s\n",
 			    sect_offset_str (sect_off),
-			    hex_string (header.signature));
+			    hex_string (header.signature_or_dwo_id));
 
       info_ptr += length;
     }
@@ -7206,12 +7228,12 @@  read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
 						dwo_abbrev_section,
 						info_ptr, rcuh_kind::TYPE);
       /* This is not an assert because it can be caused by bad debug info.  */
-      if (sig_type->signature != cu->header.signature)
+      if (sig_type->signature != cu->header.signature_or_dwo_id)
 	{
 	  error (_("Dwarf Error: signature mismatch %s vs %s while reading"
 		   " TU at offset %s [in module %s]"),
 		 hex_string (sig_type->signature),
-		 hex_string (cu->header.signature),
+		 hex_string (cu->header.signature_or_dwo_id),
 		 sect_offset_str (dwo_unit->sect_off),
 		 bfd_get_filename (abfd));
 	}
@@ -7297,47 +7319,58 @@  read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
   return 1;
 }
 
+/* Return the signature of the compile unit. In DWARF 4 and before, the
+   signature is in the DW_AT_GNU_dwo_id attribute. In DWARF 5 and later, the
+   signature is part of the header. Returns 0 if the signature is not found.  */
+static gdb::optional<ULONGEST>
+lookup_dwo_id (struct dwarf2_cu *cu, struct die_info* comp_unit_die)
+{
+  if (cu->header.version >= 5)
+    return cu->header.signature_or_dwo_id;
+  struct attribute *attr;
+  attr = dwarf2_attr (comp_unit_die, DW_AT_GNU_dwo_id, cu);
+  if (!attr)
+    return gdb::optional<ULONGEST>();
+  return DW_UNSND (attr);
+}
+
 /* Subroutine of init_cutu_and_read_dies to simplify it.
    Look up the DWO unit specified by COMP_UNIT_DIE of THIS_CU.
    Returns NULL if the specified DWO unit cannot be found.  */
 
 static struct dwo_unit *
-lookup_dwo_unit (struct dwarf2_per_cu_data *this_cu,
+lookup_dwo_unit (const struct die_reader_specs *reader,
 		 struct die_info *comp_unit_die)
 {
-  struct dwarf2_cu *cu = this_cu->cu;
-  ULONGEST signature;
+  struct dwarf2_cu *cu = reader->cu;
+  struct dwarf2_per_cu_data *per_cu = cu->per_cu;
   struct dwo_unit *dwo_unit;
   const char *comp_dir, *dwo_name;
 
   gdb_assert (cu != NULL);
 
   /* Yeah, we look dwo_name up again, but it simplifies the code.  */
-  dwo_name = dwarf2_string_attr (comp_unit_die, DW_AT_GNU_dwo_name, cu);
+  dwo_name = dwarf2_dwo_name (comp_unit_die, cu);
   comp_dir = dwarf2_string_attr (comp_unit_die, DW_AT_comp_dir, cu);
 
-  if (this_cu->is_debug_types)
+  if (per_cu->is_debug_types)
     {
       struct signatured_type *sig_type;
 
-      /* Since this_cu is the first member of struct signatured_type,
+      /* Since per_cu is the first member of struct signatured_type,
 	 we can go from a pointer to one to a pointer to the other.  */
-      sig_type = (struct signatured_type *) this_cu;
-      signature = sig_type->signature;
+      sig_type = (struct signatured_type *) per_cu;
       dwo_unit = lookup_dwo_type_unit (sig_type, dwo_name, comp_dir);
     }
   else
     {
-      struct attribute *attr;
-
-      attr = dwarf2_attr (comp_unit_die, DW_AT_GNU_dwo_id, cu);
-      if (! attr)
+      gdb::optional<ULONGEST> signature = lookup_dwo_id(cu, comp_unit_die);
+      if (!signature.has_value())
 	error (_("Dwarf Error: missing dwo_id for dwo_name %s"
 		 " [in module %s]"),
-	       dwo_name, objfile_name (this_cu->dwarf2_per_objfile->objfile));
-      signature = DW_UNSND (attr);
-      dwo_unit = lookup_dwo_comp_unit (this_cu, dwo_name, comp_dir,
-				       signature);
+	       dwo_name, objfile_name (per_cu->dwarf2_per_objfile->objfile));
+      dwo_unit = lookup_dwo_comp_unit (per_cu, dwo_name, comp_dir,
+				       *signature);
     }
 
   return dwo_unit;
@@ -7449,7 +7482,6 @@  init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
   struct die_reader_specs reader;
   struct die_info *comp_unit_die;
   int has_children;
-  struct attribute *attr;
   struct signatured_type *sig_type = NULL;
   struct dwarf2_section_info *abbrev_section;
   /* Non-zero if CU currently points to a DWO file and we need to
@@ -7523,7 +7555,7 @@  init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
 	  /* Since per_cu is the first member of struct signatured_type,
 	     we can go from a pointer to one to a pointer to the other.  */
 	  sig_type = (struct signatured_type *) this_cu;
-	  gdb_assert (sig_type->signature == cu->header.signature);
+	  gdb_assert (sig_type->signature == cu->header.signature_or_dwo_id);
 	  gdb_assert (sig_type->type_offset_in_tu
 		      == cu->header.type_cu_offset_in_tu);
 	  gdb_assert (this_cu->sect_off == cu->header.sect_off);
@@ -7586,9 +7618,9 @@  init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
 
      Note that if USE_EXISTING_OK != 0, and THIS_CU->cu already contains a
      DWO CU, that this test will fail (the attribute will not be present).  */
-  attr = dwarf2_attr (comp_unit_die, DW_AT_GNU_dwo_name, cu);
+  const char *dwo_name = dwarf2_dwo_name (comp_unit_die, cu);
   abbrev_table_up dwo_abbrev_table;
-  if (attr)
+  if (dwo_name)
     {
       struct dwo_unit *dwo_unit;
       struct die_info *dwo_comp_unit_die;
@@ -7600,7 +7632,7 @@  init_cutu_and_read_dies (struct dwarf2_per_cu_data *this_cu,
 		     sect_offset_str (this_cu->sect_off),
 		     bfd_get_filename (abfd));
 	}
-      dwo_unit = lookup_dwo_unit (this_cu, comp_unit_die);
+      dwo_unit = lookup_dwo_unit (&reader, comp_unit_die);
       if (dwo_unit != NULL)
 	{
 	  if (read_cutu_die_from_dwo (this_cu, dwo_unit,
@@ -8535,7 +8567,7 @@  read_comp_units_from_section (struct dwarf2_per_objfile *dwarf2_per_objfile,
 	  auto sig_type = XOBNEW (&objfile->objfile_obstack,
 				  struct signatured_type);
 	  memset (sig_type, 0, sizeof (*sig_type));
-	  sig_type->signature = cu_header.signature;
+	  sig_type->signature = cu_header.signature_or_dwo_id;
 	  sig_type->type_offset_in_tu = cu_header.type_cu_offset_in_tu;
 	  this_cu = &sig_type->per_cu;
 	}
@@ -11839,10 +11871,9 @@  create_dwo_cu_reader (const struct die_reader_specs *reader,
   struct create_dwo_cu_data *data = (struct create_dwo_cu_data *) datap;
   struct dwo_file *dwo_file = data->dwo_file;
   struct dwo_unit *dwo_unit = &data->dwo_unit;
-  struct attribute *attr;
 
-  attr = dwarf2_attr (comp_unit_die, DW_AT_GNU_dwo_id, cu);
-  if (attr == NULL)
+  gdb::optional<ULONGEST> signature = lookup_dwo_id(cu, comp_unit_die);
+  if (!signature.has_value())
     {
       complaint (_("Dwarf Error: debug entry at offset %s is missing"
 		   " its dwo_id [in module %s]"),
@@ -11851,7 +11882,7 @@  create_dwo_cu_reader (const struct die_reader_specs *reader,
     }
 
   dwo_unit->dwo_file = dwo_file;
-  dwo_unit->signature = DW_UNSND (attr);
+  dwo_unit->signature = *signature;
   dwo_unit->section = section;
   dwo_unit->sect_off = sect_off;
   dwo_unit->length = cu->per_cu->length;
@@ -20113,6 +20144,17 @@  dwarf2_string_attr (struct die_info *die, unsigned int name, struct dwarf2_cu *c
   return str;
 }
 
+/* Return the dwo name or NULL if not present. If present, it is in either
+   DW_AT_GNU_dwo_name or DW_AT_dwo_name atrribute.  */
+static const char *
+dwarf2_dwo_name (struct die_info *die, struct dwarf2_cu *cu)
+{
+  const char *dwo_name = dwarf2_string_attr (die, DW_AT_GNU_dwo_name, cu);
+  if (!dwo_name)
+    dwo_name = dwarf2_string_attr (die, DW_AT_dwo_name, cu);
+  return dwo_name;
+}
+
 /* Return non-zero iff the attribute NAME is defined for the given DIE,
    and holds a non-zero value.  This function should only be used for
    DW_FORM_flag or DW_FORM_flag_present attributes.  */
@@ -22812,6 +22854,23 @@  dwarf_attr_name (unsigned attr)
   return name;
 }
 
+/* Convert a unit type to corresponding DW_UT name.  */
+
+static const char *
+dwarf_unit_type_name(int unit_type) {
+  switch (unit_type) {
+    case 0x01: return "DW_UT_compile (0x01)";
+    case 0x02: return "DW_UT_type (0x02)";
+    case 0x03: return "DW_UT_partial (0x03)";
+    case 0x04: return "DW_UT_skeleton (0x04)";
+    case 0x05: return "DW_UT_split_compile (0x05)";
+    case 0x06: return "DW_UT_split_type (0x06)";
+    case 0x80: return "DW_UT_lo_user (0x80)";
+    case 0xff: return "DW_UT_hi_user (0xff)";
+    default: return nullptr;
+  }
+}
+
 /* Convert a DWARF value form code into its string name.  */
 
 static const char *