[PATCH 00/20] Make DWARF attribute references safe

[PATCH 00/20] Make DWARF attribute references safe

[Tom Tromey]

This series changes the DWARF code to always check that the use of an
attribute's value is safe -- that is, that the requested type
corresponds to one of the forms that can construct a value of that
type.

This caught some minor bugs in the DWARF reader, though, IMO, nothing
very serious.

Attribute typing is still somewhat ad hoc.  That is, while the DWARF
standard specifies the type classes of forms, gdb largely doesn't
conform to this.  Instead, it is more lenient.  This could be changed,
but I didn't want to mix things too much in this series.  Also there's
an argument to be made that there's nothing wrong with the current
approach.

Regression tested by the buildbot.  Let me know what you think.

Tom

[PATCH 01/20] Add attribute::value_as_string method

[Tom Tromey]

The full DIE reader checks that an attribute has a "string" form in
some spots, but the partial DIE reader does not.  This patch brings
the two readers in sync for one specific case, namely when examining
the linkage name.  This avoids regressions in an existing DWARF test
case.

A full fix for this problem would be preferable.  An accessor like
DW_STRING should always check the form.  However, I haven't attempted
that in this series.

Also the fact that the partial and full readers can disagree like this
is a design flaw.

gdb/ChangeLog
2020-03-25  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (partial_die_info::read) <case
	DW_AT_linkage_name>: Use value_as_string.
	(dwarf2_string_attr): Use value_as_string.
	* dwarf2/attribute.h (struct attribute) <value_as_string>: Declare
	method.
	* dwarf2/attribute.c (attribute::value_as_string): New method.
---
 gdb/ChangeLog          |  9 +++++++++
 gdb/dwarf2/attribute.c | 18 ++++++++++++++++++
 gdb/dwarf2/attribute.h |  4 ++++
 gdb/dwarf2/read.c      | 15 +++------------
 4 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
index 0e5a8c8f536..a4a6db76c8f 100644
--- a/gdb/dwarf2/attribute.c
+++ b/gdb/dwarf2/attribute.c
@@ -61,6 +61,24 @@ attribute::value_as_address () const
 
 /* See attribute.h.  */
 
