[PATCH 1/4] Increasing support for dwarf 5.

[PATCH 1/4] Increasing support for dwarf 5.

[Ali Tamur via gdb-patches]

* DW_UT_skeleton and DW_UT_split_compile compilation units also have signatures
to match the compilation unit in the skeleton and .dwo files. The signature is
part of the header.


gdb/ChangeLog:                                                                   
2019-08-26  Ali Tamur  <tamur@google.com>
                                                                                 
	* gdb/dwarf2read.c (comp_unit_head): Update comment.                     
	(dwarf2_dwo_name): New function declaration.                             
	(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 also have signature 
	in their header.                                                         
	(lookup_signature): New function. Returns the signature of the given     
	compile unit.                                                            
	(lookup_dwo_unit): API change. Use the new lookup_signature function.    
	(init_cutu_and_read_dies): Use dwarf2_dwo_name to find the dwo name.     
	(create_dwo_cu_reader): Use the added lookup_signature function.         
	(dwarf2_dwo_name): Get the dwo name if present.
---
 gdb/dwarf2read.c | 101 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 69 insertions(+), 32 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index de9755f6ce..91a4e48c8f 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -373,8 +373,9 @@ 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.  */
+  /* 64-bit signature of this type unit. In dwarf 4, it is valid only for
+     UNIT_TYPE DW_UT_type. In dwarf 5, DW_UT_split_type, DW_UT_partial,
+     DW_UT_skeleton, DW_UT_split_compile also contain a signature.  */
   ULONGEST signature;
 
   /* For types, offset in the type's DIE of the type defined by this TU.  */
@@ -1579,6 +1580,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);
 
@@ -6384,18 +6387,24 @@ 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 %d, should be DW_UT_type) [in module %s]"),
+		   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 %d, should be one of: %d, %d, %d, %d or %d) "
+		 "[in module %s]"),
+		 cu_header->unit_type, DW_UT_compile, DW_UT_skeleton,
+		 DW_UT_split_compile, DW_UT_type, DW_UT_split_type, filename);
 	}
 
       cu_header->addr_size = read_1_byte (abfd, info_ptr);
@@ -6416,13 +6425,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 = section_kind == rcuh_kind::TYPE
+    || cu_header->unit_type == DW_UT_skeleton
+    || cu_header->unit_type == DW_UT_split_compile;
 
