[15/20] Add attribute::get_unsigned method

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

Commit Message

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

-- 
2.17.2

Comments

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

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;