+const char *
+attribute::value_as_string () const
+{
+  if (form == DW_FORM_strp || form == DW_FORM_line_strp
+      || form == DW_FORM_string
+      || form == DW_FORM_strx
+      || form == DW_FORM_strx1
+      || form == DW_FORM_strx2
+      || form == DW_FORM_strx3
+      || form == DW_FORM_strx4
+      || form == DW_FORM_GNU_str_index
+      || form == DW_FORM_GNU_strp_alt)
+    return DW_STRING (this);
+  return nullptr;
+}
+
+/* See attribute.h.  */
+
 bool
 attribute::form_is_block () const
 {
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index 69b33513ad6..a9cabd69f9f 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -46,6 +46,10 @@ struct attribute
      attribute's form into account.  */
   CORE_ADDR value_as_address () const;
 
+  /* If the attribute has a string form, return the string value;
+     otherwise return NULL.  */
+  const char *value_as_string () const;
+
   /* Return non-zero if ATTR's value is a section offset --- classes
      lineptr, loclistptr, macptr or rangelistptr --- or zero, otherwise.
      You may use DW_UNSND (attr) to retrieve such offsets.
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 8c5046ef41c..98323f8920b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -17896,7 +17896,7 @@ partial_die_info::read (const struct die_reader_specs *reader,
 	  /* Note that both forms of linkage name might appear.  We
 	     assume they will be the same, and we only store the last
 	     one we see.  */
-	  linkage_name = DW_STRING (&attr);
+	  linkage_name = attr.value_as_string ();
 	  break;
 	case DW_AT_low_pc:
 	  has_low_pc_attr = 1;
@@ -18949,17 +18949,8 @@ dwarf2_string_attr (struct die_info *die, unsigned int name, struct dwarf2_cu *c
 
   if (attr != NULL)
     {
-      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);
-      else
+      str = attr->value_as_string ();
+      if (str == nullptr)
         complaint (_("string type expected for attribute %s for "
 		     "DIE at %s in module %s"),
 		   dwarf_attr_name (name), sect_offset_str (die->sect_off),
-- 
2.17.2

[PATCH 02/20] Rename struct attribute accessors

[Tom Tromey]

This removes the "value_as_" prefix from the struct value accessors.
This seemed unnecessarily wordy to me.

gdb/ChangeLog
2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (dwarf2_find_base_address, read_call_site_scope)
	(dwarf2_get_pc_bounds, dwarf2_record_block_ranges)
	(partial_die_info::read, dwarf2_string_attr, new_symbol): Update.
	* dwarf2/attribute.h (struct attribute): Rename methods.
	* dwarf2/attribute.c (attribute::address): Rename from
	value_as_address.
	(attribute::string): Rename from value_as_string.
---
 gdb/ChangeLog          | 10 ++++++++++
 gdb/dwarf2/attribute.c |  4 ++--
 gdb/dwarf2/attribute.h |  4 ++--
 gdb/dwarf2/read.c      | 24 ++++++++++++------------
 4 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
index a4a6db76c8f..1bdd4cf7abb 100644
--- a/gdb/dwarf2/attribute.c
+++ b/gdb/dwarf2/attribute.c
@@ -32,7 +32,7 @@
 /* See attribute.h.  */
 
 CORE_ADDR
-attribute::value_as_address () const
+attribute::address () const
 {
   CORE_ADDR addr;
 
@@ -62,7 +62,7 @@ attribute::value_as_address () const
 /* See attribute.h.  */
 
 const char *
-attribute::value_as_string () const
+attribute::string () const
 {
   if (form == DW_FORM_strp || form == DW_FORM_line_strp
       || form == DW_FORM_string
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index a9cabd69f9f..cefd3c5541e 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -44,11 +44,11 @@ struct attribute
 {
   /* Read the given attribute value as an address, taking the
      attribute's form into account.  */
-  CORE_ADDR value_as_address () const;
+  CORE_ADDR address () const;
 
   /* If the attribute has a string form, return the string value;
      otherwise return NULL.  */
-  const char *value_as_string () const;
+  const char *string () const;
 
   /* Return non-zero if ATTR's value is a section offset --- classes
      lineptr, loclistptr, macptr or rangelistptr --- or zero, otherwise.
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 98323f8920b..d2b274a6e3a 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -5766,12 +5766,12 @@ dwarf2_find_base_address (struct die_info *die, struct dwarf2_cu *cu)
 
   attr = dwarf2_attr (die, DW_AT_entry_pc, cu);
   if (attr != nullptr)
-    cu->base_address = attr->value_as_address ();
+    cu->base_address = attr->address ();
   else
     {
       attr = dwarf2_attr (die, DW_AT_low_pc, cu);
       if (attr != nullptr)
-	cu->base_address = attr->value_as_address ();
+	cu->base_address = attr->address ();
     }
 }
 
@@ -13022,7 +13022,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
 		 sect_offset_str (die->sect_off), objfile_name (objfile));
       return;
     }
-  pc = attr->value_as_address () + baseaddr;
+  pc = attr->address () + baseaddr;
   pc = gdbarch_adjust_dwarf2_addr (gdbarch, pc);
 
   if (cu->call_site_htab == NULL)
@@ -13724,8 +13724,8 @@ dwarf2_get_pc_bounds (struct die_info *die, CORE_ADDR *lowpc,
       attr = dwarf2_attr (die, DW_AT_low_pc, cu);
       if (attr != nullptr)
         {
-	  low = attr->value_as_address ();
-	  high = attr_high->value_as_address ();
+	  low = attr->address ();
+	  high = attr_high->address ();
 	  if (cu->header.version >= 4 && attr_high->form_is_constant ())
 	    high += low;
 	}
@@ -13897,8 +13897,8 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
       attr = dwarf2_attr (die, DW_AT_low_pc, cu);
       if (attr != nullptr)
         {
-          CORE_ADDR low = attr->value_as_address ();
-	  CORE_ADDR high = attr_high->value_as_address ();
+          CORE_ADDR low = attr->address ();
+	  CORE_ADDR high = attr_high->address ();
 
 	  if (cu->header.version >= 4 && attr_high->form_is_constant ())
 	    high += low;
@@ -17896,15 +17896,15 @@ partial_die_info::read (const struct die_reader_specs *reader,
 	  /* Note that both forms of linkage name might appear.  We
 	     assume they will be the same, and we only store the last
 	     one we see.  */
-	  linkage_name = attr.value_as_string ();
+	  linkage_name = attr.string ();
 	  break;
 	case DW_AT_low_pc:
 	  has_low_pc_attr = 1;
-	  lowpc = attr.value_as_address ();
+	  lowpc = attr.address ();
 	  break;
 	case DW_AT_high_pc:
 	  has_high_pc_attr = 1;
-	  highpc = attr.value_as_address ();
+	  highpc = attr.address ();
 	  if (cu->header.version >= 4 && attr.form_is_constant ())
 		high_pc_relative = 1;
 	  break;
@@ -18949,7 +18949,7 @@ dwarf2_string_attr (struct die_info *die, unsigned int name, struct dwarf2_cu *c
 
   if (attr != NULL)
     {
-      str = attr->value_as_string ();
+      str = attr->string ();
       if (str == nullptr)
         complaint (_("string type expected for attribute %s for "
 		     "DIE at %s in module %s"),
@@ -20076,7 +20076,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 	    {
 	      CORE_ADDR addr;
 
-	      addr = attr->value_as_address ();
+	      addr = attr->address ();
 	      addr = gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);
 	      SET_SYMBOL_VALUE_ADDRESS (sym, addr);
 	    }
-- 
2.17.2

RE: [PATCH 02/20] Rename struct attribute accessors

[Aktemur, Tankut Baris]

On Saturday, March 28, 2020 8:22 PM, Tom Tromey wrote:
> This removes the "value_as_" prefix from the struct value accessors.
> This seemed unnecessarily wordy to me.
> 
> gdb/ChangeLog
> 2020-03-28  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2/read.c (dwarf2_find_base_address, read_call_site_scope)
> 	(dwarf2_get_pc_bounds, dwarf2_record_block_ranges)
> 	(partial_die_info::read, dwarf2_string_attr, new_symbol): Update.
> 	* dwarf2/attribute.h (struct attribute): Rename methods.
> 	* dwarf2/attribute.c (attribute::address): Rename from
> 	value_as_address.
> 	(attribute::string): Rename from value_as_string.
> ---
>  gdb/ChangeLog          | 10 ++++++++++
>  gdb/dwarf2/attribute.c |  4 ++--
>  gdb/dwarf2/attribute.h |  4 ++--
>  gdb/dwarf2/read.c      | 24 ++++++++++++------------
>  4 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
> index a4a6db76c8f..1bdd4cf7abb 100644
> --- a/gdb/dwarf2/attribute.c
> +++ b/gdb/dwarf2/attribute.c
> @@ -32,7 +32,7 @@
>  /* See attribute.h.  */
> 
>  CORE_ADDR
> -attribute::value_as_address () const
> +attribute::address () const
>  {
>    CORE_ADDR addr;
> 
> @@ -62,7 +62,7 @@ attribute::value_as_address () const
>  /* See attribute.h.  */
> 
>  const char *
> -attribute::value_as_string () const
> +attribute::string () const
>  {
>    if (form == DW_FORM_strp || form == DW_FORM_line_strp
>        || form == DW_FORM_string
> diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
> index a9cabd69f9f..cefd3c5541e 100644
> --- a/gdb/dwarf2/attribute.h
> +++ b/gdb/dwarf2/attribute.h
> @@ -44,11 +44,11 @@ struct attribute
>  {
>    /* Read the given attribute value as an address, taking the
>       attribute's form into account.  */
> -  CORE_ADDR value_as_address () const;
> +  CORE_ADDR address () const;
> 
>    /* If the attribute has a string form, return the string value;
>       otherwise return NULL.  */
> -  const char *value_as_string () const;
> +  const char *string () const;
> 
>    /* Return non-zero if ATTR's value is a section offset --- classes
>       lineptr, loclistptr, macptr or rangelistptr --- or zero, otherwise.
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 98323f8920b..d2b274a6e3a 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -5766,12 +5766,12 @@ dwarf2_find_base_address (struct die_info *die, struct dwarf2_cu *cu)
> 
>    attr = dwarf2_attr (die, DW_AT_entry_pc, cu);
>    if (attr != nullptr)
> -    cu->base_address = attr->value_as_address ();
> +    cu->base_address = attr->address ();
>    else
>      {
>        attr = dwarf2_attr (die, DW_AT_low_pc, cu);
>        if (attr != nullptr)
> -	cu->base_address = attr->value_as_address ();
> +	cu->base_address = attr->address ();
>      }
>  }
> 
> @@ -13022,7 +13022,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
>  		 sect_offset_str (die->sect_off), objfile_name (objfile));
>        return;
>      }
> -  pc = attr->value_as_address () + baseaddr;
> +  pc = attr->address () + baseaddr;
>    pc = gdbarch_adjust_dwarf2_addr (gdbarch, pc);
> 
>    if (cu->call_site_htab == NULL)
> @@ -13724,8 +13724,8 @@ dwarf2_get_pc_bounds (struct die_info *die, CORE_ADDR *lowpc,
>        attr = dwarf2_attr (die, DW_AT_low_pc, cu);
>        if (attr != nullptr)
>          {
> -	  low = attr->value_as_address ();
> -	  high = attr_high->value_as_address ();
> +	  low = attr->address ();
> +	  high = attr_high->address ();
>  	  if (cu->header.version >= 4 && attr_high->form_is_constant ())
>  	    high += low;
>  	}
> @@ -13897,8 +13897,8 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
>        attr = dwarf2_attr (die, DW_AT_low_pc, cu);
>        if (attr != nullptr)
>          {
> -          CORE_ADDR low = attr->value_as_address ();
> -	  CORE_ADDR high = attr_high->value_as_address ();
> +          CORE_ADDR low = attr->address ();

The existing code had 8 spaces that can now be replaced with a tab.

Thanks.
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH 02/20] Rename struct attribute accessors

[Tom Tromey]

>>>>> ">" == Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com> writes:

>> -          CORE_ADDR low = attr->value_as_address ();
>> -	  CORE_ADDR high = attr_high->value_as_address ();
>> +          CORE_ADDR low = attr->address ();

>> The existing code had 8 spaces that can now be replaced with a tab.

Thanks, I changed this.

Tom

Re: [PATCH 02/20] Rename struct attribute accessors

[Simon Marchi]

On 2020-03-28 3:21 p.m., Tom Tromey wrote:
> This removes the "value_as_" prefix from the struct value accessors.
> This seemed unnecessarily wordy to me.

I would perhaps use "as_string" and "as_address" rather than just
"string" and "address".  At least for "address", it could be
interpreted by some as returning the address (pointer) of the
attribute object.  The "as_" prefix makes me naturally read it as
"interpret this attribute as a string", "interpret this attribute as
an address".

Not a big deal though, I'm also fine with the patch as-is.

Simon

Re: [PATCH 02/20] Rename struct attribute accessors

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I would perhaps use "as_string" and "as_address" rather than just
Simon> "string" and "address".

I considered this but didn't do it.  But, I'm making the change now.

Tom

[PATCH 03/20] Avoid using DW_* macros in dwarf2/attribute.c

[Tom Tromey]

There's no need to use the DW_* accessor macros in dwarf2/attribute.c,
and this is a necessary step toward our goal of removing them
entirely.

gdb/ChangeLog
2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/attribute.c (attribute::address): Don't use DW_UNSND or
	DW_ADDR.
	(attribute::string): Don't use DW_STRING.
	(attribute::get_ref_die_offset): Don't use DW_UNSND.
	(attribute::constant_value): Don't use DW_UNSND or DW_SND.
---
 gdb/ChangeLog          |  8 ++++++++
 gdb/dwarf2/attribute.c | 12 ++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
index 1bdd4cf7abb..634b7979143 100644
--- a/gdb/dwarf2/attribute.c
+++ b/gdb/dwarf2/attribute.c
@@ -51,10 +51,10 @@ attribute::address () const
 	 as well as update callers to pass in at least the CU's DWARF
 	 version.  This is more overhead than what we're willing to
 	 expand for a pretty rare case.  */
-      addr = DW_UNSND (this);
+      addr = u.unsnd;
     }
   else
-    addr = DW_ADDR (this);
+    addr = u.addr;
 
   return addr;
 }
@@ -73,7 +73,7 @@ attribute::string () const
       || form == DW_FORM_strx4
       || form == DW_FORM_GNU_str_index
       || form == DW_FORM_GNU_strp_alt)
-    return DW_STRING (this);
+    return u.str;
   return nullptr;
 }
 
@@ -146,7 +146,7 @@ sect_offset
 attribute::get_ref_die_offset () const
 {
   if (form_is_ref ())
-    return (sect_offset) DW_UNSND (this);
+    return (sect_offset) u.unsnd;
 
   complaint (_("unsupported die ref attribute form: '%s'"),
 	     dwarf_form_name (form));
@@ -159,13 +159,13 @@ LONGEST
 attribute::constant_value (int default_value) const
 {
   if (form == DW_FORM_sdata || form == DW_FORM_implicit_const)
-    return DW_SND (this);
+    return u.snd;
   else if (form == DW_FORM_udata
 	   || form == DW_FORM_data1
 	   || form == DW_FORM_data2
 	   || form == DW_FORM_data4
 	   || form == DW_FORM_data8)
-    return DW_UNSND (this);
+    return u.unsnd;
   else
     {
       /* For DW_FORM_data16 see attribute::form_is_constant.  */
-- 
2.17.2

[PATCH 04/20] Change some uses of DW_STRING to string method

[Tom Tromey]

This changes some of the simpler spots to use attribute::string rather
than DW_STRING.

2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (partial_die_info::read)
	(dwarf2_const_value_attr, anonymous_struct_prefix, )
	(dwarf2_name, dwarf2_fetch_constant_bytes): Use
	attribute::string.
---
 gdb/ChangeLog     |  7 +++++++
 gdb/dwarf2/read.c | 52 +++++++++++++++++++++++------------------------
 2 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d2b274a6e3a..eb5ee98de60 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -17879,14 +17879,14 @@ partial_die_info::read (const struct die_reader_specs *reader,
 	    case DW_TAG_enumerator:
 	      /* These tags always have simple identifiers already; no need
 		 to canonicalize them.  */
-	      name = DW_STRING (&attr);
+	      name = attr.string ();
 	      break;
 	    default:
 	      {
 		struct objfile *objfile = dwarf2_per_objfile->objfile;
 
 		name
-		  = dwarf2_canonicalize_name (DW_STRING (&attr), cu, objfile);
+		  = dwarf2_canonicalize_name (attr.string (), cu, objfile);
 	      }
 	      break;
 	    }
@@ -20482,7 +20482,7 @@ dwarf2_const_value_attr (const struct attribute *attr, struct type *type,
     case DW_FORM_GNU_strp_alt:
       /* DW_STRING is already allocated on the objfile obstack, point
 	 directly to it.  */
-      *bytes = (const gdb_byte *) DW_STRING (attr);
+      *bytes = (const gdb_byte *) attr->string ();
       break;
     case DW_FORM_block1:
     case DW_FORM_block2:
@@ -20930,21 +20930,21 @@ anonymous_struct_prefix (struct die_info *die, struct dwarf2_cu *cu)
     return NULL;
 
   attr = dw2_linkage_name_attr (die, cu);
-  if (attr == NULL || DW_STRING (attr) == NULL)
+  if (attr == NULL || attr->string () == NULL)
     return NULL;
 
   /* dwarf2_name had to be already called.  */
   gdb_assert (DW_STRING_IS_CANONICAL (attr));
 
   /* Strip the base name, keep any leading namespaces/classes.  */
-  base = strrchr (DW_STRING (attr), ':');
-  if (base == NULL || base == DW_STRING (attr) || base[-1] != ':')
+  base = strrchr (attr->string (), ':');
+  if (base == NULL || base == attr->string () || base[-1] != ':')
     return "";
 
   struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
   return obstack_strndup (&objfile->per_bfd->storage_obstack,
-			  DW_STRING (attr),
-			  &base[-1] - DW_STRING (attr));
+			  attr->string (),
+			  &base[-1] - attr->string ());
 }
 
 /* Return the name of the namespace/class that DIE is defined within,
@@ -21214,7 +21214,7 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
   struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
 
   attr = dwarf2_attr (die, DW_AT_name, cu);
-  if ((!attr || !DW_STRING (attr))
+  if ((!attr || !attr->string ())
       && die->tag != DW_TAG_namespace
       && die->tag != DW_TAG_class_type
       && die->tag != DW_TAG_interface_type
@@ -21232,11 +21232,11 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
     case DW_TAG_enumerator:
       /* These tags always have simple identifiers already; no need
 	 to canonicalize them.  */
-      return DW_STRING (attr);
+      return attr->string ();
 
     case DW_TAG_namespace:
-      if (attr != NULL && DW_STRING (attr) != NULL)
-	return DW_STRING (attr);
+      if (attr != NULL && attr->string () != NULL)
+	return attr->string ();
       return CP_ANONYMOUS_NAMESPACE_STR;
 
     case DW_TAG_class_type:
@@ -21247,25 +21247,25 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
 	 structures or unions.  These were of the form "._%d" in GCC 4.1,
 	 or simply "<anonymous struct>" or "<anonymous union>" in GCC 4.3
 	 and GCC 4.4.  We work around this problem by ignoring these.  */
-      if (attr && DW_STRING (attr)
-	  && (startswith (DW_STRING (attr), "._")
-	      || startswith (DW_STRING (attr), "<anonymous")))
+      if (attr && attr->string ()
+	  && (startswith (attr->string (), "._")
+	      || startswith (attr->string (), "<anonymous")))
 	return NULL;
 
       /* GCC might emit a nameless typedef that has a linkage name.  See
 	 http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47510.  */
-      if (!attr || DW_STRING (attr) == NULL)
+      if (!attr || attr->string () == NULL)
 	{
 	  attr = dw2_linkage_name_attr (die, cu);
-	  if (attr == NULL || DW_STRING (attr) == NULL)
+	  if (attr == NULL || attr->string () == NULL)
 	    return NULL;
 
-	  /* Avoid demangling DW_STRING (attr) the second time on a second
+	  /* Avoid demangling attr->string () the second time on a second
 	     call for the same DIE.  */
 	  if (!DW_STRING_IS_CANONICAL (attr))
 	    {
 	      gdb::unique_xmalloc_ptr<char> demangled
-		(gdb_demangle (DW_STRING (attr), DMGL_TYPES));
+		(gdb_demangle (attr->string (), DMGL_TYPES));
 	      if (demangled == nullptr)
 		return nullptr;
 
@@ -21275,11 +21275,11 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
 
 	  /* Strip any leading namespaces/classes, keep only the base name.
 	     DW_AT_name for named DIEs does not contain the prefixes.  */
-	  const char *base = strrchr (DW_STRING (attr), ':');
-	  if (base && base > DW_STRING (attr) && base[-1] == ':')
+	  const char *base = strrchr (attr->string (), ':');
+	  if (base && base > attr->string () && base[-1] == ':')
 	    return &base[1];
 	  else
-	    return DW_STRING (attr);
+	    return attr->string ();
 	}
       break;
 
@@ -21289,11 +21289,11 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
 
   if (!DW_STRING_IS_CANONICAL (attr))
     {
-      DW_STRING (attr) = dwarf2_canonicalize_name (DW_STRING (attr), cu,
+      DW_STRING (attr) = dwarf2_canonicalize_name (attr->string (), cu,
 						   objfile);
       DW_STRING_IS_CANONICAL (attr) = 1;
     }
-  return DW_STRING (attr);
+  return attr->string ();
 }
 
 /* Return the die that this die in an extension of, or NULL if there
@@ -21801,8 +21801,8 @@ dwarf2_fetch_constant_bytes (sect_offset sect_off,
     case DW_FORM_GNU_strp_alt:
       /* DW_STRING is already allocated on the objfile obstack, point
 	 directly to it.  */
-      result = (const gdb_byte *) DW_STRING (attr);
-      *len = strlen (DW_STRING (attr));
+      result = (const gdb_byte *) attr->string ();
+      *len = strlen (attr->string ());
       break;
     case DW_FORM_block1:
     case DW_FORM_block2:
-- 
2.17.2

Re: [PATCH 04/20] Change some uses of DW_STRING to string method

[Simon Marchi]

On 2020-03-28 3:21 p.m., Tom Tromey wrote:
> @@ -21247,25 +21247,25 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
>  	 structures or unions.  These were of the form "._%d" in GCC 4.1,
>  	 or simply "<anonymous struct>" or "<anonymous union>" in GCC 4.3
>  	 and GCC 4.4.  We work around this problem by ignoring these.  */
> -      if (attr && DW_STRING (attr)
> -	  && (startswith (DW_STRING (attr), "._")
> -	      || startswith (DW_STRING (attr), "<anonymous")))
> +      if (attr && attr->string ()
> +	  && (startswith (attr->string (), "._")
> +	      || startswith (attr->string (), "<anonymous")))

In cases like this, I think it would be worth assigning the result of attr->string
to a local variable, to avoid paying the cost of the check multiple times
unnecessarily.

But otherwise, this LGTM.

Simon

Re: [PATCH 04/20] Change some uses of DW_STRING to string method

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> In cases like this, I think it would be worth assigning the result of attr->string
Simon> to a local variable, to avoid paying the cost of the check multiple times
Simon> unnecessarily.

I made this change.

I did some profiling of the DWARF reader recently and I think that some
of these methods should probably be inlined as well.  I'll double-check
... my profiling was on a branch without this series, but inlining
form_is_ref and part of get_ref_die_offset was a win.

Tom

[PATCH 05/20] Remove some uses of DW_STRING_IS_CANONICAL

[Tom Tromey]

This removes the rvalue uses of DW_STRING_IS_CANONICAL, replacing them
with an accessor method.

2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (anonymous_struct_prefix, dwarf2_name)
	(dump_die_shallow): Use canonical_p.
	* dwarf2/attribute.h (struct attribute) <canonical_p>: New
	method.
---
 gdb/ChangeLog          | 7 +++++++
 gdb/dwarf2/attribute.h | 6 ++++++
 gdb/dwarf2/read.c      | 8 ++++----
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index cefd3c5541e..f20540559aa 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -100,6 +100,12 @@ struct attribute
 
   LONGEST constant_value (int default_value) const;
 
+  /* Return true if this attribute holds a canonical string.  */
+  bool canonical_p () const
+  {
+    return string_is_canonical;
+  }
+
 
   ENUM_BITFIELD(dwarf_attribute) name : 16;
   ENUM_BITFIELD(dwarf_form) form : 15;
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index eb5ee98de60..4b102e52e88 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -20934,7 +20934,7 @@ anonymous_struct_prefix (struct die_info *die, struct dwarf2_cu *cu)
     return NULL;
 
   /* dwarf2_name had to be already called.  */
-  gdb_assert (DW_STRING_IS_CANONICAL (attr));
+  gdb_assert (attr->canonical_p ());
 
   /* Strip the base name, keep any leading namespaces/classes.  */
   base = strrchr (attr->string (), ':');
@@ -21262,7 +21262,7 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
 
 	  /* Avoid demangling attr->string () the second time on a second
 	     call for the same DIE.  */
-	  if (!DW_STRING_IS_CANONICAL (attr))
+	  if (!attr->canonical_p ())
 	    {
 	      gdb::unique_xmalloc_ptr<char> demangled
 		(gdb_demangle (attr->string (), DMGL_TYPES));
@@ -21287,7 +21287,7 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
       break;
     }
 
-  if (!DW_STRING_IS_CANONICAL (attr))
+  if (!attr->canonical_p ())
     {
       DW_STRING (attr) = dwarf2_canonicalize_name (attr->string (), cu,
 						   objfile);
@@ -21407,7 +21407,7 @@ dump_die_shallow (struct ui_file *f, int indent, struct die_info *die)
 	  fprintf_unfiltered (f, "string: \"%s\" (%s canonicalized)",
 		   DW_STRING (&die->attrs[i])
 		   ? DW_STRING (&die->attrs[i]) : "",
-		   DW_STRING_IS_CANONICAL (&die->attrs[i]) ? "is" : "not");
+		   die->attrs[i].canonical_p () ? "is" : "not");
 	  break;
 	case DW_FORM_flag:
 	  if (DW_UNSND (&die->attrs[i]))
-- 
2.17.2

Re: [PATCH 05/20] Remove some uses of DW_STRING_IS_CANONICAL

[Simon Marchi]

On 2020-03-28 3:21 p.m., Tom Tromey wrote:
> This removes the rvalue uses of DW_STRING_IS_CANONICAL, replacing them
> with an accessor method.
> 
> 2020-03-28  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2/read.c (anonymous_struct_prefix, dwarf2_name)
> 	(dump_die_shallow): Use canonical_p.
> 	* dwarf2/attribute.h (struct attribute) <canonical_p>: New
> 	method.
> ---
>  gdb/ChangeLog          | 7 +++++++
>  gdb/dwarf2/attribute.h | 6 ++++++
>  gdb/dwarf2/read.c      | 8 ++++----
>  3 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
> index cefd3c5541e..f20540559aa 100644
> --- a/gdb/dwarf2/attribute.h
> +++ b/gdb/dwarf2/attribute.h
> @@ -100,6 +100,12 @@ struct attribute
>  
>    LONGEST constant_value (int default_value) const;
>  
> +  /* Return true if this attribute holds a canonical string.  */
> +  bool canonical_p () const
> +  {
> +    return string_is_canonical;
> +  }

Would you be able to improve the documentation to say what is
a "canonical string"?  If this is a DWARF concept and described
in the DWARF spec, you could just refer to the relevant section
of the standard too.

Is this method only relevant if the attribute is of a string
form?  If so, the method name should perhaps still have "string"
in its name, like "is_canonical_string" or "canonical_string_p".

I presume it doesn't make sense to call this method when the
attribute has a non-string form, so should there be a gdb_assert
that checks that the attribute has a string form?

Simon

Re: [PATCH 05/20] Remove some uses of DW_STRING_IS_CANONICAL

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Would you be able to improve the documentation to say what is
Simon> a "canonical string"?

Yeah, I di dthis.

Simon> If this is a DWARF concept and described
Simon> in the DWARF spec, you could just refer to the relevant section
Simon> of the standard too.

Just FYI, it isn't; it is a gdb concept.

Simon> Is this method only relevant if the attribute is of a string
Simon> form?  If so, the method name should perhaps still have "string"
Simon> in its name, like "is_canonical_string" or "canonical_string_p".

Done.

Simon> I presume it doesn't make sense to call this method when the
Simon> attribute has a non-string form, so should there be a gdb_assert
Simon> that checks that the attribute has a string form?

I had to move this to the next patch, since form_is_string doesn't exist
yet.

Tom

[PATCH 06/20] Remove DW_STRING and DW_STRING_IS_CANONICAL

[Tom Tromey]

This removes DW_STRING and DW_STRING_IS_CANONICAL, replacing them with
accessor methods on struct attribute.  The new code ensures that a
string value will only ever be used when the form allows it.

2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (read_cutu_die_from_dwo)
	(read_attribute_reprocess, read_attribute_value, read_attribute)
	(dwarf2_const_value_attr, dwarf2_name, dump_die_shallow)
	(dwarf2_fetch_constant_bytes): Update.
	* dwarf2/attribute.h (struct attribute) <form_is_string>: Declare.
	<string_init, string_set>: New methods.
	<string_is_canonical>: Update comment.
	(DW_STRING, DW_STRING_IS_CANONICAL): Remove.
	* dwarf2/attribute.c (attribute::form_is_string): New method.
	(attribute::string): Use it.
---
 gdb/ChangeLog          | 13 +++++++++++
 gdb/dwarf2/attribute.c | 26 ++++++++++++++-------
 gdb/dwarf2/attribute.h | 27 +++++++++++++++++----
 gdb/dwarf2/read.c      | 53 ++++++++++++++++--------------------------
 4 files changed, 72 insertions(+), 47 deletions(-)

diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
index 634b7979143..06b3245e4b2 100644
--- a/gdb/dwarf2/attribute.c
+++ b/gdb/dwarf2/attribute.c
@@ -61,18 +61,26 @@ attribute::address () const
 
 /* See attribute.h.  */
 
+bool
+attribute::form_is_string () const
+{
+  return (form == DW_FORM_strp || form == DW_FORM_line_strp
+	  || form == DW_FORM_string
+	  || form == DW_FORM_strx
+	  || form == DW_FORM_strx1
+	  || form == DW_FORM_strx2
+	  || form == DW_FORM_strx3
+	  || form == DW_FORM_strx4
+	  || form == DW_FORM_GNU_str_index
+	  || form == DW_FORM_GNU_strp_alt);
+}
+
+/* See attribute.h.  */
+
 const char *
 attribute::string () const
 {
-  if (form == DW_FORM_strp || form == DW_FORM_line_strp
-      || form == DW_FORM_string
-      || form == DW_FORM_strx
-      || form == DW_FORM_strx1
-      || form == DW_FORM_strx2
-      || form == DW_FORM_strx3
-      || form == DW_FORM_strx4
-      || form == DW_FORM_GNU_str_index
-      || form == DW_FORM_GNU_strp_alt)
+  if (form_is_string ())
     return u.str;
   return nullptr;
 }
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index f20540559aa..49989211018 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -89,6 +89,9 @@ struct attribute
 
   bool form_is_block () const;
 
+  /* Check if the attribute's form is a string form.  */
+  bool form_is_string () const;
+
   /* Return DIE offset of this attribute.  Return 0 with complaint if
      the attribute is not of the required kind.  */
 
@@ -106,13 +109,29 @@ struct attribute
     return string_is_canonical;
   }
 
+  /* Initialize this attribute to hold a string value.  */
+  void string_init (const char *str)
+  {
+    gdb_assert (form_is_string ());
+    u.str = str;
+    string_is_canonical = 0;
+  }
+
+  /* Set the canonical string value for this attribute.  */
+  void string_set (const char *str)
+  {
+    gdb_assert (form_is_string ());
+    u.str = str;
+    string_is_canonical = 1;
+  }
+
 
   ENUM_BITFIELD(dwarf_attribute) name : 16;
   ENUM_BITFIELD(dwarf_form) form : 15;
 
-  /* Has DW_STRING already been updated by dwarf2_canonicalize_name?  This
-     field should be in u.str (existing only for DW_STRING) but it is kept
-     here for better struct attribute alignment.  */
+  /* Has u.str already been updated by dwarf2_canonicalize_name?  This
+     field should be in u.str but it is kept here for better struct
+     attribute alignment.  */
   unsigned int string_is_canonical : 1;
 
   union
@@ -129,8 +148,6 @@ struct attribute
 
 /* Get at parts of an attribute structure.  */
 
-#define DW_STRING(attr)    ((attr)->u.str)
-#define DW_STRING_IS_CANONICAL(attr) ((attr)->string_is_canonical)
 #define DW_UNSND(attr)     ((attr)->u.unsnd)
 #define DW_BLOCK(attr)     ((attr)->u.blk)
 #define DW_SND(attr)       ((attr)->u.snd)
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 4b102e52e88..e3223e92d43 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6474,8 +6474,7 @@ read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
       comp_dir = XOBNEW (&cu->comp_unit_obstack, struct attribute);
       comp_dir->name = DW_AT_comp_dir;
       comp_dir->form = DW_FORM_string;
-      DW_STRING_IS_CANONICAL (comp_dir) = 0;
-      DW_STRING (comp_dir) = stub_comp_dir;
+      comp_dir->string_init (stub_comp_dir);
     }
 
   /* Set up for reading the DWO CU/TU.  */
@@ -18321,16 +18320,11 @@ read_attribute_reprocess (const struct die_reader_specs *reader,
       case DW_FORM_GNU_str_index:
 	{
 	  unsigned int str_index = DW_UNSND (attr);
+	  gdb_assert (!attr->canonical_p ());
 	  if (reader->dwo_file != NULL)
-	    {
-	      DW_STRING (attr) = read_dwo_str_index (reader, str_index);
-	      DW_STRING_IS_CANONICAL (attr) = 0;
-	    }
+	    attr->string_init (read_dwo_str_index (reader, str_index));
 	  else
-	    {
-	      DW_STRING (attr) = read_stub_str_index (cu, str_index);
-	      DW_STRING_IS_CANONICAL (attr) = 0;
-	    }
+	    attr->string_init (read_stub_str_index (cu, str_index));
 	  break;
 	}
       default:
@@ -18418,17 +18412,15 @@ read_attribute_value (const struct die_reader_specs *reader,
       info_ptr += bytes_read;
       break;
     case DW_FORM_string:
-      DW_STRING (attr) = read_direct_string (abfd, info_ptr, &bytes_read);
-      DW_STRING_IS_CANONICAL (attr) = 0;
+      attr->string_init (read_direct_string (abfd, info_ptr, &bytes_read));
       info_ptr += bytes_read;
       break;
     case DW_FORM_strp:
       if (!cu->per_cu->is_dwz)
 	{
-	  DW_STRING (attr) = read_indirect_string (dwarf2_per_objfile,
+	  attr->string_init (read_indirect_string (dwarf2_per_objfile,
 						   abfd, info_ptr, cu_header,
-						   &bytes_read);
-	  DW_STRING_IS_CANONICAL (attr) = 0;
+						   &bytes_read));
 	  info_ptr += bytes_read;
 	  break;
 	}
@@ -18436,10 +18428,9 @@ read_attribute_value (const struct die_reader_specs *reader,
     case DW_FORM_line_strp:
       if (!cu->per_cu->is_dwz)
 	{
-	  DW_STRING (attr)
-	    = dwarf2_per_objfile->read_line_string (info_ptr, cu_header,
-						    &bytes_read);
-	  DW_STRING_IS_CANONICAL (attr) = 0;
+	  attr->string_init
+	    (dwarf2_per_objfile->read_line_string (info_ptr, cu_header,
+						   &bytes_read));
 	  info_ptr += bytes_read;
 	  break;
 	}
@@ -18450,8 +18441,7 @@ read_attribute_value (const struct die_reader_specs *reader,
 	LONGEST str_offset = cu_header->read_offset (abfd, info_ptr,
 						     &bytes_read);
 
-	DW_STRING (attr) = dwz->read_string (objfile, str_offset);
-	DW_STRING_IS_CANONICAL (attr) = 0;
+	attr->string_init (dwz->read_string (objfile, str_offset));
 	info_ptr += bytes_read;
       }
       break;
@@ -18615,6 +18605,7 @@ read_attribute (const struct die_reader_specs *reader,
 		const gdb_byte *info_ptr, bool *need_reprocess)
 {
   attr->name = abbrev->name;
+  attr->string_is_canonical = 0;
   return read_attribute_value (reader, attr, abbrev->form,
 			       abbrev->implicit_const, info_ptr,
 			       need_reprocess);
@@ -20480,7 +20471,7 @@ dwarf2_const_value_attr (const struct attribute *attr, struct type *type,
     case DW_FORM_strx:
     case DW_FORM_GNU_str_index:
     case DW_FORM_GNU_strp_alt:
-      /* DW_STRING is already allocated on the objfile obstack, point
+      /* The string is already allocated on the objfile obstack, point
 	 directly to it.  */
       *bytes = (const gdb_byte *) attr->string ();
       break;
@@ -21269,8 +21260,7 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
 	      if (demangled == nullptr)
 		return nullptr;
 
-	      DW_STRING (attr) = objfile->intern (demangled.get ());
-	      DW_STRING_IS_CANONICAL (attr) = 1;
+	      attr->string_set (objfile->intern (demangled.get ()));
 	    }
 
 	  /* Strip any leading namespaces/classes, keep only the base name.
@@ -21288,11 +21278,8 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
     }
 
   if (!attr->canonical_p ())
-    {
-      DW_STRING (attr) = dwarf2_canonicalize_name (attr->string (), cu,
-						   objfile);
-      DW_STRING_IS_CANONICAL (attr) = 1;
-    }
+    attr->string_set (dwarf2_canonicalize_name (attr->string (), cu,
+						objfile));
   return attr->string ();
 }
 
@@ -21405,9 +21392,9 @@ dump_die_shallow (struct ui_file *f, int indent, struct die_info *die)
 	case DW_FORM_GNU_str_index:
 	case DW_FORM_GNU_strp_alt:
 	  fprintf_unfiltered (f, "string: \"%s\" (%s canonicalized)",
-		   DW_STRING (&die->attrs[i])
-		   ? DW_STRING (&die->attrs[i]) : "",
-		   die->attrs[i].canonical_p () ? "is" : "not");
+			      die->attrs[i].string ()
+			      ? die->attrs[i].string () : "",
+			      die->attrs[i].canonical_p () ? "is" : "not");
 	  break;
 	case DW_FORM_flag:
 	  if (DW_UNSND (&die->attrs[i]))
@@ -21799,7 +21786,7 @@ dwarf2_fetch_constant_bytes (sect_offset sect_off,
     case DW_FORM_strx:
     case DW_FORM_GNU_str_index:
     case DW_FORM_GNU_strp_alt:
-      /* DW_STRING is already allocated on the objfile obstack, point
+      /* The string is already allocated on the objfile obstack, point
 	 directly to it.  */
       result = (const gdb_byte *) attr->string ();
       *len = strlen (attr->string ());
-- 
2.17.2

Re: [PATCH 06/20] Remove DW_STRING and DW_STRING_IS_CANONICAL

[Simon Marchi]

On 2020-03-28 3:21 p.m., Tom Tromey wrote:
> +  /* Initialize this attribute to hold a string value.  */
> +  void string_init (const char *str)
> +  {
> +    gdb_assert (form_is_string ());
> +    u.str = str;
> +    string_is_canonical = 0;
> +  }
> +
> +  /* Set the canonical string value for this attribute.  */
> +  void string_set (const char *str)
> +  {
> +    gdb_assert (form_is_string ());
> +    u.str = str;
> +    string_is_canonical = 1;
> +  }

I don't find the "string_init" and "string_set" names very descriptive.  When
reading them in their call sites, it's not really obvious to me what's the
difference.  Perhaps "set_string_canonical" and "set_string_non_canonical"?

Simon

Re: [PATCH 06/20] Remove DW_STRING and DW_STRING_IS_CANONICAL

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I don't find the "string_init" and "string_set" names very descriptive.  When
Simon> reading them in their call sites, it's not really obvious to me what's the
Simon> difference.  Perhaps "set_string_canonical" and "set_string_non_canonical"?

I did this.

Tom

[PATCH 07/20] Remove DW_BLOCK

[Tom Tromey]

This removes the DW_BLOCK accessor in favor of methods on struct
attribute.  The methods, unlike the access, check the form.

Note that DW_FORM_data16 had to be handled by form_is_block, because
in practice that is how we store values of this form.

2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (read_call_site_scope)
	(handle_data_member_location, dwarf2_add_member_fn)
	(mark_common_block_symbol_computed, attr_to_dynamic_prop)
	(partial_die_info::read, read_attribute_value)
	(var_decode_location, dwarf2_const_value_attr, dump_die_shallow)
	(dwarf2_fetch_die_loc_sect_off, dwarf2_fetch_constant_bytes)
	(dwarf2_symbol_mark_computed): Update.
	* dwarf2/attribute.h (struct attribute) <block, set_block>: New
	methods.
	(DW_BLOCK): Remove.
	* dwarf2/attribute.c (attribute::form_is_block): Add
	DW_FORM_data16.
---
 gdb/ChangeLog          |  15 ++++++
 gdb/dwarf2/attribute.c |   3 +-
 gdb/dwarf2/attribute.h |  15 +++++-
 gdb/dwarf2/read.c      | 102 ++++++++++++++++++++---------------------
 4 files changed, 82 insertions(+), 53 deletions(-)

diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
index 06b3245e4b2..fdf202033e9 100644
--- a/gdb/dwarf2/attribute.c
+++ b/gdb/dwarf2/attribute.c
@@ -94,7 +94,8 @@ attribute::form_is_block () const
 	  || form == DW_FORM_block2
 	  || form == DW_FORM_block4
 	  || form == DW_FORM_block
-	  || form == DW_FORM_exprloc);
+	  || form == DW_FORM_exprloc
+	  || form == DW_FORM_data16);
 }
 
 /* See attribute.h.  */
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index 49989211018..f59986f78a3 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -50,6 +50,13 @@ struct attribute
      otherwise return NULL.  */
   const char *string () const;
 
+  /* Return the block value.  The attribute must have block form.  */
+  dwarf_block *block () const
+  {
+    gdb_assert (form_is_block ());
+    return u.blk;
+  }
+
   /* Return non-zero if ATTR's value is a section offset --- classes
      lineptr, loclistptr, macptr or rangelistptr --- or zero, otherwise.
      You may use DW_UNSND (attr) to retrieve such offsets.
@@ -125,6 +132,13 @@ struct attribute
     string_is_canonical = 1;
   }
 
+  /* Set the block value for this attribute.  */
+  void set_block (dwarf_block *blk)
+  {
+    gdb_assert (form_is_block ());
+    u.blk = blk;
+  }
+
 
   ENUM_BITFIELD(dwarf_attribute) name : 16;
   ENUM_BITFIELD(dwarf_form) form : 15;
@@ -149,7 +163,6 @@ struct attribute
 /* Get at parts of an attribute structure.  */
 
 #define DW_UNSND(attr)     ((attr)->u.unsnd)
-#define DW_BLOCK(attr)     ((attr)->u.blk)
 #define DW_SND(attr)       ((attr)->u.snd)
 #define DW_ADDR(attr)	   ((attr)->u.addr)
 #define DW_SIGNATURE(attr) ((attr)->u.signature)
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index e3223e92d43..87bce7e51a9 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -13127,15 +13127,15 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
       attr = dwarf2_attr (die, DW_AT_abstract_origin, cu);
     }
   SET_FIELD_DWARF_BLOCK (call_site->target, NULL);
-  if (!attr || (attr->form_is_block () && DW_BLOCK (attr)->size == 0))
+  if (!attr || (attr->form_is_block () && attr->block ()->size == 0))
     /* Keep NULL DWARF_BLOCK.  */;
   else if (attr->form_is_block ())
     {
       struct dwarf2_locexpr_baton *dlbaton;
 
       dlbaton = XOBNEW (&objfile->objfile_obstack, struct dwarf2_locexpr_baton);
-      dlbaton->data = DW_BLOCK (attr)->data;
-      dlbaton->size = DW_BLOCK (attr)->size;
+      dlbaton->data = attr->block ()->data;
+      dlbaton->size = attr->block ()->size;
       dlbaton->per_cu = cu->per_cu;
 
       SET_FIELD_DWARF_BLOCK (call_site->target, dlbaton);
@@ -13244,11 +13244,11 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
       else
 	{
 	  parameter->u.dwarf_reg = dwarf_block_to_dwarf_reg
-	    (DW_BLOCK (loc)->data, &DW_BLOCK (loc)->data[DW_BLOCK (loc)->size]);
+	    (loc->block ()->data, &loc->block ()->data[loc->block ()->size]);
 	  if (parameter->u.dwarf_reg != -1)
 	    parameter->kind = CALL_SITE_PARAMETER_DWARF_REG;
-	  else if (dwarf_block_to_sp_offset (gdbarch, DW_BLOCK (loc)->data,
-				    &DW_BLOCK (loc)->data[DW_BLOCK (loc)->size],
+	  else if (dwarf_block_to_sp_offset (gdbarch, loc->block ()->data,
+				    &loc->block ()->data[loc->block ()->size],
 					     &parameter->u.fb_offset))
 	    parameter->kind = CALL_SITE_PARAMETER_FB_OFFSET;
 	  else
@@ -13274,8 +13274,8 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
 		     objfile_name (objfile));
 	  continue;
 	}
-      parameter->value = DW_BLOCK (attr)->data;
-      parameter->value_size = DW_BLOCK (attr)->size;
+      parameter->value = attr->block ()->data;
+      parameter->value_size = attr->block ()->size;
 
       /* Parameters are not pre-cleared by memset above.  */
       parameter->data_value = NULL;
@@ -13294,8 +13294,8 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
 		       objfile_name (objfile));
 	  else
 	    {
-	      parameter->data_value = DW_BLOCK (attr)->data;
-	      parameter->data_value_size = DW_BLOCK (attr)->size;
+	      parameter->data_value = attr->block ()->data;
+	      parameter->data_value_size = attr->block ()->size;
 	    }
 	}
     }
@@ -14057,7 +14057,7 @@ handle_data_member_location (struct die_info *die, struct dwarf2_cu *cu,
       else if (attr->form_is_section_offset ())
 	dwarf2_complex_location_expr_complaint ();
       else if (attr->form_is_block ())
-	*offset = decode_locdesc (DW_BLOCK (attr), cu);
+	*offset = decode_locdesc (attr->block (), cu);
       else
 	dwarf2_complex_location_expr_complaint ();
 
@@ -14632,19 +14632,19 @@ dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
   attr = dwarf2_attr (die, DW_AT_vtable_elem_location, cu);
   if (attr != nullptr)
     {
-      if (attr->form_is_block () && DW_BLOCK (attr)->size > 0)
+      if (attr->form_is_block () && attr->block ()->size > 0)
         {
-	  if (DW_BLOCK (attr)->data[0] == DW_OP_constu)
+	  if (attr->block ()->data[0] == DW_OP_constu)
 	    {
 	      /* Old-style GCC.  */
-	      fnp->voffset = decode_locdesc (DW_BLOCK (attr), cu) + 2;
+	      fnp->voffset = decode_locdesc (attr->block (), cu) + 2;
 	    }
-	  else if (DW_BLOCK (attr)->data[0] == DW_OP_deref
-		   || (DW_BLOCK (attr)->size > 1
-		       && DW_BLOCK (attr)->data[0] == DW_OP_deref_size
-		       && DW_BLOCK (attr)->data[1] == cu->header.addr_size))
+	  else if (attr->block ()->data[0] == DW_OP_deref
+		   || (attr->block ()->size > 1
+		       && attr->block ()->data[0] == DW_OP_deref_size
+		       && attr->block ()->data[1] == cu->header.addr_size))
 	    {
-	      fnp->voffset = decode_locdesc (DW_BLOCK (attr), cu);
+	      fnp->voffset = decode_locdesc (attr->block (), cu);
 	      if ((fnp->voffset % cu->header.addr_size) != 0)
 		dwarf2_complex_location_expr_complaint ();
 	      else
@@ -15858,7 +15858,7 @@ mark_common_block_symbol_computed (struct symbol *sym,
       baton->size += 1 /* DW_OP_addr */ + cu->header.addr_size;
     }
   else
-    baton->size += DW_BLOCK (member_loc)->size;
+    baton->size += member_loc->block ()->size;
 
   ptr = (gdb_byte *) obstack_alloc (&objfile->objfile_obstack, baton->size);
   baton->data = ptr;
@@ -15878,8 +15878,8 @@ mark_common_block_symbol_computed (struct symbol *sym,
     {
       /* We have to copy the data here, because DW_OP_call4 will only
 	 use a DW_AT_location attribute.  */
-      memcpy (ptr, DW_BLOCK (member_loc)->data, DW_BLOCK (member_loc)->size);
-      ptr += DW_BLOCK (member_loc)->size;
+      memcpy (ptr, member_loc->block ()->data, member_loc->block ()->size);
+      ptr += member_loc->block ()->size;
     }
 
   *ptr++ = DW_OP_plus;
@@ -16971,8 +16971,8 @@ attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
       baton = XOBNEW (obstack, struct dwarf2_property_baton);
       baton->property_type = default_type;
       baton->locexpr.per_cu = cu->per_cu;
-      baton->locexpr.size = DW_BLOCK (attr)->size;
-      baton->locexpr.data = DW_BLOCK (attr)->data;
+      baton->locexpr.size = attr->block ()->size;
+      baton->locexpr.data = attr->block ()->data;
       switch (attr->name)
 	{
 	case DW_AT_string_length:
@@ -17017,8 +17017,8 @@ attr_to_dynamic_prop (const struct attribute *attr, struct die_info *die,
 		baton = XOBNEW (obstack, struct dwarf2_property_baton);
 		baton->property_type = die_type (target_die, target_cu);
 		baton->locexpr.per_cu = cu->per_cu;
-		baton->locexpr.size = DW_BLOCK (target_attr)->size;
-		baton->locexpr.data = DW_BLOCK (target_attr)->data;
+		baton->locexpr.size = target_attr->block ()->size;
+		baton->locexpr.data = target_attr->block ()->data;
 		baton->locexpr.is_reference = true;
 		prop->data.baton = baton;
 		prop->kind = PROP_LOCEXPR;
@@ -17911,7 +17911,7 @@ partial_die_info::read (const struct die_reader_specs *reader,
           /* Support the .debug_loc offsets.  */
           if (attr.form_is_block ())
             {
-	       d.locdesc = DW_BLOCK (&attr);
+	      d.locdesc = attr.block ();
             }
           else if (attr.form_is_section_offset ())
             {
@@ -18378,7 +18378,7 @@ read_attribute_value (const struct die_reader_specs *reader,
       info_ptr += 2;
       blk->data = read_n_bytes (abfd, info_ptr, blk->size);
       info_ptr += blk->size;
-      DW_BLOCK (attr) = blk;
+      attr->set_block (blk);
       break;
     case DW_FORM_block4:
       blk = dwarf_alloc_block (cu);
@@ -18386,7 +18386,7 @@ read_attribute_value (const struct die_reader_specs *reader,
       info_ptr += 4;
       blk->data = read_n_bytes (abfd, info_ptr, blk->size);
       info_ptr += blk->size;
-      DW_BLOCK (attr) = blk;
+      attr->set_block (blk);
       break;
     case DW_FORM_data2:
       DW_UNSND (attr) = read_2_bytes (abfd, info_ptr);
@@ -18405,7 +18405,7 @@ read_attribute_value (const struct die_reader_specs *reader,
       blk->size = 16;
       blk->data = read_n_bytes (abfd, info_ptr, 16);
       info_ptr += 16;
-      DW_BLOCK (attr) = blk;
+      attr->set_block (blk);
       break;
     case DW_FORM_sec_offset:
       DW_UNSND (attr) = cu->header.read_offset (abfd, info_ptr, &bytes_read);
@@ -18452,7 +18452,7 @@ read_attribute_value (const struct die_reader_specs *reader,
       info_ptr += bytes_read;
       blk->data = read_n_bytes (abfd, info_ptr, blk->size);
       info_ptr += blk->size;
-      DW_BLOCK (attr) = blk;
+      attr->set_block (blk);
       break;
     case DW_FORM_block1:
       blk = dwarf_alloc_block (cu);
@@ -18460,7 +18460,7 @@ read_attribute_value (const struct die_reader_specs *reader,
       info_ptr += 1;
       blk->data = read_n_bytes (abfd, info_ptr, blk->size);
       info_ptr += blk->size;
-      DW_BLOCK (attr) = blk;
+      attr->set_block (blk);
       break;
     case DW_FORM_data1:
       DW_UNSND (attr) = read_1_byte (abfd, info_ptr);
@@ -19920,7 +19920,7 @@ var_decode_location (struct attribute *attr, struct symbol *sym,
 
   /* A DW_AT_location attribute with no contents indicates that a
      variable has been optimized away.  */
-  if (attr->form_is_block () && DW_BLOCK (attr)->size == 0)
+  if (attr->form_is_block () && attr->block ()->size == 0)
     {
       SYMBOL_ACLASS_INDEX (sym) = LOC_OPTIMIZED_OUT;
       return;
@@ -19932,23 +19932,23 @@ var_decode_location (struct attribute *attr, struct symbol *sym,
      DW_OP_GNU_addr_index then mark this symbol as LOC_STATIC.  */
 
   if (attr->form_is_block ()
-      && ((DW_BLOCK (attr)->data[0] == DW_OP_addr
-	   && DW_BLOCK (attr)->size == 1 + cu_header->addr_size)
-	  || ((DW_BLOCK (attr)->data[0] == DW_OP_GNU_addr_index
-               || DW_BLOCK (attr)->data[0] == DW_OP_addrx)
-	      && (DW_BLOCK (attr)->size
-		  == 1 + leb128_size (&DW_BLOCK (attr)->data[1])))))
+      && ((attr->block ()->data[0] == DW_OP_addr
+	   && attr->block ()->size == 1 + cu_header->addr_size)
+	  || ((attr->block ()->data[0] == DW_OP_GNU_addr_index
+               || attr->block ()->data[0] == DW_OP_addrx)
+	      && (attr->block ()->size
+		  == 1 + leb128_size (&attr->block ()->data[1])))))
     {
       unsigned int dummy;
 
-      if (DW_BLOCK (attr)->data[0] == DW_OP_addr)
+      if (attr->block ()->data[0] == DW_OP_addr)
 	SET_SYMBOL_VALUE_ADDRESS
 	  (sym, cu->header.read_address (objfile->obfd,
-					 DW_BLOCK (attr)->data + 1,
+					 attr->block ()->data + 1,
 					 &dummy));
       else
 	SET_SYMBOL_VALUE_ADDRESS
-	  (sym, read_addr_index_from_leb128 (cu, DW_BLOCK (attr)->data + 1,
+	  (sym, read_addr_index_from_leb128 (cu, attr->block ()->data + 1,
 					     &dummy));
       SYMBOL_ACLASS_INDEX (sym) = LOC_STATIC;
       fixup_symbol_section (sym, objfile);
@@ -20481,7 +20481,7 @@ dwarf2_const_value_attr (const struct attribute *attr, struct type *type,
     case DW_FORM_block:
     case DW_FORM_exprloc:
     case DW_FORM_data16:
-      blk = DW_BLOCK (attr);
+      blk = attr->block ();
       if (TYPE_LENGTH (type) != blk->size)
 	dwarf2_const_value_length_mismatch_complaint (name, blk->size,
 						      TYPE_LENGTH (type));
@@ -21343,11 +21343,11 @@ dump_die_shallow (struct ui_file *f, int indent, struct die_info *die)
 	case DW_FORM_block:
 	case DW_FORM_block1:
 	  fprintf_unfiltered (f, "block: size %s",
-			      pulongest (DW_BLOCK (&die->attrs[i])->size));
+			      pulongest (die->attrs[i].block ()->size));
 	  break;
 	case DW_FORM_exprloc:
 	  fprintf_unfiltered (f, "expression: size %s",
-			      pulongest (DW_BLOCK (&die->attrs[i])->size));
+			      pulongest (die->attrs[i].block ()->size));
 	  break;
 	case DW_FORM_data16:
 	  fprintf_unfiltered (f, "constant of 16 bytes");
@@ -21685,8 +21685,8 @@ dwarf2_fetch_die_loc_sect_off (sect_offset sect_off,
 		 "is neither DW_FORM_block* nor DW_FORM_exprloc"),
 	       sect_offset_str (sect_off), objfile_name (objfile));
 
-      retval.data = DW_BLOCK (attr)->data;
-      retval.size = DW_BLOCK (attr)->size;
+      retval.data = attr->block ()->data;
+      retval.size = attr->block ()->size;
     }
   retval.per_cu = cu->per_cu;
 
@@ -21797,8 +21797,8 @@ dwarf2_fetch_constant_bytes (sect_offset sect_off,
     case DW_FORM_block:
     case DW_FORM_exprloc:
     case DW_FORM_data16:
-      result = DW_BLOCK (attr)->data;
-      *len = DW_BLOCK (attr)->size;
+      result = attr->block ()->data;
+      *len = attr->block ()->size;
       break;
 
       /* The DW_AT_const_value attributes are supposed to carry the
@@ -22578,8 +22578,8 @@ dwarf2_symbol_mark_computed (const struct attribute *attr, struct symbol *sym,
 	     info_buffer for SYM's objfile; right now we never release
 	     that buffer, but when we do clean up properly this may
 	     need to change.  */
-	  baton->size = DW_BLOCK (attr)->size;
-	  baton->data = DW_BLOCK (attr)->data;
+	  baton->size = attr->block ()->size;
+	  baton->data = attr->block ()->data;
 	}
       else
 	{
-- 
2.17.2

Re: [PATCH 07/20] Remove DW_BLOCK

[Simon Marchi]

On 2020-03-28 3:21 p.m., Tom Tromey wrote:
> @@ -22578,8 +22578,8 @@ dwarf2_symbol_mark_computed (const struct attribute *attr, struct symbol *sym,
>  	     info_buffer for SYM's objfile; right now we never release
>  	     that buffer, but when we do clean up properly this may
>  	     need to change.  */
> -	  baton->size = DW_BLOCK (attr)->size;
> -	  baton->data = DW_BLOCK (attr)->data;
> +	  baton->size = attr->block ()->size;
> +	  baton->data = attr->block ()->data;

Again, in these cases, it would probably be worth using a local variable to store
the result of attr->block (), to avoid doing the check twice.

Simon

[PATCH 08/20] Remove DW_SIGNATURE

[Tom Tromey]

This removes DW_SIGNATURE in favor of methods on struct attribute.  As
usual, the methods check the form, which DW_SIGNATURE did not do.

2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (read_attribute_value, lookup_die_type)
	(dump_die_shallow, follow_die_sig, get_DW_AT_signature_type):
	Update.
	* dwarf2/attribute.h (struct attribute) <signature,
	set_signature>: New methods.
	(DW_SIGNATURE): Remove.
---
 gdb/ChangeLog          |  9 +++++++++
 gdb/dwarf2/attribute.h | 16 +++++++++++++++-
 gdb/dwarf2/read.c      | 10 +++++-----
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index f59986f78a3..8a7aab84eb2 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -57,6 +57,14 @@ struct attribute
     return u.blk;
   }
 
+  /* Return the signature.  The attribute must have signature
+     form.  */
+  ULONGEST signature () const
+  {
+    gdb_assert (form == DW_FORM_ref_sig8);
+    return u.signature;
+  }
+
   /* Return non-zero if ATTR's value is a section offset --- classes
      lineptr, loclistptr, macptr or rangelistptr --- or zero, otherwise.
      You may use DW_UNSND (attr) to retrieve such offsets.
@@ -139,6 +147,13 @@ struct attribute
     u.blk = blk;
   }
 
+  /* Set the signature value for this attribute.  */
+  void set_signature (ULONGEST signature)
+  {
+    gdb_assert (form == DW_FORM_ref_sig8);
+    u.signature = signature;
+  }
+
 
   ENUM_BITFIELD(dwarf_attribute) name : 16;
   ENUM_BITFIELD(dwarf_form) form : 15;
@@ -165,6 +180,5 @@ struct attribute
 #define DW_UNSND(attr)     ((attr)->u.unsnd)
 #define DW_SND(attr)       ((attr)->u.snd)
 #define DW_ADDR(attr)	   ((attr)->u.addr)
-#define DW_SIGNATURE(attr) ((attr)->u.signature)
 
 #endif /* GDB_DWARF2_ATTRIBUTE_H */
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 87bce7e51a9..cecdc41a0eb 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -18503,7 +18503,7 @@ read_attribute_value (const struct die_reader_specs *reader,
       info_ptr += 8;
       break;
     case DW_FORM_ref_sig8:
-      DW_SIGNATURE (attr) = read_8_bytes (abfd, info_ptr);
+      attr->set_signature (read_8_bytes (abfd, info_ptr));
       info_ptr += 8;
       break;
     case DW_FORM_ref_udata:
@@ -20693,7 +20693,7 @@ lookup_die_type (struct die_info *die, const struct attribute *attr,
     }
   else if (attr->form == DW_FORM_ref_sig8)
     {
-      ULONGEST signature = DW_SIGNATURE (attr);
+      ULONGEST signature = attr->signature ();
 
       return get_signatured_type (die, signature, cu);
     }
@@ -21383,7 +21383,7 @@ dump_die_shallow (struct ui_file *f, int indent, struct die_info *die)
 	  break;
 	case DW_FORM_ref_sig8:
 	  fprintf_unfiltered (f, "signature: %s",
-			      hex_string (DW_SIGNATURE (&die->attrs[i])));
+			      hex_string (die->attrs[i].signature ()));
 	  break;
 	case DW_FORM_string:
 	case DW_FORM_strp:
@@ -21950,7 +21950,7 @@ static struct die_info *
 follow_die_sig (struct die_info *src_die, const struct attribute *attr,
 		struct dwarf2_cu **ref_cu)
 {
-  ULONGEST signature = DW_SIGNATURE (attr);
+  ULONGEST signature = attr->signature ();
   struct signatured_type *sig_type;
   struct die_info *die;
 
@@ -22057,7 +22057,7 @@ get_DW_AT_signature_type (struct die_info *die, const struct attribute *attr,
     }
   else if (attr->form == DW_FORM_ref_sig8)
     {
-      return get_signatured_type (die, DW_SIGNATURE (attr), cu);
+      return get_signatured_type (die, attr->signature (), cu);
     }
   else
     {
-- 
2.17.2

[PATCH 09/20] Remove DW_SND

[Tom Tromey]

This removes DW_SND in favor of accessors on struct attribute.
These accessors check that the form is appropriate.

gdb/ChangeLog
2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (get_alignment, read_array_order)
	(read_attribute_value, dwarf2_const_value_attr)
	(dump_die_shallow, dwarf2_fetch_constant_bytes): Update.
	* dwarf2/attribute.h (struct attribute) <get_signed, set_signed>:
	New methods.
	(DW_SND): Remove.
---
 gdb/ChangeLog          |  9 +++++++++
 gdb/dwarf2/attribute.h | 16 +++++++++++++++-
 gdb/dwarf2/read.c      | 20 ++++++++++++--------
 3 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index 8a7aab84eb2..a08c5a13e44 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -65,6 +65,14 @@ struct attribute
     return u.signature;
   }
 
+  /* Return the signed value.  The attribute must have the appropriate
+     form.  */
+  LONGEST get_signed () const
+  {
+    gdb_assert (form == DW_FORM_sdata || form == DW_FORM_implicit_const);
+    return u.snd;
+  }
+
   /* Return non-zero if ATTR's value is a section offset --- classes
      lineptr, loclistptr, macptr or rangelistptr --- or zero, otherwise.
      You may use DW_UNSND (attr) to retrieve such offsets.
@@ -154,6 +162,13 @@ struct attribute
     u.signature = signature;
   }
 
+  /* Set this attribute to a signed integer.  */
+  void set_signed (LONGEST snd)
+  {
+    gdb_assert (form == DW_FORM_sdata || form == DW_FORM_implicit_const);
+    u.snd = snd;
+  }
+
 
   ENUM_BITFIELD(dwarf_attribute) name : 16;
   ENUM_BITFIELD(dwarf_form) form : 15;
@@ -178,7 +193,6 @@ struct attribute
 /* Get at parts of an attribute structure.  */
 
 #define DW_UNSND(attr)     ((attr)->u.unsnd)
-#define DW_SND(attr)       ((attr)->u.snd)
 #define DW_ADDR(attr)	   ((attr)->u.addr)
 
 #endif /* GDB_DWARF2_ATTRIBUTE_H */
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index cecdc41a0eb..6eecf051c84 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -14812,7 +14812,7 @@ get_alignment (struct dwarf2_cu *cu, struct die_info *die)
   ULONGEST align;
   if (attr->form == DW_FORM_sdata)
     {
-      LONGEST val = DW_SND (attr);
+      LONGEST val = attr->get_signed ();
       if (val < 0)
 	{
 	  complaint (_("DW_AT_alignment value must not be negative"
@@ -15764,7 +15764,11 @@ read_array_order (struct die_info *die, struct dwarf2_cu *cu)
   attr = dwarf2_attr (die, DW_AT_ordering, cu);
 
   if (attr != nullptr)
-    return (enum dwarf_array_dim_ordering) DW_SND (attr);
+    {
+      LONGEST val = attr->constant_value (-1);
+      if (val == DW_ORD_row_major || val == DW_ORD_col_major)
+	return (enum dwarf_array_dim_ordering) val;
+    }
 
   /* GNU F77 is a special case, as at 08/2004 array type info is the
      opposite order to the dwarf2 specification, but data is still
@@ -18474,7 +18478,7 @@ read_attribute_value (const struct die_reader_specs *reader,
       DW_UNSND (attr) = 1;
       break;
     case DW_FORM_sdata:
-      DW_SND (attr) = read_signed_leb128 (abfd, info_ptr, &bytes_read);
+      attr->set_signed (read_signed_leb128 (abfd, info_ptr, &bytes_read));
       info_ptr += bytes_read;
       break;
     case DW_FORM_udata:
@@ -18523,7 +18527,7 @@ read_attribute_value (const struct die_reader_specs *reader,
 				       info_ptr, need_reprocess);
       break;
     case DW_FORM_implicit_const:
-      DW_SND (attr) = implicit_const;
+      attr->set_signed (implicit_const);
       break;
     case DW_FORM_addrx:
     case DW_FORM_GNU_addr_index:
@@ -20508,7 +20512,7 @@ dwarf2_const_value_attr (const struct attribute *attr, struct type *type,
 
     case DW_FORM_sdata:
     case DW_FORM_implicit_const:
-      *value = DW_SND (attr);
+      *value = attr->get_signed ();
       break;
 
     case DW_FORM_udata:
@@ -21373,7 +21377,6 @@ dump_die_shallow (struct ui_file *f, int indent, struct die_info *die)
 	case DW_FORM_data4:
 	case DW_FORM_data8:
 	case DW_FORM_udata:
-	case DW_FORM_sdata:
 	  fprintf_unfiltered (f, "constant: %s",
 			      pulongest (DW_UNSND (&die->attrs[i])));
 	  break;
@@ -21411,9 +21414,10 @@ dump_die_shallow (struct ui_file *f, int indent, struct die_info *die)
 	  fprintf_unfiltered (f,
 			      "unexpected attribute form: DW_FORM_indirect");
 	  break;
+	case DW_FORM_sdata:
 	case DW_FORM_implicit_const:
 	  fprintf_unfiltered (f, "constant: %s",
-			      plongest (DW_SND (&die->attrs[i])));
+			      plongest (die->attrs[i].get_signed ()));
 	  break;
 	default:
 	  fprintf_unfiltered (f, "unsupported attribute form: %d.",
@@ -21839,7 +21843,7 @@ dwarf2_fetch_constant_bytes (sect_offset sect_off,
     case DW_FORM_implicit_const:
       type = die_type (die, cu);
       result = write_constant_as_bytes (obstack, byte_order,
-					type, DW_SND (attr), len);
+					type, attr->get_signed (), len);
       break;
 
     case DW_FORM_udata:
-- 
2.17.2

[PATCH 10/20] Use setter for attribute's unsigned value

[Tom Tromey]

This adds form_is_unsigned and an unsigned setter method to struct
attribute, and updates the remaining code.  Now DW_UNSND is no longer
used as an lvalue.

gdb/ChangeLog
2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (read_attribute_value): Update.
	* dwarf2/attribute.h (struct attribute) <form_is_unsigned,
	set_unsigned>: New methods.
	* dwarf2/attribute.c (attribute::form_is_unsigned): New method.
---
 gdb/ChangeLog          |  7 ++++++
 gdb/dwarf2/attribute.c | 23 +++++++++++++++++++
 gdb/dwarf2/attribute.h | 10 ++++++++
 gdb/dwarf2/read.c      | 52 +++++++++++++++++++++---------------------
 4 files changed, 66 insertions(+), 26 deletions(-)

diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
index fdf202033e9..72ec13c11f9 100644
--- a/gdb/dwarf2/attribute.c
+++ b/gdb/dwarf2/attribute.c
@@ -183,3 +183,26 @@ attribute::constant_value (int default_value) const
       return default_value;
     }
 }
+
+/* See attribute.h.  */
+
+bool
+attribute::form_is_unsigned () const
+{
+  return (form == DW_FORM_ref_addr
+	  || form == DW_FORM_GNU_ref_alt
+	  || form == DW_FORM_data2
+	  || form == DW_FORM_data4
+	  || form == DW_FORM_data8
+	  || form == DW_FORM_sec_offset
+	  || form == DW_FORM_data1
+	  || form == DW_FORM_flag
+	  || form == DW_FORM_flag_present
+	  || form == DW_FORM_udata
+	  || form == DW_FORM_rnglistx
+	  || form == DW_FORM_ref1
+	  || form == DW_FORM_ref2
+	  || form == DW_FORM_ref4
+	  || form == DW_FORM_ref8
+	  || form == DW_FORM_ref_udata);
+}
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index a08c5a13e44..0a4c8647f6e 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -115,6 +115,9 @@ struct attribute
   /* Check if the attribute's form is a string form.  */
   bool form_is_string () const;
 
+  /* Check if the attribute's form is an unsigned integer form.  */
+  bool form_is_unsigned () const;
+
   /* Return DIE offset of this attribute.  Return 0 with complaint if
      the attribute is not of the required kind.  */
 
@@ -169,6 +172,13 @@ struct attribute
     u.snd = snd;
   }
 
+  /* Set this attribute to an unsigned integer.  */
+  void set_unsigned (ULONGEST unsnd)
+  {
+    gdb_assert (form_is_unsigned ());
+    u.unsnd = unsnd;
+  }
+
 
   ENUM_BITFIELD(dwarf_attribute) name : 16;
   ENUM_BITFIELD(dwarf_form) form : 15;
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 6eecf051c84..75ac56efc02 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -18360,15 +18360,16 @@ read_attribute_value (const struct die_reader_specs *reader,
     {
     case DW_FORM_ref_addr:
       if (cu->header.version == 2)
-	DW_UNSND (attr) = cu->header.read_address (abfd, info_ptr,
-						   &bytes_read);
+	attr->set_unsigned (cu->header.read_address (abfd, info_ptr,
+						     &bytes_read));
       else
-	DW_UNSND (attr) = cu->header.read_offset (abfd, info_ptr,
-						  &bytes_read);
+	attr->set_unsigned (cu->header.read_offset (abfd, info_ptr,
+						    &bytes_read));
       info_ptr += bytes_read;
       break;
     case DW_FORM_GNU_ref_alt:
-      DW_UNSND (attr) = cu->header.read_offset (abfd, info_ptr, &bytes_read);
+      attr->set_unsigned (cu->header.read_offset (abfd, info_ptr,
+						  &bytes_read));
       info_ptr += bytes_read;
       break;
     case DW_FORM_addr:
@@ -18393,15 +18394,15 @@ read_attribute_value (const struct die_reader_specs *reader,
       attr->set_block (blk);
       break;
     case DW_FORM_data2:
-      DW_UNSND (attr) = read_2_bytes (abfd, info_ptr);
+      attr->set_unsigned (read_2_bytes (abfd, info_ptr));
       info_ptr += 2;
       break;
     case DW_FORM_data4:
-      DW_UNSND (attr) = read_4_bytes (abfd, info_ptr);
+      attr->set_unsigned (read_4_bytes (abfd, info_ptr));
       info_ptr += 4;
       break;
     case DW_FORM_data8:
-      DW_UNSND (attr) = read_8_bytes (abfd, info_ptr);
+      attr->set_unsigned (read_8_bytes (abfd, info_ptr));
       info_ptr += 8;
       break;
     case DW_FORM_data16:
@@ -18412,7 +18413,8 @@ read_attribute_value (const struct die_reader_specs *reader,
       attr->set_block (blk);
       break;
     case DW_FORM_sec_offset:
-      DW_UNSND (attr) = cu->header.read_offset (abfd, info_ptr, &bytes_read);
+      attr->set_unsigned (cu->header.read_offset (abfd, info_ptr,
+						  &bytes_read));
       info_ptr += bytes_read;
       break;
     case DW_FORM_string:
@@ -18467,15 +18469,12 @@ read_attribute_value (const struct die_reader_specs *reader,
       attr->set_block (blk);
       break;
     case DW_FORM_data1:
-      DW_UNSND (attr) = read_1_byte (abfd, info_ptr);
-      info_ptr += 1;
-      break;
     case DW_FORM_flag:
-      DW_UNSND (attr) = read_1_byte (abfd, info_ptr);
+      attr->set_unsigned (read_1_byte (abfd, info_ptr));
       info_ptr += 1;
       break;
     case DW_FORM_flag_present:
-      DW_UNSND (attr) = 1;
+      attr->set_unsigned (1);
       break;
     case DW_FORM_sdata:
       attr->set_signed (read_signed_leb128 (abfd, info_ptr, &bytes_read));
@@ -18483,27 +18482,27 @@ read_attribute_value (const struct die_reader_specs *reader,
       break;
     case DW_FORM_udata:
     case DW_FORM_rnglistx:
-      DW_UNSND (attr) = read_unsigned_leb128 (abfd, info_ptr, &bytes_read);
+      attr->set_unsigned (read_unsigned_leb128 (abfd, info_ptr, &bytes_read));
       info_ptr += bytes_read;
       break;
     case DW_FORM_ref1:
-      DW_UNSND (attr) = (to_underlying (cu->header.sect_off)
-			 + read_1_byte (abfd, info_ptr));
+      attr->set_unsigned ((to_underlying (cu->header.sect_off)
+			   + read_1_byte (abfd, info_ptr)));
       info_ptr += 1;
       break;
     case DW_FORM_ref2:
-      DW_UNSND (attr) = (to_underlying (cu->header.sect_off)
-			 + read_2_bytes (abfd, info_ptr));
+      attr->set_unsigned ((to_underlying (cu->header.sect_off)
+			   + read_2_bytes (abfd, info_ptr)));
       info_ptr += 2;
       break;
     case DW_FORM_ref4:
-      DW_UNSND (attr) = (to_underlying (cu->header.sect_off)
-			 + read_4_bytes (abfd, info_ptr));
+      attr->set_unsigned ((to_underlying (cu->header.sect_off)
+			   + read_4_bytes (abfd, info_ptr)));
       info_ptr += 4;
       break;
     case DW_FORM_ref8:
-      DW_UNSND (attr) = (to_underlying (cu->header.sect_off)
-			 + read_8_bytes (abfd, info_ptr));
+      attr->set_unsigned ((to_underlying (cu->header.sect_off)
+			   + read_8_bytes (abfd, info_ptr)));
       info_ptr += 8;
       break;
     case DW_FORM_ref_sig8:
@@ -18511,8 +18510,9 @@ read_attribute_value (const struct die_reader_specs *reader,
       info_ptr += 8;
       break;
     case DW_FORM_ref_udata:
-      DW_UNSND (attr) = (to_underlying (cu->header.sect_off)
-			 + read_unsigned_leb128 (abfd, info_ptr, &bytes_read));
+      attr->set_unsigned ((to_underlying (cu->header.sect_off)
+			   + read_unsigned_leb128 (abfd, info_ptr,
+						   &bytes_read)));
       info_ptr += bytes_read;
       break;
     case DW_FORM_indirect:
@@ -18595,7 +18595,7 @@ read_attribute_value (const struct die_reader_specs *reader,
       complaint
         (_("Suspicious DW_AT_byte_size value treated as zero instead of %s"),
          hex_string (DW_UNSND (attr)));
-      DW_UNSND (attr) = 0;
+      attr->set_unsigned (0);
     }
 
   return info_ptr;
-- 
2.17.2

[PATCH 11/20] Add reprocessing flag to struct attribute

[Tom Tromey]

Some forms require "reprocessing" -- a second pass to update their
value appropriately.  In this case, we'll set the unsigned value on
the attribute, and then later set it to the correct value.

To handle this, we introduce a reprocessing flag to attribute.  Then,
we manage this flag to ensure that setting and unsetting is done
properly.

gdb/ChangeLog
2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (read_cutu_die_from_dwo): Use OBSTACK_ZALLOC.
	(read_attribute_reprocess, read_attribute_value): Update.
	(read_attribute): Clear requires_reprocessing.
	* dwarf2/attribute.h (struct attribute) <get_unsigned_reprocess,
	form_is_reprocessed>: New methods.
	<string_init>: Clear requires_reprocessing.
	<set_unsigned_reprocess>: New method.
	<name>: Shrink by one bit.
	<requires_reprocessing>: New member.
	* dwarf2/attribute.c (attribute::form_is_reprocessed): New
	method.
---
 gdb/ChangeLog          | 14 ++++++++++++++
 gdb/dwarf2/attribute.c | 14 ++++++++++++++
 gdb/dwarf2/attribute.h | 30 +++++++++++++++++++++++++++++-
 gdb/dwarf2/read.c      | 12 +++++++-----
 4 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
index 72ec13c11f9..73c1ef9f792 100644
--- a/gdb/dwarf2/attribute.c
+++ b/gdb/dwarf2/attribute.c
@@ -206,3 +206,17 @@ attribute::form_is_unsigned () const
 	  || form == DW_FORM_ref8
 	  || form == DW_FORM_ref_udata);
 }
+
+/* See attribute.h.  */
+
+bool
+attribute::form_is_reprocessed () const
+{
+  return (form == DW_FORM_strx1
+	  || form == DW_FORM_strx2
+	  || form == DW_FORM_strx3
+	  || form == DW_FORM_strx4
+	  || form == DW_FORM_GNU_str_index
+	  || form == DW_FORM_addrx
+	  || form == DW_FORM_GNU_addr_index);
+}
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index 0a4c8647f6e..b96fdac5201 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -73,6 +73,15 @@ struct attribute
     return u.snd;
   }
 
+  /* Return the unsigned value, but only for attributes requiring
+     reprocessing.  */
+  ULONGEST get_unsigned_reprocess () const
+  {
+    gdb_assert (form_is_reprocessed ());
+    gdb_assert (requires_reprocessing);
+    return u.unsnd;
+  }
+
   /* Return non-zero if ATTR's value is a section offset --- classes
      lineptr, loclistptr, macptr or rangelistptr --- or zero, otherwise.
      You may use DW_UNSND (attr) to retrieve such offsets.
@@ -118,6 +127,10 @@ struct attribute
   /* Check if the attribute's form is an unsigned integer form.  */
   bool form_is_unsigned () const;
 
+  /* Check if the attribute's form is a form that requires
+     "reprocessing".  */
+  bool form_is_reprocessed () const;
+
   /* Return DIE offset of this attribute.  Return 0 with complaint if
      the attribute is not of the required kind.  */
 
@@ -141,6 +154,7 @@ struct attribute
     gdb_assert (form_is_string ());
     u.str = str;
     string_is_canonical = 0;
+    requires_reprocessing = 0;
   }
 
   /* Set the canonical string value for this attribute.  */
@@ -179,8 +193,22 @@ struct attribute
     u.unsnd = unsnd;
   }
 
+  /* Temporarily this attribute to an unsigned integer.  This is used
+     only for those forms that require reprocessing.  */
+  void set_unsigned_reprocess (ULONGEST unsnd)
+  {
+    gdb_assert (form_is_reprocessed ());
+    u.unsnd = unsnd;
+    requires_reprocessing = 1;
+  }
+
+
+  ENUM_BITFIELD(dwarf_attribute) name : 15;
+
+  /* If this requires reprocessing, is it in its final form, or is it
+     still stored as an unsigned?  */
+  unsigned int requires_reprocessing : 1;
 
-  ENUM_BITFIELD(dwarf_attribute) name : 16;
   ENUM_BITFIELD(dwarf_form) form : 15;
 
   /* Has u.str already been updated by dwarf2_canonicalize_name?  This
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 75ac56efc02..a5c2c52375d 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6471,7 +6471,7 @@ read_cutu_die_from_dwo (struct dwarf2_per_cu_data *this_cu,
   else if (stub_comp_dir != NULL)
     {
       /* Reconstruct the comp_dir attribute to simplify the code below.  */
-      comp_dir = XOBNEW (&cu->comp_unit_obstack, struct attribute);
+      comp_dir = OBSTACK_ZALLOC (&cu->comp_unit_obstack, struct attribute);
       comp_dir->name = DW_AT_comp_dir;
       comp_dir->form = DW_FORM_string;
       comp_dir->string_init (stub_comp_dir);
@@ -18314,7 +18314,7 @@ read_attribute_reprocess (const struct die_reader_specs *reader,
     {
       case DW_FORM_addrx:
       case DW_FORM_GNU_addr_index:
-        DW_ADDR (attr) = read_addr_index (cu, DW_UNSND (attr));
+        DW_ADDR (attr) = read_addr_index (cu, attr->get_unsigned_reprocess ());
         break;
       case DW_FORM_strx:
       case DW_FORM_strx1:
@@ -18323,7 +18323,7 @@ read_attribute_reprocess (const struct die_reader_specs *reader,
       case DW_FORM_strx4:
       case DW_FORM_GNU_str_index:
 	{
-	  unsigned int str_index = DW_UNSND (attr);
+	  unsigned int str_index = attr->get_unsigned_reprocess ();
 	  gdb_assert (!attr->canonical_p ());
 	  if (reader->dwo_file != NULL)
 	    attr->string_init (read_dwo_str_index (reader, str_index));
@@ -18532,7 +18532,8 @@ read_attribute_value (const struct die_reader_specs *reader,
     case DW_FORM_addrx:
     case DW_FORM_GNU_addr_index:
       *need_reprocess = true;
-      DW_UNSND (attr) = read_unsigned_leb128 (abfd, info_ptr, &bytes_read);
+      attr->set_unsigned_reprocess (read_unsigned_leb128 (abfd, info_ptr,
+							  &bytes_read));
       info_ptr += bytes_read;
       break;
     case DW_FORM_strx:
@@ -18569,7 +18570,7 @@ read_attribute_value (const struct die_reader_specs *reader,
 	    info_ptr += bytes_read;
 	  }
 	*need_reprocess = true;
-	 DW_UNSND (attr) = str_index;
+	attr->set_unsigned_reprocess (str_index);
 	}
       break;
     default:
@@ -18610,6 +18611,7 @@ read_attribute (const struct die_reader_specs *reader,
 {
   attr->name = abbrev->name;
   attr->string_is_canonical = 0;
+  attr->requires_reprocessing = 0;
   return read_attribute_value (reader, attr, abbrev->form,
 			       abbrev->implicit_const, info_ptr,
 			       need_reprocess);
-- 
2.17.2

Re: [PATCH 11/20] Add reprocessing flag to struct attribute

[Simon Marchi]

On 2020-03-28 3:21 p.m., Tom Tromey wrote:
> diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
> index 72ec13c11f9..73c1ef9f792 100644
> --- a/gdb/dwarf2/attribute.c
> +++ b/gdb/dwarf2/attribute.c
> @@ -206,3 +206,17 @@ attribute::form_is_unsigned () const
>  	  || form == DW_FORM_ref8
>  	  || form == DW_FORM_ref_udata);
>  }
> +
> +/* See attribute.h.  */
> +
> +bool
> +attribute::form_is_reprocessed () const

The name is odd, the "form" isn't reprocessed.  What would you think of
"form_requires_processing"?  The "requires_reprocessing" field implicitly
means "attribute_requires_reprocessing".  But if we want to avoid confusion
between the two, the field could be renamed "reprocessing_done", and its
logic inverted.

> @@ -179,8 +193,22 @@ struct attribute
>      u.unsnd = unsnd;
>    }
>  
> +  /* Temporarily this attribute to an unsigned integer.  This is used

Missing a word here?

> +     only for those forms that require reprocessing.  */
> +  void set_unsigned_reprocess (ULONGEST unsnd)
> +  {
> +    gdb_assert (form_is_reprocessed ());
> +    u.unsnd = unsnd;
> +    requires_reprocessing = 1;
> +  }
> +
> +
> +  ENUM_BITFIELD(dwarf_attribute) name : 15;
> +
> +  /* If this requires reprocessing, is it in its final form, or is it
> +     still stored as an unsigned?  */
> +  unsigned int requires_reprocessing : 1;

It would be good to explain what we mean by "reprocessing", here would be a good
place.  It would be good to give the example of the DW_FORM_strx form, where we
could encounter this form before having seen the corresponding
DW_AT_str_offsets_base attribute.  So we first store the offset as an unsigned
integer in the attribute, then "reprocess" it later to fetch the actual string.

Simon

Re: [PATCH 11/20] Add reprocessing flag to struct attribute

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> The name is odd, the "form" isn't reprocessed.  What would you think of
Simon> "form_requires_processing"?

Seems good, I made this change and the others.

Simon> The "requires_reprocessing" field implicitly
Simon> means "attribute_requires_reprocessing".  But if we want to avoid confusion
Simon> between the two, the field could be renamed "reprocessing_done", and its
Simon> logic inverted.

I think it is fine as-is, especially with the expanded comment.

Tom

[PATCH 12/20] Remove DW_ADDR

[Tom Tromey]

This removes DW_ADDR in favor of accessor methods.

gdb/ChangeLog
2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (read_attribute_reprocess, read_attribute_value)
	(dwarf2_const_value_attr, dump_die_shallow)
	(dwarf2_fetch_constant_bytes): Update.
	* dwarf2/attribute.h (struct attribute) <form_is_ref>: Update
	comment.
	<set_address>: New method.
	(DW_ADDR): Remove.
	* dwarf2/attribute.c (attribute::form_is_ref): Update comment.
---
 gdb/ChangeLog          | 11 +++++++++++
 gdb/dwarf2/attribute.c |  3 +--
 gdb/dwarf2/attribute.h | 17 ++++++++++++++---
 gdb/dwarf2/read.c      | 18 +++++++++++-------
 4 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
index 73c1ef9f792..94bedf6dbd3 100644
--- a/gdb/dwarf2/attribute.c
+++ b/gdb/dwarf2/attribute.c
@@ -128,8 +128,7 @@ attribute::form_is_constant () const
     }
 }
 
-/* DW_ADDR is always stored already as sect_offset; despite for the forms
-   besides DW_FORM_ref_addr it is stored as cu_offset in the DWARF file.  */
+/* See attribute.h.  */
 
 bool
 attribute::form_is_ref () const
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index b96fdac5201..972beef9825 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -111,8 +111,9 @@ struct attribute
 
   bool form_is_constant () const;
 
-  /* DW_ADDR is always stored already as sect_offset; despite for the forms
-     besides DW_FORM_ref_addr it is stored as cu_offset in the DWARF file.  */
+  /* The address is always stored already as sect_offset; despite for
+     the forms besides DW_FORM_ref_addr it is stored as cu_offset in
+     the DWARF file.  */
 
   bool form_is_ref () const;
 
@@ -202,6 +203,17 @@ struct attribute
     requires_reprocessing = 1;
   }
 
+  /* Set this attribute to an address.  */
+  void set_address (CORE_ADDR addr)
+  {
+    gdb_assert (form == DW_FORM_addr
+		|| ((form == DW_FORM_addrx
+		     || form == DW_FORM_GNU_addr_index)
+		    && requires_reprocessing));
+    u.addr = addr;
+    requires_reprocessing = 0;
+  }
+
 
   ENUM_BITFIELD(dwarf_attribute) name : 15;
 
@@ -231,6 +243,5 @@ struct attribute
 /* Get at parts of an attribute structure.  */
 
 #define DW_UNSND(attr)     ((attr)->u.unsnd)
-#define DW_ADDR(attr)	   ((attr)->u.addr)
 
 #endif /* GDB_DWARF2_ATTRIBUTE_H */
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index a5c2c52375d..b010c9c3b01 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -18314,7 +18314,8 @@ read_attribute_reprocess (const struct die_reader_specs *reader,
     {
       case DW_FORM_addrx:
       case DW_FORM_GNU_addr_index:
-        DW_ADDR (attr) = read_addr_index (cu, attr->get_unsigned_reprocess ());
+	attr->set_address (read_addr_index (cu,
+					    attr->get_unsigned_reprocess ()));
         break;
       case DW_FORM_strx:
       case DW_FORM_strx1:
@@ -18373,9 +18374,12 @@ read_attribute_value (const struct die_reader_specs *reader,
       info_ptr += bytes_read;
       break;
     case DW_FORM_addr:
-      DW_ADDR (attr) = cu->header.read_address (abfd, info_ptr, &bytes_read);
-      DW_ADDR (attr) = gdbarch_adjust_dwarf2_addr (gdbarch, DW_ADDR (attr));
-      info_ptr += bytes_read;
+      {
+	CORE_ADDR addr = cu->header.read_address (abfd, info_ptr, &bytes_read);
+	addr = gdbarch_adjust_dwarf2_addr (gdbarch, addr);
+	attr->set_address (addr);
+	info_ptr += bytes_read;
+      }
       break;
     case DW_FORM_block2:
       blk = dwarf_alloc_block (cu);
@@ -20468,7 +20472,7 @@ dwarf2_const_value_attr (const struct attribute *attr, struct type *type,
 
 	data[0] = DW_OP_addr;
 	store_unsigned_integer (&data[1], cu_header->addr_size,
-				byte_order, DW_ADDR (attr));
+				byte_order, attr->address ());
 	data[cu_header->addr_size + 1] = DW_OP_stack_value;
       }
       break;
@@ -21342,7 +21346,7 @@ dump_die_shallow (struct ui_file *f, int indent, struct die_info *die)
 	case DW_FORM_addrx:
 	case DW_FORM_GNU_addr_index:
 	  fprintf_unfiltered (f, "address: ");
-	  fputs_filtered (hex_string (DW_ADDR (&die->attrs[i])), f);
+	  fputs_filtered (hex_string (die->attrs[i].address ()), f);
 	  break;
 	case DW_FORM_block2:
 	case DW_FORM_block4:
@@ -21783,7 +21787,7 @@ dwarf2_fetch_constant_bytes (sect_offset sect_off,
 
 	*len = cu->header.addr_size;
 	tem = (gdb_byte *) obstack_alloc (obstack, *len);
-	store_unsigned_integer (tem, *len, byte_order, DW_ADDR (attr));
+	store_unsigned_integer (tem, *len, byte_order, attr->address ());
 	result = tem;
       }
       break;
-- 
2.17.2

Re: [PATCH 12/20] Remove DW_ADDR

[Simon Marchi]

On 2020-03-28 3:22 p.m., Tom Tromey wrote:
> @@ -202,6 +203,17 @@ struct attribute
>      requires_reprocessing = 1;
>    }
>  
> +  /* Set this attribute to an address.  */
> +  void set_address (CORE_ADDR addr)
> +  {
> +    gdb_assert (form == DW_FORM_addr
> +		|| ((form == DW_FORM_addrx
> +		     || form == DW_FORM_GNU_addr_index)
> +		    && requires_reprocessing));

I wonder if we should assert

  gdb_assert (!requires_reprocessing);

in attribute::address and attribute::string.  Otherwise, we could miss
reprocessing an attribute and we'd return a bogus value, it seems.

Simon

Re: [PATCH 12/20] Remove DW_ADDR

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I wonder if we should assert

Simon>   gdb_assert (!requires_reprocessing);

Simon> in attribute::address and attribute::string.  Otherwise, we could miss
Simon> reprocessing an attribute and we'd return a bogus value, it seems.

Makes sense, I did this.

Tom

[PATCH 13/20] Change how reprocessing is done

[Tom Tromey]

Currently gdb keeps a vector of attributes that require reprocessing.
However, now that there is a reprocessing flag in the attribute, we
can remove the vector and instead simply loop over attributes a second
time.  Normally there are not many attributes, so this should be
reasonably cheap.

gdb/ChangeLog
2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (skip_one_die): Update.
	(read_full_die_1): Change how reprocessing is done.
	(partial_die_info::read): Update.
	(read_attribute_value): Remove need_reprocess parameter.
	(read_attribute): Likewise.
	* dwarf2/attribute.h (struct attribute) <reprocessing_p>: New
	method.
---
 gdb/ChangeLog          | 10 +++++++++
 gdb/dwarf2/attribute.h |  6 ++++++
 gdb/dwarf2/read.c      | 48 +++++++++++++++++++-----------------------
 3 files changed, 38 insertions(+), 26 deletions(-)

diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index 972beef9825..9b387e5df05 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -214,6 +214,12 @@ struct attribute
     requires_reprocessing = 0;
   }
 
+  /* True if this attribute requires reprocessing.  */
+  bool reprocessing_p () const
+  {
+    return requires_reprocessing;
+  }
+
 
   ENUM_BITFIELD(dwarf_attribute) name : 15;
 
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index b010c9c3b01..d3897ac2198 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -1192,7 +1192,7 @@ static const struct cu_partial_die_info find_partial_die (sect_offset, int,
 
 static const gdb_byte *read_attribute (const struct die_reader_specs *,
 				       struct attribute *, struct attr_abbrev *,
-				       const gdb_byte *, bool *need_reprocess);
+				       const gdb_byte *);
 
 static void read_attribute_reprocess (const struct die_reader_specs *reader,
 				      struct attribute *attr);
@@ -8464,9 +8464,7 @@ skip_one_die (const struct die_reader_specs *reader, const gdb_byte *info_ptr,
       /* The only abbrev we care about is DW_AT_sibling.  */
       if (abbrev->attrs[i].name == DW_AT_sibling)
 	{
-	  bool ignored;
-	  read_attribute (reader, &attr, &abbrev->attrs[i], info_ptr,
-			  &ignored);
+	  read_attribute (reader, &attr, &abbrev->attrs[i], info_ptr);
 	  if (attr.form == DW_FORM_ref_addr)
 	    complaint (_("ignoring absolute DW_AT_sibling"));
 	  else
@@ -17484,15 +17482,13 @@ read_full_die_1 (const struct die_reader_specs *reader,
      attributes.  */
   die->num_attrs = abbrev->num_attrs;
 
-  std::vector<int> indexes_that_need_reprocess;
+  bool any_need_reprocess = false;
   for (i = 0; i < abbrev->num_attrs; ++i)
     {
-      bool need_reprocess;
-      info_ptr =
-        read_attribute (reader, &die->attrs[i], &abbrev->attrs[i],
-			info_ptr, &need_reprocess);
-      if (need_reprocess)
-        indexes_that_need_reprocess.push_back (i);
+      info_ptr = read_attribute (reader, &die->attrs[i], &abbrev->attrs[i],
+				 info_ptr);
+      if (die->attrs[i].reprocessing_p ())
+	any_need_reprocess = true;
     }
 
   struct attribute *attr = die->attr (DW_AT_str_offsets_base);
@@ -17502,8 +17498,14 @@ read_full_die_1 (const struct die_reader_specs *reader,
   auto maybe_addr_base = die->addr_base ();
   if (maybe_addr_base.has_value ())
     cu->addr_base = *maybe_addr_base;
-  for (int index : indexes_that_need_reprocess)
-    read_attribute_reprocess (reader, &die->attrs[index]);
+  if (any_need_reprocess)
+    {
+      for (i = 0; i < abbrev->num_attrs; ++i)
+	{
+	  if (die->attrs[i].reprocessing_p ())
+	    read_attribute_reprocess (reader, &die->attrs[i]);
+	}
+    }
   *diep = die;
   return info_ptr;
 }
@@ -17857,13 +17859,12 @@ partial_die_info::read (const struct die_reader_specs *reader,
   std::vector<struct attribute> attr_vec (abbrev.num_attrs);
   for (i = 0; i < abbrev.num_attrs; ++i)
     {
-      bool need_reprocess;
       info_ptr = read_attribute (reader, &attr_vec[i], &abbrev.attrs[i],
-				 info_ptr, &need_reprocess);
+				 info_ptr);
       /* String and address offsets that need to do the reprocessing have
          already been read at this point, so there is no need to wait until
 	 the loop terminates to do the reprocessing.  */
-      if (need_reprocess)
+      if (attr_vec[i].reprocessing_p ())
 	read_attribute_reprocess (reader, &attr_vec[i]);
       attribute &attr = attr_vec[i];
       /* Store the data if it is of an attribute we want to keep in a
@@ -18342,8 +18343,7 @@ read_attribute_reprocess (const struct die_reader_specs *reader,
 static const gdb_byte *
 read_attribute_value (const struct die_reader_specs *reader,
 		      struct attribute *attr, unsigned form,
-		      LONGEST implicit_const, const gdb_byte *info_ptr,
-		      bool *need_reprocess)
+		      LONGEST implicit_const, const gdb_byte *info_ptr)
 {
   struct dwarf2_cu *cu = reader->cu;
   struct dwarf2_per_objfile *dwarf2_per_objfile
@@ -18354,7 +18354,6 @@ read_attribute_value (const struct die_reader_specs *reader,
   struct comp_unit_head *cu_header = &cu->header;
   unsigned int bytes_read;
   struct dwarf_block *blk;
-  *need_reprocess = false;
 
   attr->form = (enum dwarf_form) form;
   switch (form)
@@ -18528,14 +18527,13 @@ read_attribute_value (const struct die_reader_specs *reader,
 	  info_ptr += bytes_read;
 	}
       info_ptr = read_attribute_value (reader, attr, form, implicit_const,
-				       info_ptr, need_reprocess);
+				       info_ptr);
       break;
     case DW_FORM_implicit_const:
       attr->set_signed (implicit_const);
       break;
     case DW_FORM_addrx:
     case DW_FORM_GNU_addr_index:
-      *need_reprocess = true;
       attr->set_unsigned_reprocess (read_unsigned_leb128 (abfd, info_ptr,
 							  &bytes_read));
       info_ptr += bytes_read;
@@ -18573,9 +18571,8 @@ read_attribute_value (const struct die_reader_specs *reader,
 	    str_index = read_unsigned_leb128 (abfd, info_ptr, &bytes_read);
 	    info_ptr += bytes_read;
 	  }
-	*need_reprocess = true;
 	attr->set_unsigned_reprocess (str_index);
-	}
+      }
       break;
     default:
       error (_("Dwarf Error: Cannot handle %s in DWARF reader [in module %s]"),
@@ -18611,14 +18608,13 @@ read_attribute_value (const struct die_reader_specs *reader,
 static const gdb_byte *
 read_attribute (const struct die_reader_specs *reader,
 		struct attribute *attr, struct attr_abbrev *abbrev,
-		const gdb_byte *info_ptr, bool *need_reprocess)
+		const gdb_byte *info_ptr)
 {
   attr->name = abbrev->name;
   attr->string_is_canonical = 0;
   attr->requires_reprocessing = 0;
   return read_attribute_value (reader, attr, abbrev->form,
-			       abbrev->implicit_const, info_ptr,
-			       need_reprocess);
+			       abbrev->implicit_const, info_ptr);
 }
 
 /* Return pointer to string at .debug_str offset STR_OFFSET.  */
-- 
2.17.2

Re: [PATCH 13/20] Change how reprocessing is done

[Simon Marchi]

On 2020-03-28 3:22 p.m., Tom Tromey wrote:
> Currently gdb keeps a vector of attributes that require reprocessing.
> However, now that there is a reprocessing flag in the attribute, we
> can remove the vector and instead simply loop over attributes a second
> time.  Normally there are not many attributes, so this should be
> reasonably cheap.

That sounds reasonable, it simplifies the code.

> 
> gdb/ChangeLog
> 2020-03-28  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2/read.c (skip_one_die): Update.
> 	(read_full_die_1): Change how reprocessing is done.
> 	(partial_die_info::read): Update.
> 	(read_attribute_value): Remove need_reprocess parameter.
> 	(read_attribute): Likewise.
> 	* dwarf2/attribute.h (struct attribute) <reprocessing_p>: New
> 	method.
> ---
>  gdb/ChangeLog          | 10 +++++++++
>  gdb/dwarf2/attribute.h |  6 ++++++
>  gdb/dwarf2/read.c      | 48 +++++++++++++++++++-----------------------
>  3 files changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
> index 972beef9825..9b387e5df05 100644
> --- a/gdb/dwarf2/attribute.h
> +++ b/gdb/dwarf2/attribute.h
> @@ -214,6 +214,12 @@ struct attribute
>      requires_reprocessing = 0;
>    }
>  
> +  /* True if this attribute requires reprocessing.  */
> +  bool reprocessing_p () const
> +  {
> +    return requires_reprocessing;
> +  }

Nitpicking on names again: the method name sounds like we inquire whether
the attribute is currently reprocessing (as if it was a lengthy operation).

So I'd name it requires_reprocessing_p.  Or just requires_reprocessing (I still
find those _p suffixes for predicates odd).

Simon

Re: [PATCH 13/20] Change how reprocessing is done

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> +  /* True if this attribute requires reprocessing.  */
>> +  bool reprocessing_p () const
>> +  {
>> +    return requires_reprocessing;
>> +  }

Simon> Nitpicking on names again: the method name sounds like we inquire whether
Simon> the attribute is currently reprocessing (as if it was a lengthy operation).

Simon> So I'd name it requires_reprocessing_p.  Or just requires_reprocessing (I still
Simon> find those _p suffixes for predicates odd).

Sounds good, I used requires_reprocessing_p.

Tom

[PATCH 14/20] Change how accessibility is handled in dwarf2/read.c

[Tom Tromey]

dwarf2/read.c uses dwarf2_default_access_attribute to check for the
default access attribute.  This patch simplifies the code by moving
more of the access processing into this function, changing its name to
reflect the difference.  This also ensures that the attribute's form
is respected, by changing to code to use the constant_value method.

gdb/ChangeLog
2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (dwarf2_access_attribute): Rename from
	dwarf2_default_access_attribute.  Look up attribute.
	(dwarf2_add_field, dwarf2_add_type_defn, dwarf2_add_member_fn):
	Update.
---
 gdb/ChangeLog     |  7 +++++++
 gdb/dwarf2/read.c | 38 +++++++++++++++++---------------------
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d3897ac2198..efa59fcab4d 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -14001,12 +14001,24 @@ producer_is_codewarrior (struct dwarf2_cu *cu)
   return cu->producer_is_codewarrior;
 }
 
-/* Return the default accessibility type if it is not overridden by
+/* Return the accessibility type if it is not overridden by
    DW_AT_accessibility.  */
 
 static enum dwarf_access_attribute
-dwarf2_default_access_attribute (struct die_info *die, struct dwarf2_cu *cu)
+dwarf2_access_attribute (struct die_info *die, struct dwarf2_cu *cu)
 {
+  attribute *attr = dwarf2_attr (die, DW_AT_accessibility, cu);
+  if (attr != nullptr)
+    {
+      LONGEST value = attr->constant_value (-1);
+      if (value == DW_ACCESS_public
+	  || value == DW_ACCESS_protected
+	  || value == DW_ACCESS_private)
+	return (dwarf_access_attribute) value;
+      complaint (_("Unhandled DW_AT_accessibility value (%s)"),
+		 plongest (value));
+    }
+
   if (cu->header.version < 3 || producer_is_gxx_lt_4_6 (cu))
     {
       /* The default DWARF 2 accessibility for members is public, the default
@@ -14089,11 +14101,7 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
       new_field = &fip->fields.back ();
     }
 
-  attr = dwarf2_attr (die, DW_AT_accessibility, cu);
-  if (attr != nullptr)
-    new_field->accessibility = DW_UNSND (attr);
-  else
-    new_field->accessibility = dwarf2_default_access_attribute (die, cu);
+  new_field->accessibility = dwarf2_access_attribute (die, cu);
   if (new_field->accessibility != DW_ACCESS_public)
     fip->non_public_fields = 1;
 
@@ -14315,12 +14323,7 @@ dwarf2_add_type_defn (struct field_info *fip, struct die_info *die,
   fp.type = read_type_die (die, cu);
 
   /* Save accessibility.  */
-  enum dwarf_access_attribute accessibility;
-  struct attribute *attr = dwarf2_attr (die, DW_AT_accessibility, cu);
-  if (attr != NULL)
-    accessibility = (enum dwarf_access_attribute) DW_UNSND (attr);
-  else
-    accessibility = dwarf2_default_access_attribute (die, cu);
+  dwarf_access_attribute accessibility = dwarf2_access_attribute (die, cu);
   switch (accessibility)
     {
     case DW_ACCESS_public:
@@ -14332,8 +14335,6 @@ dwarf2_add_type_defn (struct field_info *fip, struct die_info *die,
     case DW_ACCESS_protected:
       fp.is_protected = 1;
       break;
-    default:
-      complaint (_("Unhandled DW_AT_accessibility value (%x)"), accessibility);
     }
 
   if (die->tag == DW_TAG_typedef)
@@ -14510,7 +14511,6 @@ dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
   struct fn_field *fnp;
   const char *fieldname;
   struct type *this_type;
-  enum dwarf_access_attribute accessibility;
 
   if (cu->language == language_ada)
     error (_("unexpected member function in Ada type"));
@@ -14589,11 +14589,7 @@ dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
      is_volatile is irrelevant, as it is needed by gdb_mangle_name only.  */
 
   /* Get accessibility.  */
-  attr = dwarf2_attr (die, DW_AT_accessibility, cu);
-  if (attr != nullptr)
-    accessibility = (enum dwarf_access_attribute) DW_UNSND (attr);
-  else
-    accessibility = dwarf2_default_access_attribute (die, cu);
+  dwarf_access_attribute accessibility = dwarf2_access_attribute (die, cu);
   switch (accessibility)
     {
     case DW_ACCESS_private:
-- 
2.17.2

Re: [PATCH 14/20] Change how accessibility is handled in dwarf2/read.c

[Simon Marchi]

On 2020-03-28 3:22 p.m., Tom Tromey wrote:
> dwarf2/read.c uses dwarf2_default_access_attribute to check for the
> default access attribute.  This patch simplifies the code by moving
> more of the access processing into this function, changing its name to
> reflect the difference.  This also ensures that the attribute's form
> is respected, by changing to code to use the constant_value method.
> 
> gdb/ChangeLog
> 2020-03-28  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2/read.c (dwarf2_access_attribute): Rename from
> 	dwarf2_default_access_attribute.  Look up attribute.
> 	(dwarf2_add_field, dwarf2_add_type_defn, dwarf2_add_member_fn):
> 	Update.
> ---
>  gdb/ChangeLog     |  7 +++++++
>  gdb/dwarf2/read.c | 38 +++++++++++++++++---------------------
>  2 files changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index d3897ac2198..efa59fcab4d 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -14001,12 +14001,24 @@ producer_is_codewarrior (struct dwarf2_cu *cu)
>    return cu->producer_is_codewarrior;
>  }
>  
> -/* Return the default accessibility type if it is not overridden by
> +/* Return the accessibility type if it is not overridden by
>     DW_AT_accessibility.  */

Maybe I don't understand correctly, but I think the "if it is not overridden by
DW_AT_accessibility" part of the comment is no longer true.  It's more, return
the value of DW_AT_accessibility, or if it's not present, the default accessibility
value.

Otherwise, LGTM.

Simon

[PATCH 15/20] Add attribute::get_unsigned method

[Tom Tromey]

This introduces a new attribute::get_unsigned method and changes a few
spots to use it.

gdb/ChangeLog
2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (dw2_get_file_names_reader)
	(dwarf2_build_include_psymtabs, handle_DW_AT_stmt_list)
	(dwarf2_cu::setup_type_unit_groups, fill_in_loclist_baton)
	(dwarf2_symbol_mark_computed): Use get_unsigned.
	* dwarf2/attribute.h (struct attribute) <get_unsigned>: New
	method.
	<form_is_section_offset>: Update comment.
---
 gdb/ChangeLog          | 10 ++++++++++
 gdb/dwarf2/attribute.h | 11 ++++++++++-
 gdb/dwarf2/read.c      | 22 +++++++++++-----------
 3 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index 9b387e5df05..546156283b3 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -82,9 +82,18 @@ struct attribute
     return u.unsnd;
   }
 
+  /* Return the unsigned value.  Requires that the form be an unsigned
+     form, and that reprocessing not be needed.  */
+  ULONGEST get_unsigned () const
+  {
+    gdb_assert (form_is_unsigned ());
+    gdb_assert (!requires_reprocessing);
+    return u.unsnd;
+  }
+
   /* Return non-zero if ATTR's value is a section offset --- classes
      lineptr, loclistptr, macptr or rangelistptr --- or zero, otherwise.
-     You may use DW_UNSND (attr) to retrieve such offsets.
+     You may use the get_unsigned method to retrieve such offsets.
 
      Section 7.5.4, "Attribute Encodings", explains that no attribute
      may have a value that belongs to more than one of these classes; it
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index efa59fcab4d..abeb0e1a4e9 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -3046,11 +3046,11 @@ dw2_get_file_names_reader (const struct die_reader_specs *reader,
   sect_offset line_offset {};
 
   attr = dwarf2_attr (comp_unit_die, DW_AT_stmt_list, cu);
-  if (attr != nullptr)
+  if (attr != nullptr && attr->form_is_unsigned ())
     {
       struct quick_file_names find_entry;
 
-      line_offset = (sect_offset) DW_UNSND (attr);
+      line_offset = (sect_offset) attr->get_unsigned ();
 
       /* We may have already read in this line header (TU line header sharing).
 	 If we have we're done.  */
@@ -5892,8 +5892,8 @@ dwarf2_build_include_psymtabs (struct dwarf2_cu *cu,
   struct attribute *attr;
 
   attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
-  if (attr != nullptr)
-    lh = dwarf_decode_line_header ((sect_offset) DW_UNSND (attr), cu);
+  if (attr != nullptr && attr->form_is_unsigned ())
+    lh = dwarf_decode_line_header ((sect_offset) attr->get_unsigned (), cu);
   if (lh == NULL)
     return;  /* No linetable, so no includes.  */
 
@@ -10560,10 +10560,10 @@ handle_DW_AT_stmt_list (struct die_info *die, struct dwarf2_cu *cu,
   gdb_assert (! cu->per_cu->is_debug_types);
 
   attr = dwarf2_attr (die, DW_AT_stmt_list, cu);
-  if (attr == NULL)
+  if (attr == NULL || !attr->form_is_unsigned ())
     return;
 
-  sect_offset line_offset = (sect_offset) DW_UNSND (attr);
+  sect_offset line_offset = (sect_offset) attr->get_unsigned ();
 
   /* The line header hash table is only created if needed (it exists to
      prevent redundant reading of the line table for partial_units).
@@ -10752,9 +10752,9 @@ dwarf2_cu::setup_type_unit_groups (struct die_info *die)
   /* We have to handle the case of both a missing DW_AT_stmt_list or bad
      debug info.  */
   line_header_up lh;
-  if (attr != NULL)
+  if (attr != NULL && attr->form_is_unsigned ())
     {
-      sect_offset line_offset = (sect_offset) DW_UNSND (attr);
+      sect_offset line_offset = (sect_offset) attr->get_unsigned ();
       lh = dwarf_decode_line_header (line_offset, this);
     }
   if (lh == NULL)
@@ -22526,8 +22526,8 @@ fill_in_loclist_baton (struct dwarf2_cu *cu,
   gdb_assert (baton->per_cu);
   /* We don't know how long the location list is, but make sure we
      don't run off the edge of the section.  */
-  baton->size = section->size - DW_UNSND (attr);
-  baton->data = section->buffer + DW_UNSND (attr);
+  baton->size = section->size - attr->get_unsigned ();
+  baton->data = section->buffer + attr->get_unsigned ();
   if (cu->base_address.has_value ())
     baton->base_address = *cu->base_address;
   else
@@ -22548,7 +22548,7 @@ dwarf2_symbol_mark_computed (const struct attribute *attr, struct symbol *sym,
       /* .debug_loc{,.dwo} may not exist at all, or the offset may be outside
 	 the section.  If so, fall through to the complaint in the
 	 other branch.  */
-      && DW_UNSND (attr) < section->get_size (objfile))
+      && attr->get_unsigned () < section->get_size (objfile))
     {
       struct dwarf2_loclist_baton *baton;
 
-- 
2.17.2

Re: [PATCH 15/20] Add attribute::get_unsigned method

[Simon Marchi]

On 2020-03-28 3:22 p.m., Tom Tromey wrote:
> This introduces a new attribute::get_unsigned method and changes a few
> spots to use it.
> 
> gdb/ChangeLog
> 2020-03-28  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2/read.c (dw2_get_file_names_reader)
> 	(dwarf2_build_include_psymtabs, handle_DW_AT_stmt_list)
> 	(dwarf2_cu::setup_type_unit_groups, fill_in_loclist_baton)
> 	(dwarf2_symbol_mark_computed): Use get_unsigned.
> 	* dwarf2/attribute.h (struct attribute) <get_unsigned>: New
> 	method.
> 	<form_is_section_offset>: Update comment.
> ---
>  gdb/ChangeLog          | 10 ++++++++++
>  gdb/dwarf2/attribute.h | 11 ++++++++++-
>  gdb/dwarf2/read.c      | 22 +++++++++++-----------
>  3 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
> index 9b387e5df05..546156283b3 100644
> --- a/gdb/dwarf2/attribute.h
> +++ b/gdb/dwarf2/attribute.h
> @@ -82,9 +82,18 @@ struct attribute
>      return u.unsnd;
>    }
>  
> +  /* Return the unsigned value.  Requires that the form be an unsigned
> +     form, and that reprocessing not be needed.  */
> +  ULONGEST get_unsigned () const
> +  {
> +    gdb_assert (form_is_unsigned ());
> +    gdb_assert (!requires_reprocessing);

I see what you're trying to do here, but I don't think it's really useful to assert !requires_reprocessing.

For string and address forms that require reprocessing, it's true that we set an unsigned value, but the
form is still some string of address form (we don't change it to some unsigned form).  So it's not really
possible for form_is_unsigned()  to return true, and requires_reprocessing to be true.

Instead, gdb_assert (!requires_reprocessing) should be added to ::address and ::string.

Note that I don't mind if we leave the assert in this function, it's true in any case that requires_reprocessing
should be false at this point.

Also, I noted that this method is named "get_unsigned", since you obviously can't name it "unsigned".  If you
used my idea to name the others "as_string" and "as_address", this one could be "as_unsigned", and the names
would all be coherent.

Simon

Re: [PATCH 15/20] Add attribute::get_unsigned method

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Instead, gdb_assert (!requires_reprocessing) should be added to
Simon> ::address and ::string.

I did this in the earlier patch.

Simon> Note that I don't mind if we leave the assert in this function,
Simon> it's true in any case that requires_reprocessing should be false
Simon> at this point.

I guess I'll leave it for completeness.

Simon> Also, I noted that this method is named "get_unsigned", since you
Simon> obviously can't name it "unsigned".  If you used my idea to name
Simon> the others "as_string" and "as_address", this one could be
Simon> "as_unsigned", and the names would all be coherent.

Yep, I'm doing this throughout.

Tom

[PATCH 16/20] Change is_valid_DW_AT_defaulted to a method on attribute

[Tom Tromey]

This changes is_valid_DW_AT_defaulted to be a method on struct attribute.
Now it correctly respects the form of the attribute.

2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (is_valid_DW_AT_defaulted): Move to attribute.c.
	(dwarf2_add_member_fn): Update.
	* dwarf2/attribute.h (struct attribute) <defaulted>: Declare.
	* dwarf2/attribute.c (attribute::defaulted): New method, from
	is_valid_DW_AT_defaulted.
---
 gdb/ChangeLog          |  8 ++++++++
 gdb/dwarf2/attribute.c | 21 +++++++++++++++++++++
 gdb/dwarf2/attribute.h |  8 ++++++++
 gdb/dwarf2/read.c      | 23 ++---------------------
 4 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
index 94bedf6dbd3..ee5e6321bde 100644
--- a/gdb/dwarf2/attribute.c
+++ b/gdb/dwarf2/attribute.c
@@ -219,3 +219,24 @@ attribute::form_is_reprocessed () const
 	  || form == DW_FORM_addrx
 	  || form == DW_FORM_GNU_addr_index);
 }
+
+/* See attribute.h.  */
+
+dwarf_defaulted_attribute
+attribute::defaulted () const
+{
+  LONGEST value = constant_value (-1);
+
+  switch (value)
+    {
+    case DW_DEFAULTED_no:
+    case DW_DEFAULTED_in_class:
+    case DW_DEFAULTED_out_of_class:
+      return (dwarf_defaulted_attribute) value;
+    }
+
+  if (form_is_constant ())
+    complaint (_("unrecognized DW_AT_defaulted value (%s)"),
+	       plongest (value));
+  return DW_DEFAULTED_no;
+}
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index 546156283b3..dce93b154a8 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -29,6 +29,7 @@
 
 #include "dwarf2.h"
 #include "gdbtypes.h"
+#include "gdbsupport/gdb_optional.h"
 
 /* Blocks are a bunch of untyped bytes.  */
 struct dwarf_block
@@ -229,6 +230,13 @@ struct attribute
     return requires_reprocessing;
   }
 
+  /* Return the value as one of the recognized enum
+     dwarf_defaulted_attribute constants according to DWARF5 spec,
+     Table 7.24.  If the value is incorrect, or if this attribute has
+     the wrong form, then a complaint is issued and DW_DEFAULTED_no is
+     returned.  */
+  dwarf_defaulted_attribute defaulted () const;
+
 
   ENUM_BITFIELD(dwarf_attribute) name : 15;
 
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index abeb0e1a4e9..ec4e6e2e3b4 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -14479,25 +14479,6 @@ dwarf2_is_constructor (struct die_info *die, struct dwarf2_cu *cu)
 	  && (type_name[len] == '\0' || type_name[len] == '<'));
 }
 
-/* Check if the given VALUE is a recognized enum
-   dwarf_defaulted_attribute constant according to DWARF5 spec,
-   Table 7.24.  */
-
-static bool
-is_valid_DW_AT_defaulted (ULONGEST value)
-{
-  switch (value)
-    {
-    case DW_DEFAULTED_no:
-    case DW_DEFAULTED_in_class:
-    case DW_DEFAULTED_out_of_class:
-      return true;
-    }
-
-  complaint (_("unrecognized DW_AT_defaulted value (%s)"), pulongest (value));
-  return false;
-}
-
 /* Add a member function to the proper fieldlist.  */
 
 static void
@@ -14607,8 +14588,8 @@ dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
 
   /* Check for defaulted methods.  */
   attr = dwarf2_attr (die, DW_AT_defaulted, cu);
-  if (attr != nullptr && is_valid_DW_AT_defaulted (DW_UNSND (attr)))
-    fnp->defaulted = (enum dwarf_defaulted_attribute) DW_UNSND (attr);
+  if (attr != nullptr)
+    fnp->defaulted = attr->defaulted ();
 
   /* Check for deleted methods.  */
   attr = dwarf2_attr (die, DW_AT_deleted, cu);
-- 
2.17.2

Re: [PATCH 16/20] Change is_valid_DW_AT_defaulted to a method on attribute

[Simon Marchi]

On 2020-03-28 3:22 p.m., Tom Tromey wrote:
> This changes is_valid_DW_AT_defaulted to be a method on struct attribute.
> Now it correctly respects the form of the attribute.
> 
> 2020-03-28  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2/read.c (is_valid_DW_AT_defaulted): Move to attribute.c.
> 	(dwarf2_add_member_fn): Update.
> 	* dwarf2/attribute.h (struct attribute) <defaulted>: Declare.
> 	* dwarf2/attribute.c (attribute::defaulted): New method, from
> 	is_valid_DW_AT_defaulted.
> ---
>  gdb/ChangeLog          |  8 ++++++++
>  gdb/dwarf2/attribute.c | 21 +++++++++++++++++++++
>  gdb/dwarf2/attribute.h |  8 ++++++++
>  gdb/dwarf2/read.c      | 23 ++---------------------
>  4 files changed, 39 insertions(+), 21 deletions(-)
> 
> diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
> index 94bedf6dbd3..ee5e6321bde 100644
> --- a/gdb/dwarf2/attribute.c
> +++ b/gdb/dwarf2/attribute.c
> @@ -219,3 +219,24 @@ attribute::form_is_reprocessed () const
>  	  || form == DW_FORM_addrx
>  	  || form == DW_FORM_GNU_addr_index);
>  }
> +
> +/* See attribute.h.  */
> +
> +dwarf_defaulted_attribute
> +attribute::defaulted () const
> +{
> +  LONGEST value = constant_value (-1);
> +
> +  switch (value)
> +    {
> +    case DW_DEFAULTED_no:
> +    case DW_DEFAULTED_in_class:
> +    case DW_DEFAULTED_out_of_class:
> +      return (dwarf_defaulted_attribute) value;
> +    }
> +
> +  if (form_is_constant ())
> +    complaint (_("unrecognized DW_AT_defaulted value (%s)"),
> +	       plongest (value));
> +  return DW_DEFAULTED_no;

I don't quite understand the logic here, we check form_is_constant after having called
constant_value?  If the form is not a constant, shouldn't we emit a warning to that
effect (and return _no)?

> +}
> diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
> index 546156283b3..dce93b154a8 100644
> --- a/gdb/dwarf2/attribute.h
> +++ b/gdb/dwarf2/attribute.h
> @@ -29,6 +29,7 @@
>  
>  #include "dwarf2.h"
>  #include "gdbtypes.h"
> +#include "gdbsupport/gdb_optional.h"
>  
>  /* Blocks are a bunch of untyped bytes.  */
>  struct dwarf_block
> @@ -229,6 +230,13 @@ struct attribute
>      return requires_reprocessing;
>    }
>  
> +  /* Return the value as one of the recognized enum
> +     dwarf_defaulted_attribute constants according to DWARF5 spec,
> +     Table 7.24.  If the value is incorrect, or if this attribute has
> +     the wrong form, then a complaint is issued and DW_DEFAULTED_no is
> +     returned.  */
> +  dwarf_defaulted_attribute defaulted () const;
> +
>  

There's an extra empty line here, might as well remove it.

Simon

Re: [PATCH 16/20] Change is_valid_DW_AT_defaulted to a method on attribute

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> +  if (form_is_constant ())
>> +    complaint (_("unrecognized DW_AT_defaulted value (%s)"),
>> +	       plongest (value));
>> +  return DW_DEFAULTED_no;

Simon> I don't quite understand the logic here, we check
Simon> form_is_constant after having called constant_value?  If the form
Simon> is not a constant, shouldn't we emit a warning to that effect
Simon> (and return _no)?

In this case the complaint is issued by constant_value.  This avoids
duplicate complaints.  I will make a note to this effect.

>> +  /* Return the value as one of the recognized enum
>> +     dwarf_defaulted_attribute constants according to DWARF5 spec,
>> +     Table 7.24.  If the value is incorrect, or if this attribute has
>> +     the wrong form, then a complaint is issued and DW_DEFAULTED_no is
>> +     returned.  */
>> +  dwarf_defaulted_attribute defaulted () const;
>> +

Simon> There's an extra empty line here, might as well remove it.

I often leave space between the methods and the data members, but I
suppose I don't really care that much.

It would be better to add "private:" but that requires another round of
patching.

Tom

[PATCH 17/20] Change die_info methods to check the attribute's form

[Tom Tromey]

This changes two die_info methods to check the form of the attribute
before using it.

2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/die.h (struct die_info) <addr_base, ranges_base>: Check
	the attribute's form.
---
 gdb/ChangeLog    |  5 +++++
 gdb/dwarf2/die.h | 14 ++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/gdb/dwarf2/die.h b/gdb/dwarf2/die.h
index 5522ebdf311..37f83a45a50 100644
--- a/gdb/dwarf2/die.h
+++ b/gdb/dwarf2/die.h
@@ -39,11 +39,12 @@ struct die_info
   gdb::optional<ULONGEST> addr_base ()
   {
     for (unsigned i = 0; i < num_attrs; ++i)
-      if (attrs[i].name == DW_AT_addr_base
-	  || attrs[i].name == DW_AT_GNU_addr_base)
+      if ((attrs[i].name == DW_AT_addr_base
+	   || attrs[i].name == DW_AT_GNU_addr_base)
+	  && attrs[i].form_is_unsigned ())
 	{
 	  /* If both exist, just use the first one.  */
-	  return DW_UNSND (&attrs[i]);
+	  return attrs[i].get_unsigned ();
 	}
     return gdb::optional<ULONGEST> ();
   }
@@ -54,11 +55,12 @@ struct die_info
   ULONGEST ranges_base ()
   {
     for (unsigned i = 0; i < num_attrs; ++i)
-      if (attrs[i].name == DW_AT_rnglists_base
-	  || attrs[i].name == DW_AT_GNU_ranges_base)
+      if ((attrs[i].name == DW_AT_rnglists_base
+	   || attrs[i].name == DW_AT_GNU_ranges_base)
+	  && attrs[i].form_is_unsigned ())
 	{
 	  /* If both exist, just use the first one.  */
-	  return DW_UNSND (&attrs[i]);
+	  return attrs[i].get_unsigned ();
 	}
     return 0;
   }
-- 
2.17.2

Re: [PATCH 17/20] Change die_info methods to check the attribute's form

[Simon Marchi]

On 2020-03-28 3:22 p.m., Tom Tromey wrote:
> This changes two die_info methods to check the form of the attribute
> before using it.
> 
> 2020-03-28  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2/die.h (struct die_info) <addr_base, ranges_base>: Check
> 	the attribute's form.
> ---
>  gdb/ChangeLog    |  5 +++++
>  gdb/dwarf2/die.h | 14 ++++++++------
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/dwarf2/die.h b/gdb/dwarf2/die.h
> index 5522ebdf311..37f83a45a50 100644
> --- a/gdb/dwarf2/die.h
> +++ b/gdb/dwarf2/die.h
> @@ -39,11 +39,12 @@ struct die_info
>    gdb::optional<ULONGEST> addr_base ()
>    {
>      for (unsigned i = 0; i < num_attrs; ++i)
> -      if (attrs[i].name == DW_AT_addr_base
> -	  || attrs[i].name == DW_AT_GNU_addr_base)
> +      if ((attrs[i].name == DW_AT_addr_base
> +	   || attrs[i].name == DW_AT_GNU_addr_base)
> +	  && attrs[i].form_is_unsigned ())
>  	{
>  	  /* If both exist, just use the first one.  */
> -	  return DW_UNSND (&attrs[i]);
> +	  return attrs[i].get_unsigned ();
>  	}
>      return gdb::optional<ULONGEST> ();
>    }

A bit like in the previous patch, if there is a DW_AT_addr_base, but it's
not of the right form, I think we should emit a warning saying that we ignore
it, instead of just ignoring it silently.

That's probably out of the scope of this patch though, but I'm just noting it.

Simon

Re: [PATCH 17/20] Change die_info methods to check the attribute's form

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> +      if ((attrs[i].name == DW_AT_addr_base
>> +	   || attrs[i].name == DW_AT_GNU_addr_base)
>> +	  && attrs[i].form_is_unsigned ())
>> {
>> /* If both exist, just use the first one.  */
>> -	  return DW_UNSND (&attrs[i]);
>> +	  return attrs[i].get_unsigned ();
>> }
>> return gdb::optional<ULONGEST> ();
>> }

Simon> A bit like in the previous patch, if there is a DW_AT_addr_base, but it's
Simon> not of the right form, I think we should emit a warning saying that we ignore
Simon> it, instead of just ignoring it silently.

I tend to think gdb complaints are just time-wasters TBH.

Normally no one examines them.  They aren't visible to users, and if
they were they wouldn't make sense or be actionable anyway.

I enable complaints in my gdbinit but they've turned out just to be
noise.  In fact, last time I fixed a bug that was noted by a complaint,
it turned out I didn't realize that gdb was complaining until well after
the fact.

I'm all for checking the DWARF output of compilers, but I think it's
better as a separate tool; and should be done in a context where someone
actually wants to fix the compiler bugs.

I guess that's why I left out complaints in some spots.

Tom

Re: [PATCH 17/20] Change die_info methods to check the attribute's form

[Simon Marchi]

On 2020-03-30 3:04 p.m., Tom Tromey wrote:
> I tend to think gdb complaints are just time-wasters TBH.
> 
> Normally no one examines them.  They aren't visible to users, and if
> they were they wouldn't make sense or be actionable anyway.
> 
> I enable complaints in my gdbinit but they've turned out just to be
> noise.  In fact, last time I fixed a bug that was noted by a complaint,
> it turned out I didn't realize that gdb was complaining until well after
> the fact.

Hmm I didn't even know you had to enable them.  When I debug gdb with gdb, I
see some lines like this:

During symbol reading: Member function "~_Sp_counted_base" (offset 0xbc3e90) is virtual but the vtable offset is not specified
During symbol reading: cannot get low and high bounds for subprogram DIE at 0xbd80c0
During symbol reading: Multiple children of DIE 0xbf6816 refer to DIE 0xbf6804 as their abstract origin

I thought those were complaints.

> I'm all for checking the DWARF output of compilers, but I think it's
> better as a separate tool; and should be done in a context where someone
> actually wants to fix the compiler bugs.
> 
> I guess that's why I left out complaints in some spots.

I agree that there's nothing the users can't do much about those issues, so
we shouldn't flood them with meaningless (for them) warnings.  But I'm also
worried that silently ignoring suspicious situations just leads to problems
staying around for longer.

Though if nobody fixes them, they are not really useful.  I'd like to take
the time to take a look at each complaint of GDB and address it (either fix
GDB or open a bug with the compiler), but the reality is that I don't have
time for that.

I think the patches are fine how they are in this regard, this is an issue
orthogonal to the goal of your patchset anyway.

Simon

Re: [PATCH 17/20] Change die_info methods to check the attribute's form

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Hmm I didn't even know you had to enable them.  When I debug gdb
Simon> with gdb, I see some lines like this:

Simon> During symbol reading: Member function "~_Sp_counted_base" (offset 0xbc3e90) is virtual but the vtable offset is not specified
Simon> During symbol reading: cannot get low and high bounds for subprogram DIE at 0xbd80c0
Simon> During symbol reading: Multiple children of DIE 0xbf6816 refer to DIE 0xbf6804 as their abstract origin

Simon> I thought those were complaints.

Yeah.  That happens because gdb-gdb.gdb (lol) has

  set complaints 1

If you try with -nx you won't see these.

Tom

[PATCH 18/20] Add attribute::virtuality method

[Tom Tromey]

This adds a new attribute::virtuality method and changes the DWARF
reader to use it.  This also ensures that the attibute's form will now
be respected.

2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (dwarf2_add_field, dwarf2_add_member_fn): Update.
	* dwarf2/attribute.h (struct attribute) <virtuality>: New method.
	* dwarf2/attribute.c (attribute::virtuality): New method.
---
 gdb/ChangeLog          |  6 ++++++
 gdb/dwarf2/attribute.c | 21 +++++++++++++++++++++
 gdb/dwarf2/attribute.h |  5 +++++
 gdb/dwarf2/read.c      |  4 ++--
 4 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
index ee5e6321bde..7f428279757 100644
--- a/gdb/dwarf2/attribute.c
+++ b/gdb/dwarf2/attribute.c
@@ -240,3 +240,24 @@ attribute::defaulted () const
 	       plongest (value));
   return DW_DEFAULTED_no;
 }
+
+/* See attribute.h.  */
+
+dwarf_virtuality_attribute
+attribute::virtuality () const
+{
+  LONGEST value = constant_value (-1);
+
+  switch (value)
+    {
+    case DW_VIRTUALITY_none:
+    case DW_VIRTUALITY_virtual:
+    case DW_VIRTUALITY_pure_virtual:
+      return (dwarf_virtuality_attribute) value;
+    }
+
+  if (form_is_constant ())
+    complaint (_("unrecognized DW_AT_virtuality value (%s)"),
+	       plongest (value));
+  return DW_VIRTUALITY_none;
+}
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index dce93b154a8..7c0ce1615cf 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -237,6 +237,11 @@ struct attribute
      returned.  */
   dwarf_defaulted_attribute defaulted () const;
 
+  /* Return the attribute's value as a dwarf_virtuality_attribute
+     constant according to DWARF spec.  An unrecognized value will
+     issue a complaint and return DW_VIRTUALITY_none.  */
+  dwarf_virtuality_attribute virtuality () const;
+
 
   ENUM_BITFIELD(dwarf_attribute) name : 15;
 
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ec4e6e2e3b4..66a508c95ea 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -14107,7 +14107,7 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
 
   attr = dwarf2_attr (die, DW_AT_virtuality, cu);
   if (attr != nullptr)
-    new_field->virtuality = DW_UNSND (attr);
+    new_field->virtuality = attr->virtuality ();
   else
     new_field->virtuality = DW_VIRTUALITY_none;
 
@@ -14661,7 +14661,7 @@ dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
   else
     {
       attr = dwarf2_attr (die, DW_AT_virtuality, cu);
-      if (attr && DW_UNSND (attr))
+      if (attr != nullptr && attr->virtuality () != DW_VIRTUALITY_none)
 	{
 	  /* GCC does this, as of 2008-08-25; PR debug/37237.  */
 	  complaint (_("Member function \"%s\" (offset %s) is virtual "
-- 
2.17.2

[PATCH 19/20] Add attribute::boolean method

[Tom Tromey]

This adds a new attribute::boolean method, and updates the reader to
use it.  The main benefit of this change is that now the code will
respect the attribute's form.

2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (read_func_scope, prototyped_function_p)
	(read_subroutine_type, partial_die_info::read)
	(dwarf2_flag_true_p, new_symbol, dump_die_shallow)
	(dwarf2_add_member_fn): Update.
	* dwarf2/attribute.h (struct attribute) <boolean>: Declare.
	* dwarf2/attribute.c (attribute::boolean): New method.
---
 gdb/ChangeLog          |  9 +++++++++
 gdb/dwarf2/attribute.c | 12 ++++++++++++
 gdb/dwarf2/attribute.h |  4 ++++
 gdb/dwarf2/read.c      | 30 +++++++++++++++---------------
 4 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
index 7f428279757..db01f8ac0ca 100644
--- a/gdb/dwarf2/attribute.c
+++ b/gdb/dwarf2/attribute.c
@@ -261,3 +261,15 @@ attribute::virtuality () const
 	       plongest (value));
   return DW_VIRTUALITY_none;
 }
+
+/* See attribute.h.  */
+
+bool
+attribute::boolean () const
+{
+  if (form == DW_FORM_flag_present)
+    return true;
+  else if (form == DW_FORM_flag)
+    return u.unsnd != 0;
+  return constant_value (0) != 0;
+}
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index 7c0ce1615cf..20947a11642 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -242,6 +242,10 @@ struct attribute
      issue a complaint and return DW_VIRTUALITY_none.  */
   dwarf_virtuality_attribute virtuality () const;
 
+  /* Return the attribute's value as a boolean.  An unrecognized form
+     will issue a complaint and return false.  */
+  bool boolean () const;
+
 
   ENUM_BITFIELD(dwarf_attribute) name : 15;
 
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 66a508c95ea..cf4a2a333ef 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -12768,7 +12768,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
       <= PC_BOUNDS_INVALID)
     {
       attr = dwarf2_attr (die, DW_AT_external, cu);
-      if (!attr || !DW_UNSND (attr))
+      if (attr == nullptr || !attr->boolean ())
 	complaint (_("cannot get low and high bounds "
 		     "for subprogram DIE at %s"),
 		   sect_offset_str (die->sect_off));
@@ -14583,7 +14583,7 @@ dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
 
   /* Check for artificial methods.  */
   attr = dwarf2_attr (die, DW_AT_artificial, cu);
-  if (attr && DW_UNSND (attr) != 0)
+  if (attr && attr->boolean ())
     fnp->is_artificial = 1;
 
   /* Check for defaulted methods.  */
@@ -14593,7 +14593,7 @@ dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
 
   /* Check for deleted methods.  */
   attr = dwarf2_attr (die, DW_AT_deleted, cu);
-  if (attr != nullptr && DW_UNSND (attr) != 0)
+  if (attr != nullptr && attr->boolean ())
     fnp->is_deleted = 1;
 
   fnp->is_constructor = dwarf2_is_constructor (die, cu);
@@ -16486,7 +16486,7 @@ prototyped_function_p (struct die_info *die, struct dwarf2_cu *cu)
   struct attribute *attr;
 
   attr = dwarf2_attr (die, DW_AT_prototyped, cu);
-  if (attr && (DW_UNSND (attr) != 0))
+  if (attr && attr->boolean ())
     return 1;
 
   /* The DWARF standard implies that the DW_AT_prototyped attribute
@@ -16555,7 +16555,7 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
   /* Record whether the function returns normally to its caller or not
      if the DWARF producer set that information.  */
   attr = dwarf2_attr (die, DW_AT_noreturn, cu);
-  if (attr && (DW_UNSND (attr) != 0))
+  if (attr && attr->boolean ())
     TYPE_NO_RETURN (ftype) = 1;
 
   /* We need to add the subroutine type to the die immediately so
@@ -16612,7 +16612,7 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
 		 4.5 does not yet generate.  */
 	      attr = dwarf2_attr (child_die, DW_AT_artificial, cu);
 	      if (attr != nullptr)
-		TYPE_FIELD_ARTIFICIAL (ftype, iparams) = DW_UNSND (attr);
+		TYPE_FIELD_ARTIFICIAL (ftype, iparams) = attr->boolean ();
 	      else
 		TYPE_FIELD_ARTIFICIAL (ftype, iparams) = 0;
 	      arg_type = die_type (child_die, cu);
@@ -17906,10 +17906,10 @@ partial_die_info::read (const struct die_reader_specs *reader,
             }
 	  break;
 	case DW_AT_external:
-	  is_external = DW_UNSND (&attr);
+	  is_external = attr.boolean ();
 	  break;
 	case DW_AT_declaration:
-	  is_declaration = DW_UNSND (&attr);
+	  is_declaration = attr.boolean ();
 	  break;
 	case DW_AT_type:
 	  has_type = 1;
@@ -17982,7 +17982,7 @@ partial_die_info::read (const struct die_reader_specs *reader,
 	  break;
 
 	case DW_AT_main_subprogram:
-	  main_subprogram = DW_UNSND (&attr);
+	  main_subprogram = attr.boolean ();
 	  break;
 
 	case DW_AT_ranges:
@@ -18954,7 +18954,7 @@ dwarf2_flag_true_p (struct die_info *die, unsigned name, struct dwarf2_cu *cu)
 {
   struct attribute *attr = dwarf2_attr (die, name, cu);
 
-  return (attr && DW_UNSND (attr));
+  return attr != nullptr && attr->boolean ();
 }
 
 static int
@@ -20064,7 +20064,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 	     finish_block.  */
 	  SYMBOL_ACLASS_INDEX (sym) = LOC_BLOCK;
 	  attr2 = dwarf2_attr (die, DW_AT_external, cu);
-	  if ((attr2 && (DW_UNSND (attr2) != 0))
+	  if ((attr2 != nullptr && attr2->boolean ())
 	      || cu->language == language_ada
 	      || cu->language == language_fortran)
 	    {
@@ -20116,7 +20116,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 	      attr2 = dwarf2_attr (die, DW_AT_external, cu);
 	      if (!suppress_add)
 		{
-		  if (attr2 && (DW_UNSND (attr2) != 0))
+		  if (attr2 != nullptr && attr2->boolean ())
 		    list_to_add = cu->get_builder ()->get_global_symbols ();
 		  else
 		    list_to_add = cu->list_in_scope;
@@ -20144,7 +20144,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 		     out, but the variable address is set to null;
 		     do not add such variables into symbol table.  */
 		}
-	      else if (attr2 && (DW_UNSND (attr2) != 0))
+	      else if (attr2 != nullptr && attr2->boolean ())
 		{
 		  if (SYMBOL_CLASS (sym) == LOC_STATIC
 		      && (objfile->flags & OBJF_MAINLINE) == 0
@@ -20193,7 +20193,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 		  if (!suppress_add)
 		    list_to_add = cu->list_in_scope;
 		}
-	      else if (attr2 && (DW_UNSND (attr2) != 0)
+	      else if (attr2 != nullptr && attr2->boolean ()
 		       && dwarf2_attr (die, DW_AT_type, cu) != NULL)
 		{
 		  /* A variable with DW_AT_external is never static, but it
@@ -21379,7 +21379,7 @@ dump_die_shallow (struct ui_file *f, int indent, struct die_info *die)
 			      die->attrs[i].canonical_p () ? "is" : "not");
 	  break;
 	case DW_FORM_flag:
-	  if (DW_UNSND (&die->attrs[i]))
+	  if (die->attrs[i].boolean ())
 	    fprintf_unfiltered (f, "flag: TRUE");
 	  else
 	    fprintf_unfiltered (f, "flag: FALSE");
-- 
2.17.2

[PATCH 20/20] Remove DW_UNSND

[Tom Tromey]

This removes DW_UNSND, replacing uses with either get_unsigned or
constant_value, depending primarily on whether or not the form is
already known to be appropriate.

2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (lookup_dwo_id, get_type_unit_group)
	(read_file_scope, dwarf2_get_pc_bounds)
	(dwarf2_record_block_ranges, dwarf2_add_field, get_alignment)
	(read_structure_type, handle_struct_member_die)
	(read_enumeration_type, read_array_type, read_set_type)
	(read_tag_pointer_type, read_tag_reference_type)
	(read_subroutine_type, read_base_type, read_subrange_type)
	(read_full_die_1, partial_die_info::read)
	(partial_die_info::read, by, new_symbol)
	(dwarf2_const_value_data, dwarf2_const_value_attr)
	(dump_die_shallow, dwarf2_fetch_constant_bytes)
	(prepare_one_comp_unit): Update.
	* dwarf2/attribute.h (DW_UNSND): Remove.
---
 gdb/ChangeLog          |  16 +++++
 gdb/dwarf2/attribute.h |   4 --
 gdb/dwarf2/read.c      | 152 ++++++++++++++++++++---------------------
 3 files changed, 91 insertions(+), 81 deletions(-)

diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index 20947a11642..04c057cd7d2 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -272,8 +272,4 @@ struct attribute
   u;
 };
 
-/* Get at parts of an attribute structure.  */
-
-#define DW_UNSND(attr)     ((attr)->u.unsnd)
-
 #endif /* GDB_DWARF2_ATTRIBUTE_H */
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index cf4a2a333ef..d772f2b21bf 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6589,9 +6589,9 @@ lookup_dwo_id (struct dwarf2_cu *cu, struct die_info* comp_unit_die)
     return cu->header.signature;
   struct attribute *attr;
   attr = dwarf2_attr (comp_unit_die, DW_AT_GNU_dwo_id, cu);
-  if (attr == nullptr)
+  if (attr == nullptr || !attr->form_is_unsigned ())
     return gdb::optional<ULONGEST> ();
-  return DW_UNSND (attr);
+  return attr->get_unsigned ();
 }
 
 /* Subroutine of cutu_reader to simplify it.
@@ -7081,9 +7081,9 @@ get_type_unit_group (struct dwarf2_cu *cu, const struct attribute *stmt_list)
 
   /* Do we need to create a new group, or can we use an existing one?  */
 
-  if (stmt_list)
+  if (stmt_list != nullptr && stmt_list->form_is_unsigned ())
     {
-      line_offset = DW_UNSND (stmt_list);
+      line_offset = stmt_list->get_unsigned ();
       ++tu_stats->nr_symtab_sharers;
     }
   else
@@ -10704,19 +10704,19 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
   attr = dwarf2_attr (die, DW_AT_macros, cu);
   if (attr == NULL)
     attr = dwarf2_attr (die, DW_AT_GNU_macros, cu);
-  if (attr && cu->line_header)
+  if (attr != nullptr && attr->form_is_unsigned () && cu->line_header)
     {
       if (dwarf2_attr (die, DW_AT_macro_info, cu))
 	complaint (_("CU refers to both DW_AT_macros and DW_AT_macro_info"));
 
-      dwarf_decode_macros (cu, DW_UNSND (attr), 1);
+      dwarf_decode_macros (cu, attr->get_unsigned (), 1);
     }
   else
     {
       attr = dwarf2_attr (die, DW_AT_macro_info, cu);
-      if (attr && cu->line_header)
+      if (attr != nullptr && attr->form_is_unsigned () && cu->line_header)
 	{
-	  unsigned int macro_offset = DW_UNSND (attr);
+	  unsigned int macro_offset = attr->get_unsigned ();
 
 	  dwarf_decode_macros (cu, macro_offset, 0);
 	}
@@ -13736,13 +13736,13 @@ dwarf2_get_pc_bounds (struct die_info *die, CORE_ADDR *lowpc,
   else
     {
       attr = dwarf2_attr (die, DW_AT_ranges, cu);
-      if (attr != NULL)
+      if (attr != nullptr && attr->form_is_unsigned ())
 	{
 	  /* DW_AT_rnglists_base does not apply to DIEs from the DWO skeleton.
 	     We take advantage of the fact that DW_AT_ranges does not appear
 	     in DW_TAG_compile_unit of DWO files.  */
 	  int need_ranges_base = die->tag != DW_TAG_compile_unit;
-	  unsigned int ranges_offset = (DW_UNSND (attr)
+	  unsigned int ranges_offset = (attr->get_unsigned ()
 					+ (need_ranges_base
 					   ? cu->ranges_base
 					   : 0));
@@ -13907,7 +13907,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
     }
 
   attr = dwarf2_attr (die, DW_AT_ranges, cu);
-  if (attr != nullptr)
+  if (attr != nullptr && attr->form_is_unsigned ())
     {
       /* DW_AT_rnglists_base does not apply to DIEs from the DWO skeleton.
 	 We take advantage of the fact that DW_AT_ranges does not appear
@@ -13916,7 +13916,7 @@ dwarf2_record_block_ranges (struct die_info *die, struct block *block,
 
       /* The value of the DW_AT_ranges attribute is the offset of the
          address range list in the .debug_ranges section.  */
-      unsigned long offset = (DW_UNSND (attr)
+      unsigned long offset = (attr->get_unsigned ()
 			      + (need_ranges_base ? cu->ranges_base : 0));
 
       std::vector<blockrange> blockvec;
@@ -14128,7 +14128,7 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
       attr = dwarf2_attr (die, DW_AT_bit_size, cu);
       if (attr != nullptr)
 	{
-	  FIELD_BITSIZE (*fp) = DW_UNSND (attr);
+	  FIELD_BITSIZE (*fp) = attr->constant_value (0);
 	}
       else
 	{
@@ -14139,7 +14139,7 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
       if (handle_data_member_location (die, cu, &offset))
 	SET_FIELD_BITPOS (*fp, offset * bits_per_byte);
       attr = dwarf2_attr (die, DW_AT_bit_offset, cu);
-      if (attr != nullptr)
+      if (attr != nullptr && attr->form_is_unsigned ())
 	{
 	  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
 	    {
@@ -14148,7 +14148,8 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
 	         anonymous object to the MSB of the field.  We don't
 	         have to do anything special since we don't need to
 	         know the size of the anonymous object.  */
-	      SET_FIELD_BITPOS (*fp, FIELD_BITPOS (*fp) + DW_UNSND (attr));
+	      SET_FIELD_BITPOS (*fp, (FIELD_BITPOS (*fp)
+				      + attr->get_unsigned ()));
 	    }
 	  else
 	    {
@@ -14159,15 +14160,15 @@ dwarf2_add_field (struct field_info *fip, struct die_info *die,
 	         the field itself.  The result is the bit offset of
 	         the LSB of the field.  */
 	      int anonymous_size;
-	      int bit_offset = DW_UNSND (attr);
+	      int bit_offset = attr->get_unsigned ();
 
 	      attr = dwarf2_attr (die, DW_AT_byte_size, cu);
-	      if (attr != nullptr)
+	      if (attr != nullptr && attr->form_is_unsigned ())
 		{
 		  /* The size of the anonymous object containing
 		     the bit field is explicit, so use the
 		     indicated size (in bytes).  */
-		  anonymous_size = DW_UNSND (attr);
+		  anonymous_size = attr->get_unsigned ();
 		}
 	      else
 		{
@@ -14784,22 +14785,16 @@ get_alignment (struct dwarf2_cu *cu, struct die_info *die)
       return 0;
     }
 
-  ULONGEST align;
-  if (attr->form == DW_FORM_sdata)
+  LONGEST val = attr->constant_value (0);
+  if (val < 0)
     {
-      LONGEST val = attr->get_signed ();
-      if (val < 0)
-	{
-	  complaint (_("DW_AT_alignment value must not be negative"
-		       " - DIE at %s [in module %s]"),
-		     sect_offset_str (die->sect_off),
-		     objfile_name (cu->per_cu->dwarf2_per_objfile->objfile));
-	  return 0;
-	}
-      align = val;
+      complaint (_("DW_AT_alignment value must not be negative"
+		   " - DIE at %s [in module %s]"),
+		 sect_offset_str (die->sect_off),
+		 objfile_name (cu->per_cu->dwarf2_per_objfile->objfile));
+      return 0;
     }
-  else
-    align = DW_UNSND (attr);
+  ULONGEST align = val;
 
   if (align == 0)
     {
@@ -14967,18 +14962,18 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
      the default value DW_CC_normal.  */
   attr = dwarf2_attr (die, DW_AT_calling_convention, cu);
   if (attr != nullptr
-      && is_valid_DW_AT_calling_convention_for_type (DW_UNSND (attr)))
+      && is_valid_DW_AT_calling_convention_for_type (attr->constant_value (0)))
     {
       ALLOCATE_CPLUS_STRUCT_TYPE (type);
       TYPE_CPLUS_CALLING_CONVENTION (type)
-	= (enum dwarf_calling_convention) (DW_UNSND (attr));
+	= (enum dwarf_calling_convention) (attr->constant_value (0));
     }
 
   attr = dwarf2_attr (die, DW_AT_byte_size, cu);
   if (attr != nullptr)
     {
       if (attr->form_is_constant ())
-        TYPE_LENGTH (type) = DW_UNSND (attr);
+        TYPE_LENGTH (type) = attr->constant_value (0);
       else
 	{
 	  /* For the moment, dynamic type sizes are not supported
@@ -15108,7 +15103,8 @@ handle_struct_member_die (struct die_info *child_die, struct type *type,
       if (discr == NULL)
 	fi->fields.back ().variant.default_branch = true;
       else
-	fi->fields.back ().variant.discriminant_value = DW_UNSND (discr);
+	fi->fields.back ().variant.discriminant_value
+	  = discr->constant_value (0);
     }
 }
 
@@ -15468,7 +15464,7 @@ read_enumeration_type (struct die_info *die, struct dwarf2_cu *cu)
   attr = dwarf2_attr (die, DW_AT_byte_size, cu);
   if (attr != nullptr)
     {
-      TYPE_LENGTH (type) = DW_UNSND (attr);
+      TYPE_LENGTH (type) = attr->constant_value (0);
     }
   else
     {
@@ -15641,7 +15637,7 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
 
   attr = dwarf2_attr (die, DW_AT_bit_stride, cu);
   if (attr != NULL)
-    bit_stride = DW_UNSND (attr);
+    bit_stride = attr->constant_value (0);
 
   /* Irix 6.2 native cc creates array types without children for
      arrays with unspecified length.  */
@@ -15707,10 +15703,10 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
      implementation may choose to implement triple vectors using this
      attribute.  */
   attr = dwarf2_attr (die, DW_AT_byte_size, cu);
-  if (attr != nullptr)
+  if (attr != nullptr && attr->form_is_unsigned ())
     {
-      if (DW_UNSND (attr) >= TYPE_LENGTH (type))
-	TYPE_LENGTH (type) = DW_UNSND (attr);
+      if (attr->get_unsigned () >= TYPE_LENGTH (type))
+	TYPE_LENGTH (type) = attr->get_unsigned ();
       else
 	complaint (_("DW_AT_byte_size for array type smaller "
 		     "than the total size of elements"));
@@ -15787,8 +15783,8 @@ read_set_type (struct die_info *die, struct dwarf2_cu *cu)
   set_type = create_set_type (NULL, domain_type);
 
   attr = dwarf2_attr (die, DW_AT_byte_size, cu);
-  if (attr != nullptr)
-    TYPE_LENGTH (set_type) = DW_UNSND (attr);
+  if (attr != nullptr && attr->form_is_unsigned ())
+    TYPE_LENGTH (set_type) = attr->get_unsigned ();
 
   maybe_set_alignment (cu, die, set_type);
 
@@ -16144,13 +16140,13 @@ read_tag_pointer_type (struct die_info *die, struct dwarf2_cu *cu)
 
   attr_byte_size = dwarf2_attr (die, DW_AT_byte_size, cu);
   if (attr_byte_size)
-    byte_size = DW_UNSND (attr_byte_size);
+    byte_size = attr_byte_size->constant_value (cu_header->addr_size);
   else
     byte_size = cu_header->addr_size;
 
   attr_address_class = dwarf2_attr (die, DW_AT_address_class, cu);
   if (attr_address_class)
-    addr_class = DW_UNSND (attr_address_class);
+    addr_class = attr_address_class->constant_value (DW_ADDR_none);
   else
     addr_class = DW_ADDR_none;
 
@@ -16256,7 +16252,7 @@ read_tag_reference_type (struct die_info *die, struct dwarf2_cu *cu,
   attr = dwarf2_attr (die, DW_AT_byte_size, cu);
   if (attr != nullptr)
     {
-      TYPE_LENGTH (type) = DW_UNSND (attr);
+      TYPE_LENGTH (type) = attr->constant_value (cu_header->addr_size);
     }
   else
     {
@@ -16544,9 +16540,9 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
      the default value DW_CC_normal.  */
   attr = dwarf2_attr (die, DW_AT_calling_convention, cu);
   if (attr != nullptr
-      && is_valid_DW_AT_calling_convention_for_subroutine (DW_UNSND (attr)))
+      && is_valid_DW_AT_calling_convention_for_subroutine (attr->constant_value (0)))
     TYPE_CALLING_CONVENTION (ftype)
-      = (enum dwarf_calling_convention) (DW_UNSND (attr));
+      = (enum dwarf_calling_convention) attr->constant_value (0);
   else if (cu->producer && strstr (cu->producer, "IBM XL C for OpenCL"))
     TYPE_CALLING_CONVENTION (ftype) = DW_CC_GDB_IBM_OpenCL;
   else
@@ -16814,11 +16810,11 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
   gdbarch *arch;
 
   attr = dwarf2_attr (die, DW_AT_encoding, cu);
-  if (attr != nullptr)
-    encoding = DW_UNSND (attr);
+  if (attr != nullptr && attr->form_is_constant ())
+    encoding = attr->constant_value (0);
   attr = dwarf2_attr (die, DW_AT_byte_size, cu);
   if (attr != nullptr)
-    bits = DW_UNSND (attr) * TARGET_CHAR_BIT;
+    bits = attr->constant_value (0) * TARGET_CHAR_BIT;
   name = dwarf2_name (die, cu);
   if (!name)
     complaint (_("DW_AT_name missing from DW_TAG_base_type"));
@@ -16827,9 +16823,9 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
   enum bfd_endian byte_order = gdbarch_byte_order (arch);
 
   attr = dwarf2_attr (die, DW_AT_endianity, cu);
-  if (attr)
+  if (attr != nullptr && attr->form_is_constant ())
     {
-      int endianity = DW_UNSND (attr);
+      int endianity = attr->constant_value (0);
 
       switch (endianity)
 	{
@@ -17285,7 +17281,7 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
 
   attr = dwarf2_attr (die, DW_AT_byte_size, cu);
   if (attr != nullptr)
-    TYPE_LENGTH (range_type) = DW_UNSND (attr);
+    TYPE_LENGTH (range_type) = attr->constant_value (0);
 
   maybe_set_alignment (cu, die, range_type);
 
@@ -17469,8 +17465,8 @@ read_full_die_1 (const struct die_reader_specs *reader,
     }
 
   struct attribute *attr = die->attr (DW_AT_str_offsets_base);
-  if (attr != nullptr)
-    cu->str_offsets_base = DW_UNSND (attr);
+  if (attr != nullptr && attr->form_is_unsigned ())
+    cu->str_offsets_base = attr->get_unsigned ();
 
   auto maybe_addr_base = die->addr_base ();
   if (maybe_addr_base.has_value ())
@@ -17962,14 +17958,17 @@ partial_die_info::read (const struct die_reader_specs *reader,
 	     Although DWARF now specifies a way to provide this
 	     information, we support this practice for backward
 	     compatibility.  */
-	  if (DW_UNSND (&attr) == DW_CC_program
+	  if (attr.constant_value (0) == DW_CC_program
 	      && cu->language == language_fortran)
 	    main_subprogram = 1;
 	  break;
 	case DW_AT_inline:
-	  if (DW_UNSND (&attr) == DW_INL_inlined
-	      || DW_UNSND (&attr) == DW_INL_declared_inlined)
-	    may_be_inlined = 1;
+	  {
+	    LONGEST value = attr.constant_value (-1);
+	    if (value == DW_INL_inlined
+		|| value == DW_INL_declared_inlined)
+	      may_be_inlined = 1;
+	  }
 	  break;
 
 	case DW_AT_import:
@@ -17991,7 +17990,7 @@ partial_die_info::read (const struct die_reader_specs *reader,
 	       but that requires a full DIE, so instead we just
 	       reimplement it.  */
 	    int need_ranges_base = tag != DW_TAG_compile_unit;
-	    unsigned int ranges_offset = (DW_UNSND (&attr)
+	    unsigned int ranges_offset = (attr.constant_value (0)
 					  + (need_ranges_base
 					     ? cu->ranges_base
 					     : 0));
@@ -18569,11 +18568,11 @@ read_attribute_value (const struct die_reader_specs *reader,
      treat them as zero by default.  */
   if (attr->name == DW_AT_byte_size
       && form == DW_FORM_data4
-      && DW_UNSND (attr) >= 0xffffffff)
+      && attr->get_unsigned () >= 0xffffffff)
     {
       complaint
         (_("Suspicious DW_AT_byte_size value treated as zero instead of %s"),
-         hex_string (DW_UNSND (attr)));
+         hex_string (attr->get_unsigned ()));
       attr->set_unsigned (0);
     }
 
@@ -20019,16 +20018,15 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 			  inlined_func ? DW_AT_call_line : DW_AT_decl_line,
 			  cu);
       if (attr != nullptr)
-	{
-	  SYMBOL_LINE (sym) = DW_UNSND (attr);
-	}
+	SYMBOL_LINE (sym) = attr->constant_value (0);
 
       attr = dwarf2_attr (die,
 			  inlined_func ? DW_AT_call_file : DW_AT_decl_file,
 			  cu);
-      if (attr != nullptr)
+      if (attr != nullptr && attr->form_is_unsigned ())
 	{
-	  file_name_index file_index = (file_name_index) DW_UNSND (attr);
+	  file_name_index file_index
+	    = (file_name_index) attr->get_unsigned ();
 	  struct file_entry *fe;
 
 	  if (cu->line_header != NULL)
@@ -20378,7 +20376,7 @@ dwarf2_const_value_data (const struct attribute *attr, struct obstack *obstack,
   struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
   enum bfd_endian byte_order = bfd_big_endian (objfile->obfd) ?
 				BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE;
-  LONGEST l = DW_UNSND (attr);
+  LONGEST l = attr->constant_value (0);
 
   if (bits < sizeof (*value) * 8)
     {
@@ -20495,7 +20493,7 @@ dwarf2_const_value_attr (const struct attribute *attr, struct type *type,
       break;
 
     case DW_FORM_udata:
-      *value = DW_UNSND (attr);
+      *value = attr->get_unsigned ();
       break;
 
     default:
@@ -21337,11 +21335,11 @@ dump_die_shallow (struct ui_file *f, int indent, struct die_info *die)
 	  break;
 	case DW_FORM_ref_addr:
 	  fprintf_unfiltered (f, "ref address: ");
-	  fputs_filtered (hex_string (DW_UNSND (&die->attrs[i])), f);
+	  fputs_filtered (hex_string (die->attrs[i].get_unsigned ()), f);
 	  break;
 	case DW_FORM_GNU_ref_alt:
 	  fprintf_unfiltered (f, "alt ref address: ");
-	  fputs_filtered (hex_string (DW_UNSND (&die->attrs[i])), f);
+	  fputs_filtered (hex_string (die->attrs[i].get_unsigned ()), f);
 	  break;
 	case DW_FORM_ref1:
 	case DW_FORM_ref2:
@@ -21349,7 +21347,7 @@ dump_die_shallow (struct ui_file *f, int indent, struct die_info *die)
 	case DW_FORM_ref8:
 	case DW_FORM_ref_udata:
 	  fprintf_unfiltered (f, "constant ref: 0x%lx (adjusted)",
-			      (long) (DW_UNSND (&die->attrs[i])));
+			      (long) (die->attrs[i].get_unsigned ()));
 	  break;
 	case DW_FORM_data1:
 	case DW_FORM_data2:
@@ -21357,11 +21355,11 @@ dump_die_shallow (struct ui_file *f, int indent, struct die_info *die)
 	case DW_FORM_data8:
 	case DW_FORM_udata:
 	  fprintf_unfiltered (f, "constant: %s",
-			      pulongest (DW_UNSND (&die->attrs[i])));
+			      pulongest (die->attrs[i].get_unsigned ()));
 	  break;
 	case DW_FORM_sec_offset:
 	  fprintf_unfiltered (f, "section offset: %s",
-			      pulongest (DW_UNSND (&die->attrs[i])));
+			      pulongest (die->attrs[i].get_unsigned ()));
 	  break;
 	case DW_FORM_ref_sig8:
 	  fprintf_unfiltered (f, "signature: %s",
@@ -21828,7 +21826,7 @@ dwarf2_fetch_constant_bytes (sect_offset sect_off,
     case DW_FORM_udata:
       type = die_type (die, cu);
       result = write_constant_as_bytes (obstack, byte_order,
-					type, DW_UNSND (attr), len);
+					type, attr->get_unsigned (), len);
       break;
 
     default:
@@ -22839,7 +22837,7 @@ prepare_one_comp_unit (struct dwarf2_cu *cu, struct die_info *comp_unit_die,
   /* Set the language we're debugging.  */
   attr = dwarf2_attr (comp_unit_die, DW_AT_language, cu);
   if (attr != nullptr)
-    set_cu_language (DW_UNSND (attr), cu);
+    set_cu_language (attr->constant_value (0), cu);
   else
     {
       cu->language = pretend_language;
-- 
2.17.2