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

["Ali Tamur via gdb-patches" <gdb-patches@sourceware.org>]

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(-)

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 *
-- 
2.23.0.187.g17f5b7556c-goog