diff mbox series

dwarf-reader: get subrange_type bounds signedness from underlying type

Message ID 87h7ql9m7g.fsf@redhat.com
State New
Headers
Series dwarf-reader: get subrange_type bounds signedness from underlying type |

Commit Message

Dodji Seketeli Oct. 23, 2020, 12:10 p.m. UTC 
  Hello Mark,

I have taken the patch you submitted at
https://sourceware.org/bugzilla/show_bug.cgi?id=26684#c10 and put it
together below for.

Thank you for taking the time to work on this.

You might want to review and sign it off so that it can be applied to
master.

Thanks!

From 906813b19eb44a6a4fc2e6378ecfb8543926b0b4 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Fri, 23 Oct 2020 11:14:38 +0200
Subject: [PATCH] dwarf-reader: get subrange_type bounds signedness from underlying type

This patch was kindly submitted by Mark Wielaard in a comment of a
problem report at https://sourceware.org/bugzilla/show_bug.cgi?id=26684#c10.

I am merely mechanically putting it together as a commit here.

When reading the bounds of a subrange_type, we need to know if the
constant value of those bounds is signed or not.

For that, we used to just look at the form of those constant and infer
the signedness from there.

The problem is that the signedness is really determined by the value
of the DW_AT_encoding attribute present on the underlying type of the
subrange_type; the form of the bound is mere implementation detail
that is downstream of the information of carried by the DW_AT_encoding
attribute.

This patch thus does the right thing by looking at the underlying type
of the subrange_type and by doing away with the unnecessary messing
with attribute value forms.

	* src/abg-dwarf-reader.cc (die_attribute_has_form)
	(die_attribute_is_signed, die_attribute_is_unsigned)
	(die_attribute_has_no_signedness): Remove static functions.
	(die_constant_attribute): Add the 'is_signed' parameter.
	(die_address_attribute): Adjust comment.
	(build_subrange_type): Determine signedness of the bounds by
	looking at the DW_AT_encoding attribute of the underlying type.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 src/abg-dwarf-reader.cc | 137 ++++++++++------------------------------
 1 file changed, 33 insertions(+), 104 deletions(-)

Comments

Mark Wielaard Oct. 23, 2020, 7:03 p.m. UTC | #1 
Hi Dodji,

On Fri, Oct 23, 2020 at 02:10:59PM +0200, Dodji Seketeli wrote:
> I have taken the patch you submitted at
> https://sourceware.org/bugzilla/show_bug.cgi?id=26684#c10 and put it
> together below for.
> 
> Thank you for taking the time to work on this.
> 
> You might want to review and sign it off so that it can be applied to
> master.

Thansk, I had forgotten about this cleanup.

> When reading the bounds of a subrange_type, we need to know if the
> constant value of those bounds is signed or not.
> 
> For that, we used to just look at the form of those constant and infer
> the signedness from there.
> 
> The problem is that the signedness is really determined by the value
> of the DW_AT_encoding attribute present on the underlying type of the
> subrange_type; the form of the bound is mere implementation detail
> that is downstream of the information of carried by the DW_AT_encoding
> attribute.
> 
> This patch thus does the right thing by looking at the underlying type
> of the subrange_type and by doing away with the unnecessary messing
> with attribute value forms.
> 
> 	* src/abg-dwarf-reader.cc (die_attribute_has_form)
> 	(die_attribute_is_signed, die_attribute_is_unsigned)
> 	(die_attribute_has_no_signedness): Remove static functions.
> 	(die_constant_attribute): Add the 'is_signed' parameter.
> 	(die_address_attribute): Adjust comment.
> 	(build_subrange_type): Determine signedness of the bounds by
> 	looking at the DW_AT_encoding attribute of the underlying type.
> 
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>

Looks good.

Signed-off-by: Mark Wielaard <mark@klomp.org>

Cheers,

Mark
Dodji Seketeli Oct. 27, 2020, 9:32 a.m. UTC | #2 
Hello Mark,

> On Fri, Oct 23, 2020 at 02:10:59PM +0200, Dodji Seketeli wrote:

[...]

>> 	* src/abg-dwarf-reader.cc (die_attribute_has_form)
>> 	(die_attribute_is_signed, die_attribute_is_unsigned)
>> 	(die_attribute_has_no_signedness): Remove static functions.
>> 	(die_constant_attribute): Add the 'is_signed' parameter.
>> 	(die_address_attribute): Adjust comment.
>> 	(build_subrange_type): Determine signedness of the bounds by
>> 	looking at the DW_AT_encoding attribute of the underlying type.

