[06/20] Remove DW_STRING and DW_STRING_IS_CANONICAL

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

Commit Message

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

-- 
2.17.2

Comments

Simon Marchi March 30, 2020, 3:10 p.m. | #1
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
Tom Tromey March 31, 2020, 12:23 a.m. | #2
>>>>> "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

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