+  if (header_has_signature)
+    {
       cu_header->signature = 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;
@@ -7291,46 +7306,59 @@ 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 ULONGEST
+lookup_signature (struct dwarf2_cu *cu, struct die_info* comp_unit_die)
+{
+  ULONGEST signature = 0;
+  struct attribute *attr;
+
+ attr = dwarf2_attr (comp_unit_die, DW_AT_GNU_dwo_id, cu);
+ if (attr)
+   signature = DW_UNSND (attr);
+ else if (cu->header.signature)
+   signature = cu->header.signature;
+ return signature;
+}
+
 /* 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)
+      ULONGEST signature = lookup_signature(cu, comp_unit_die);
+      if (!signature)
 	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,
+	       dwo_name, objfile_name (per_cu->dwarf2_per_objfile->objfile));
+      dwo_unit = lookup_dwo_comp_unit (per_cu, dwo_name, comp_dir,
 				       signature);
     }
 
@@ -7443,7 +7471,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
@@ -7580,9 +7607,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;
@@ -7594,7 +7621,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,
@@ -11833,10 +11860,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)
+  ULONGEST signature = lookup_signature(cu, comp_unit_die);
+  if (!signature)
     {
       complaint (_("Dwarf Error: debug entry at offset %s is missing"
 		   " its dwo_id [in module %s]"),
@@ -11845,7 +11871,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;
@@ -20102,6 +20128,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.  */
-- 
2.23.0.187.g17f5b7556c-goog

Re: [PATCH 1/4] Increasing support for dwarf 5.

[Simon Marchi]

Hi Ali,

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.

In commit titles / commit messages / comments, can you please use "DWARF" instead of
"dwarf", since that is the official spelling?

Could you also precise (here and in the commit message) how you have tested this?  Which
compiler, which version, which test case?

Here are a few comments on the code, just the surface since I am learning about
the subject as we go.

On 2019-08-27 7:41 p.m., Ali Tamur via gdb-patches wrote:
> * DW_UT_skeleton and DW_UT_split_compile compilation units also have signatures
> to match the compilation unit in the skeleton and .dwo files. The signature is
> part of the header.
> 
> 
> gdb/ChangeLog:                                                                   
> 2019-08-26  Ali Tamur  <tamur@google.com>
>                                                                                  
> 	* gdb/dwarf2read.c (comp_unit_head): Update comment.                     
> 	(dwarf2_dwo_name): New function declaration.                             
> 	(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 also have signature 
> 	in their header.                                                         
> 	(lookup_signature): New function. Returns the signature of the given     
> 	compile unit.                                                            
> 	(lookup_dwo_unit): API change. Use the new lookup_signature function.    
> 	(init_cutu_and_read_dies): Use dwarf2_dwo_name to find the dwo name.     
> 	(create_dwo_cu_reader): Use the added lookup_signature function.         
> 	(dwarf2_dwo_name): Get the dwo name if present.
> ---
>  gdb/dwarf2read.c | 101 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 69 insertions(+), 32 deletions(-)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index de9755f6ce..91a4e48c8f 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -373,8 +373,9 @@ 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.  */
> +  /* 64-bit signature of this type unit. In dwarf 4, it is valid only for
> +     UNIT_TYPE DW_UT_type. In dwarf 5, DW_UT_split_type, DW_UT_partial,
> +     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.

If I understand the spec correctly, these have a dwo_id:

- DW_UT_skeleton
- DW_UT_split_compile

And these have a type_signature:

- DW_UT_type
- DW_UT_split_type

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?

  /* Unit-type-dependent data.  */
  union {
    /* Used in DWARF 5 if UNIT_TYPE is DW_UT_skeleton or
       DW_UT_split_compile.  */
    struct {
      /* 64-bit compilation unit id that links the skeleton unit and its
         corresponding split unit. */
      ULONGEST dwo_id;
    } skeleton_split_compile;

    /* Used in DWARF 5 if UNIT_TYPE is DW_UT_type or DW_UT_split_type.  Also
       used in DWARF 4 for type units.  */
    struct  {
      /* 64-bit signature of the type described by this unit.  */
      ULONGEST signature;

      /* Offset in the type's DIE of the type defined by this TU.  */
      cu_offset type_cu_offset_in_tu;
    } type_split_type;
  };

>  
>    /* For types, offset in the type's DIE of the type defined by this TU.  */
> @@ -1579,6 +1580,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);
>  
> @@ -6384,18 +6387,24 @@ 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 %d, should be DW_UT_type) [in module %s]"),
> +		   cu_header->unit_type, filename);

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.

>  	  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 %d, should be one of: %d, %d, %d, %d or %d) "
> +		 "[in module %s]"),
> +		 cu_header->unit_type, DW_UT_compile, DW_UT_skeleton,
> +		 DW_UT_split_compile, DW_UT_type, DW_UT_split_type, filename);
>  	}

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), ...)

?

> @@ -7291,46 +7306,59 @@ 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 ULONGEST
> +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?

> +{
> +  ULONGEST signature = 0;
> +  struct attribute *attr;
> +
> + attr = dwarf2_attr (comp_unit_die, DW_AT_GNU_dwo_id, cu);
> + if (attr)
> +   signature = DW_UNSND (attr);
> + else if (cu->header.signature)
> +   signature = cu->header.signature;
> + return signature;
> +}

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

switch (cu->header.version)
  {
  case 4:
    // Do the thing for DWARF 4.
    break;
  case 5:
    // Do the thing for DWARF 5.
    break;
  default:
    internal_error (...);
  }