[...]

Mark Wielaard <mark@klomp.org> writes:

> Looks good.
>
> Signed-off-by: Mark Wielaard <mark@klomp.org>

Thanks!

Applied to master.

Cheers,
diff mbox series

Patch

diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index 1d4e5141..9b42f4ef 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -392,25 +392,12 @@  die_signed_constant_attribute(const Dwarf_Die*die,
 static bool
 die_constant_attribute(const Dwarf_Die *die,
 		       unsigned attr_name,
+		       bool is_signed,
 		       array_type_def::subrange_type::bound_value &value);
 
-static bool
-die_attribute_has_form(const Dwarf_Die* die,
-		       unsigned	attr_name,
-		       unsigned int	form);
-
 static bool
 form_is_DW_FORM_strx(unsigned form);
 
-static bool
-die_attribute_is_signed(const Dwarf_Die* die, unsigned attr_name);
-
-static bool
-die_attribute_is_unsigned(const Dwarf_Die* die, unsigned attr_name);
-
-static bool
-die_attribute_has_no_signedness(const Dwarf_Die* die, unsigned attr_name);
-
 static bool
 die_address_attribute(Dwarf_Die* die, unsigned attr_name, Dwarf_Addr& result);
 
@@ -8111,7 +8098,7 @@  die_unsigned_constant_attribute(const Dwarf_Die*	die,
 
 /// Read a signed constant value from a given attribute.
 ///
-/// The signed constant expected must be of form DW_FORM_sdata.
+/// The signed constant expected must be of constant form.
 ///
 /// @param die the DIE to get the attribute from.
 ///
@@ -8149,6 +8136,9 @@  die_signed_constant_attribute(const Dwarf_Die *die,
 ///
 /// @param attr_name the attribute name to consider.
 ///
+/// @param is_signed true if the attribute value has to read as
+/// signed.
+///
 /// @param value the resulting value read from attribute @p attr_name
 /// on DIE @p die.
 ///
@@ -8157,10 +8147,10 @@  die_signed_constant_attribute(const Dwarf_Die *die,
 static bool
 die_constant_attribute(const Dwarf_Die *die,
 		       unsigned attr_name,
+		       bool is_signed,
 		       array_type_def::subrange_type::bound_value &value)
 {
-  if (die_attribute_is_unsigned(die, attr_name)
-      || die_attribute_has_no_signedness(die, attr_name))
+  if (!is_signed)
     {
       uint64_t l = 0;
       if (!die_unsigned_constant_attribute(die, attr_name, l))
@@ -8177,29 +8167,6 @@  die_constant_attribute(const Dwarf_Die *die,
   return true;
 }
 
-/// Test if a given attribute on a DIE has a particular form.
-///
-/// @param die the DIE to consider.
-///
-/// @param attr_name the attribute name to consider on DIE @p die.
-///
-/// @param attr_form the attribute form that we expect attribute @p
-/// attr_name has on DIE @p die.
-///
-/// @return true iff the attribute named @p attr_name on DIE @p die
-/// has the form @p attr_form.
-static bool
-die_attribute_has_form(const Dwarf_Die	*die,
-		       unsigned	attr_name,
-		       unsigned int	attr_form)
-{
-  Dwarf_Attribute attr;
-  if (!dwarf_attr_integrate(const_cast<Dwarf_Die*>(die), attr_name, &attr))
-    return false;
-
-  return dwarf_hasform(&attr, attr_form);
-}
-
 /// Test if a given DWARF form is DW_FORM_strx{1,4}.
 ///
 /// Unfortunaly, the DW_FORM_strx{1,4} are enumerators of an untagged
@@ -8228,55 +8195,6 @@  form_is_DW_FORM_strx(unsigned form)
   return false;
 }
 
-/// Test if a given DIE attribute is signed.
-///
-/// @param die the DIE to consider.
-///
-/// @param attr_name the attribute name to consider.
-///
-/// @return true iff the attribute named @p attr_name on DIE @p die is
-/// signed.
-static bool
-die_attribute_is_signed(const Dwarf_Die* die, unsigned attr_name)
-{
-  if (die_attribute_has_form(die, attr_name, DW_FORM_sdata))
-    return true;
-  return false;
-}
-
-/// Test if a given DIE attribute is unsigned.
-///
-/// @param die the DIE to consider.
-///
-/// @param attr_name the attribute name to consider.
-///
-/// @return true iff the attribute named @p attr_name on DIE @p die is
-/// unsigned.
-static bool
-die_attribute_is_unsigned(const Dwarf_Die* die, unsigned attr_name)
-{
-  if (die_attribute_has_form(die, attr_name, DW_FORM_udata))
-    return true;
-  return false;
-}
-
-/// Test if a given DIE attribute is neither explicitely signed nor
-/// unsigned.  Usually this is the case for attribute of the form
-/// DW_FORM_data*.
-///
-/// @param die the DIE to consider.
-///
-/// @param attr_name the name of the attribute to consider.
-///
-/// @return true iff the attribute named @p attr_name of DIE @p die is
-/// neither specifically signed nor unsigned.
-static bool
-die_attribute_has_no_signedness(const Dwarf_Die *die, unsigned attr_name)
-{
-  return (!die_attribute_is_unsigned(die, attr_name)
-	  && !die_attribute_is_signed(die, attr_name));
-}
-
 /// Get the value of a DIE attribute; that value is meant to be a
 /// flag.
 ///
@@ -8387,17 +8305,17 @@  die_die_attribute(const Dwarf_Die* die,
   return dwarf_formref_die(&attr, &result);
 }
 
-/// Read and return a DW_FORM_addr attribute from a given DIE.
+/// Read and return an addresss class attribute from a given DIE.
 ///
 /// @param die the DIE to consider.
 ///
-/// @param attr_name the name of the DW_FORM_addr attribute to read
+/// @param attr_name the name of the address class attribute to read
 /// the value from.
 ///
 /// @param the resulting address.
 ///
 /// @return true iff the attribute could be read, was of the expected
-/// DW_FORM_addr and could thus be translated into the @p result.
+/// address class and could thus be translated into the @p result.
 static bool
 die_address_attribute(Dwarf_Die* die, unsigned attr_name, Dwarf_Addr& result)
 {
@@ -14974,6 +14892,27 @@  build_subrange_type(read_context&	ctxt,
 
   string name = die_name(die);
 
+  // load the underlying type.
+  Dwarf_Die underlying_type_die;
+  type_base_sptr underlying_type;
+  /* Unless there is an underlying type which says differently.  */
+  bool is_signed = true;
+  if (die_die_attribute(die, DW_AT_type, underlying_type_die))
+    underlying_type =
+      is_type(build_ir_node_from_die(ctxt,
+				     &underlying_type_die,
+				     /*called_from_public_decl=*/true,
+				     where_offset));
+
+  if (underlying_type)
+    {
+      uint64_t ate;
+      if (die_unsigned_constant_attribute (&underlying_type_die,
+					   DW_AT_encoding,
+					   ate))
+	  is_signed = (ate == DW_ATE_signed || ate == DW_ATE_signed_char);
+    }
+
   translation_unit::language language = ctxt.cur_transl_unit()->get_language();
   array_type_def::subrange_type::bound_value lower_bound =
     get_default_array_lower_bound(language);
@@ -14990,10 +14929,10 @@  build_subrange_type(read_context&	ctxt,
   //     values of the subrange.
   //
   // So let's look for DW_AT_lower_bound first.
-  die_constant_attribute(die, DW_AT_lower_bound, lower_bound);
+  die_constant_attribute(die, DW_AT_lower_bound, is_signed, lower_bound);
 
   // Then, DW_AT_upper_bound.
-  if (!die_constant_attribute(die, DW_AT_upper_bound, upper_bound))
+  if (!die_constant_attribute(die, DW_AT_upper_bound, is_signed, upper_bound))
     {
       // The DWARF 4 spec says, in [5.11 Subrange Type
       // Entries]:
@@ -15037,16 +14976,6 @@  build_subrange_type(read_context&	ctxt,
 				       location()));
   result->is_infinite(is_infinite);
 
-  // load the underlying type.
-  Dwarf_Die underlying_type_die;
-  type_base_sptr underlying_type;
-  if (die_die_attribute(die, DW_AT_type, underlying_type_die))
-    underlying_type =
-      is_type(build_ir_node_from_die(ctxt,
-				     &underlying_type_die,
-				     /*called_from_public_decl=*/true,
-				     where_offset));
-
   if (underlying_type)
     result->set_underlying_type(underlying_type);