[11/20] Add reprocessing flag to struct attribute

Message ID 20200328192208.11324-12-tom@tromey.com
State Superseded
Headers show
Series
  • Make DWARF attribute references safe
Related show

Commit Message

Tom Tromey March 28, 2020, 7:21 p.m.
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(-)

-- 
2.17.2

Comments

Simon Marchi March 30, 2020, 3:32 p.m. | #1
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
Tom Tromey April 4, 2020, 2:02 p.m. | #2
>>>>> "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

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