The idea being that we want to be sure not to use cu->header.signature if the CU
is DWARF 4, and we want to be sure not to use any DW_AT_GNU_dwo_id attribute if the
CU is DWARF 5 (AFAIK it shouldn't be there).

> +
>  /* 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)
> +      ULONGEST signature = lookup_signature(cu, comp_unit_die);
> +      if (!signature)
>  	error (_("Dwarf Error: missing dwo_id for dwo_name %s"
>  		 " [in module %s]"),

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.

> @@ -20102,6 +20128,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;
> +}

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.

Simon

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

[Ali Tamur via gdb-patches]

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

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

[Simon Marchi]

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 v2 1/4] DWARF 5 support: Handle dwo_id

[Ali Tamur via gdb-patches]

> 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).                                         
Done. Both with and without -gsplit-dwarf the set of tests that fail is          
identical. This is expected as unless I introduced a bug, there should be no     
behavioral change at all for DWARF 4. I also ran the testsuite with -gdwarf-5    
and the number of failures went down from 32687 to 1163, but until gdb can       
handle 'hello world' (hopefully at the end my patch series) I think that is      
not a very meaningful metric.                                                    
                                                                                 
> 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.                            
Updated the commit message to include the gcc version (8.3.0).                   
                                                                                 
> 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.                                                
Done.                                                                            
                                                                                 
> Space before parenthesis.  Applies to the rest of the patch, for both          
> function signatures and calls.                                                 
Done.                                                                            
                                                                                 
> Let's use %s and dwarf_unit_type_name to print DW_UT_type here, as you did below.
Done.                                                                            
                                                                                 
> This comment needs to be updated (the last part).                              
Done.                                                                            
                                                                                 
> Ref: https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Comparison_With_NULL_And_Zero
Done.                                                                            
                                                                                 
> 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.             
Done.                                                                            
                                                                                 
> Format the switch-case like:                                                   
Done.                                                                            
                                                                                 
Sorry for the style mistakes; I am accustomed to a different style and also      
have become too dependent on clang-tidy.
---


* 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 (version 8.3.0) against master branch (also with
-gsplit-dwarf and -gdwarf-4 flags) 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): Update comment.                     
        (dwarf2_dwo_name): New function declaration.                             
        (dwarf_unit_type_name): New function declaration.                        
        (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         
        (currently named as "signature") in their header. Also clarify error     
        messages.                                                                
        (lookup_dwo_id): New function. Returns the dwo id of the given           
        compile unit.                                                            
        (lookup_dwo_unit): Use the new lookup_dwo_id function.                   
        (init_cutu_and_read_dies): Use the new dwarf2_dwo_name and lookup_dwo_id 
        functions.                                                               
        (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 | 119 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 94 insertions(+), 25 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index fb888da7b8..fc30ddccd0 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -373,8 +373,11 @@ 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.  */
+
+  /* 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;
 
   /* For types, offset in the type's DIE of the type defined by this 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);
@@ -6390,18 +6397,28 @@ 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 %s) [in module %s]"),
+		   dwarf_unit_type_name (cu_header->unit_type),
+		   dwarf_unit_type_name (DW_UT_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 +6439,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 = section_kind == rcuh_kind::TYPE
+    || cu_header->unit_type == DW_UT_skeleton
+    || cu_header->unit_type == DW_UT_split_compile;
 
+  if (header_has_signature)
+    {
       cu_header->signature = 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;
@@ -7297,6 +7320,21 @@ read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
   return 1;
 }
 
+/* Return the signature of the compile unit, if found. 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.  */
+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;
+  struct attribute *attr;
+  attr = dwarf2_attr (comp_unit_die, DW_AT_GNU_dwo_id, cu);
+  if (attr == nullptr)
+    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.  */
@@ -7306,14 +7344,13 @@ lookup_dwo_unit (struct dwarf2_per_cu_data *this_cu,
 		 struct die_info *comp_unit_die)
 {
   struct dwarf2_cu *cu = this_cu->cu;
-  ULONGEST signature;
   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)
@@ -7323,21 +7360,17 @@ lookup_dwo_unit (struct dwarf2_per_cu_data *this_cu,
       /* Since this_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;
       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);
+				       *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
@@ -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 != nullptr)
     {
       struct dwo_unit *dwo_unit;
       struct die_info *dwo_comp_unit_die;
@@ -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 == nullptr)
+    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,33 @@ 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

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

[Simon Marchi]

On 2019-09-09 2:58 p.m., Ali Tamur via gdb-patches wrote:
>> 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).                                         
> Done. Both with and without -gsplit-dwarf the set of tests that fail is          
> identical. This is expected as unless I introduced a bug, there should be no     
> behavioral change at all for DWARF 4. I also ran the testsuite with -gdwarf-5    
> and the number of failures went down from 32687 to 1163, but until gdb can       
> handle 'hello world' (hopefully at the end my patch series) I think that is      
> not a very meaningful metric.                                                    

You're right that it's not very meaningful, but still encouraging :).

An interesting one will be to compare the results for DWARF 4 vs for DWARF 5.

>> 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.                            
> Updated the commit message to include the gcc version (8.3.0).                   

Thanks.

> Sorry for the style mistakes; I am accustomed to a different style and also      
> have become too dependent on clang-tidy.

No problem, and I completely understand.  I had a taste of clang-format (I
presume you meant clang-format) for C++ and black for Python, and it's really
nice not to have to think about formatting.  It would be wonderful to have
something equivalent here :).

I just found one last little thing:

> gdb/ChangeLog:                                                                   
>                                                                                  
>         * gdb/dwarf2read.c (comp_unit_head): Update comment.                     

The file name here should just be "dwarf2read.c", as it should be relative to
the ChangeLog file the entry is in.

The patch LGTM with that fixed, you can go ahead and push.  Thanks for following
up quickly and efficiently on review comments.

I'd like to take a look at the other patches of the series, but that won't be
before at least next week, as I'll be quite busy with the GNU Cauldron Conference
until then.  But if somebody else wants to review them, please go ahead.

Simon

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

[Ali Tamur via gdb-patches]

Thank you for the review, I am submitting now.

On Mon, Sep 9, 2019 at 2:21 PM Simon Marchi <simark@simark.ca> wrote:
>
> On 2019-09-09 2:58 p.m., Ali Tamur via gdb-patches wrote:
> >> 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).
> > Done. Both with and without -gsplit-dwarf the set of tests that fail is
> > identical. This is expected as unless I introduced a bug, there should be no
> > behavioral change at all for DWARF 4. I also ran the testsuite with -gdwarf-5
> > and the number of failures went down from 32687 to 1163, but until gdb can
> > handle 'hello world' (hopefully at the end my patch series) I think that is
> > not a very meaningful metric.
>
> You're right that it's not very meaningful, but still encouraging :).
>
> An interesting one will be to compare the results for DWARF 4 vs for DWARF 5.
>
> >> 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.
> > Updated the commit message to include the gcc version (8.3.0).
>
> Thanks.
>
> > Sorry for the style mistakes; I am accustomed to a different style and also
> > have become too dependent on clang-tidy.
>
> No problem, and I completely understand.  I had a taste of clang-format (I
> presume you meant clang-format) for C++ and black for Python, and it's really
> nice not to have to think about formatting.  It would be wonderful to have
> something equivalent here :).
>
> I just found one last little thing:
>
> > gdb/ChangeLog:
> >
> >         * gdb/dwarf2read.c (comp_unit_head): Update comment.
>
> The file name here should just be "dwarf2read.c", as it should be relative to
> the ChangeLog file the entry is in.
>
> The patch LGTM with that fixed, you can go ahead and push.  Thanks for following
> up quickly and efficiently on review comments.
>
> I'd like to take a look at the other patches of the series, but that won't be
> before at least next week, as I'll be quite busy with the GNU Cauldron Conference
> until then.  But if somebody else wants to review them, please go ahead.
>
> Simon

[PATCH v2 2/4] DWARF 5 support: Handle DW_FORM_strx

[Ali Tamur via gdb-patches]

Simon Marchi reviewed the first patch in the series but he is out this week,
and I'm hoping maybe someone else can take a look. This one should not be
controversial. Thanks.
---


* Handle DW_FORM_strx forms everywhere.
* A couple of annoying whitespace corrections.

Tested with CC=/usr/bin/gcc (version 8.3.0) against master branch (also with
-gsplit-dwarf and -gdwarf-4 flags) 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:

	* dwarf2read.c (skip_one_die): Handle DW_FORM_strx forms.
	(process_full_comp_unit): Correct whitespace.
	(process_full_type_unit): Likewise.
	(dwarf2_string_attr): Handle strx forms.
	(dump_die_shallow): Correct whitespace.
	(cu_debug_loc_section): Likewise.
---
 gdb/dwarf2read.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index a75941867a..bcf3d679bc 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -9298,6 +9298,7 @@ skip_one_die (const struct die_reader_specs *reader, const gdb_byte *info_ptr,
 	case DW_FORM_data1:
 	case DW_FORM_ref1:
 	case DW_FORM_flag:
+	case DW_FORM_strx1:
 	  info_ptr += 1;
 	  break;
 	case DW_FORM_flag_present:
@@ -9305,10 +9306,15 @@ skip_one_die (const struct die_reader_specs *reader, const gdb_byte *info_ptr,
 	  break;
 	case DW_FORM_data2:
 	case DW_FORM_ref2:
+	case DW_FORM_strx2:
 	  info_ptr += 2;
 	  break;
+	case DW_FORM_strx3:
+	  info_ptr += 3;
+	  break;
 	case DW_FORM_data4:
 	case DW_FORM_ref4:
+	case DW_FORM_strx4:
 	  info_ptr += 4;
 	  break;
 	case DW_FORM_data8:
@@ -10339,7 +10345,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
   if (cu->language == language_go)
     fixup_go_packaging (cu);
 
-  /* Now that we have processed all the DIEs in the CU, all the types 
+  /* Now that we have processed all the DIEs in the CU, all the types
      should be complete, and it should now be safe to compute all of the
      physnames.  */
   compute_delayed_physnames (cu);
@@ -10388,7 +10394,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu,
 
 	 Still one can confuse GDB by using non-standard GCC compilation
 	 options - this waits on GCC PR other/32998 (-frecord-gcc-switches).
-	 */ 
+	 */
       if (cu->has_loclist && gcc_4_minor >= 5)
 	cust->locations_valid = 1;
 
@@ -10443,7 +10449,7 @@ process_full_type_unit (struct dwarf2_per_cu_data *per_cu,
   if (cu->language == language_go)
     fixup_go_packaging (cu);
 
-  /* Now that we have processed all the DIEs in the CU, all the types 
+  /* Now that we have processed all the DIEs in the CU, all the types
      should be complete, and it should now be safe to compute all of the
      physnames.  */
   compute_delayed_physnames (cu);
@@ -20130,6 +20136,10 @@ dwarf2_string_attr (struct die_info *die, unsigned int name, struct dwarf2_cu *c
       if (attr->form == DW_FORM_strp || attr->form == DW_FORM_line_strp
 	  || attr->form == DW_FORM_string
 	  || attr->form == DW_FORM_strx
+	  || attr->form == DW_FORM_strx1
+	  || attr->form == DW_FORM_strx2
+	  || attr->form == DW_FORM_strx3
+	  || attr->form == DW_FORM_strx4
 	  || attr->form == DW_FORM_GNU_str_index
 	  || attr->form == DW_FORM_GNU_strp_alt)
 	str = DW_STRING (attr);
@@ -23024,7 +23034,7 @@ dump_die_shallow (struct ui_file *f, int indent, struct die_info *die)
 	case DW_FORM_indirect:
 	  /* The reader will have reduced the indirect form to
 	     the "base form" so this form should not occur.  */
-	  fprintf_unfiltered (f, 
+	  fprintf_unfiltered (f,
 			      "unexpected attribute form: DW_FORM_indirect");
 	  break;
 	case DW_FORM_implicit_const:
@@ -25125,7 +25135,7 @@ cu_debug_loc_section (struct dwarf2_cu *cu)
   if (cu->dwo_unit)
     {
       struct dwo_sections *sections = &cu->dwo_unit->dwo_file->sections;
-      
+
       return cu->header.version >= 5 ? &sections->loclists : &sections->loc;
     }
   return (cu->header.version >= 5 ? &dwarf2_per_objfile->loclists
-- 
2.23.0.162.g0b9fbb3734-goog

Re: [PATCH v2 2/4] DWARF 5 support: Handle DW_FORM_strx

[Simon Marchi]

On 2019-09-10 2:45 p.m., Ali Tamur via gdb-patches wrote:
> Simon Marchi reviewed the first patch in the series but he is out this week,
> and I'm hoping maybe someone else can take a look. This one should not be
> controversial. Thanks.
> ---
> 
> 
> * Handle DW_FORM_strx forms everywhere.
> * A couple of annoying whitespace corrections.
> 
> Tested with CC=/usr/bin/gcc (version 8.3.0) against master branch (also with
> -gsplit-dwarf and -gdwarf-4 flags) and there was no increase in the set of
> tests that fails.
> 
> This is part of an effort to support DWARF 5 in gdb.

Hi Ali,

For the whitespace fixes, could you push a separate obvious patch that takes care of that?

Otherwise the patch LGTM.

Simon