Re: [PATCH 5/9] Add support for printing value of DWARF-based fixed-point type objects

[Joel Brobecker <brobecker@adacore.com>]

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

Hi Simon,

> > +   of the corresponding TYPE by setting its TYPE_FIXED_POINT_INFO.
> > +   CU is the DIE's CU.  */
> > +
> > +static void
> > +finish_fixed_point_type (struct type *type, struct die_info *die,
> > +			 struct dwarf2_cu *cu)
> 
> Unless there's a good reason not to (coming up in the following
> patches), I would make this function create the "struct type", instead
> of having the caller create it and pass it.  In other words, this
> signature:
> 
> struct type *create_fixed_point_type (die_info *die, dwarf2_cu *cu)
> 
> That just makes it more obvious that it's a simple "die" to "type"
> transform.

While implementing this suggestion, I'm realizing that doing so
requires either:

(1) Recomputing the following information for the CU: encoding,
    bitsize if any, and type name, which means:

        | attr = dwarf2_attr (die, DW_AT_encoding, cu);
        | 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 = attr->constant_value (0) * TARGET_CHAR_BIT;
        | name = dwarf2_name (die, cu);
        | if (!name)
        |   complaint (_("DW_AT_name missing from DW_TAG_base_type"));

    Or:

(2) Passing that information as arguments to the function.

I find option (1) really sad because of the code duplication.
And I find option (2) somewhat unsatisfactory, because we then
run the risk of passing inconsistent information.

This might explain partly why the init + finish approach isn't
new in this unit...

I'm attaching the patch which shows how option (1) looks like.
For me, the current approach strikes a better balance between
API cleanliness and implementation considerations. But I don't
have a strong opinion against implementing what you suggest,
including through option (2) (or other options I missed).

Let me know what you think.

-- 
Joel

[-- Attachment #2: 0001-make-finish_fixed_point_type-create-the-type-renamin.patch --]
[-- Type: text/x-diff, Size: 2750 bytes --]

From 2e76b9ca55aa93189d22d649c8698ab8d572ed7a Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Sat, 14 Nov 2020 05:43:33 -0500
Subject: [PATCH] make finish_fixed_point_type create the type (renaming it
 accordingly)

Change-Id: Ic1b81f5e8328617627675a609ede8320edd749c0
---
 gdb/dwarf2/read.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 4a447d4..6ba24b3 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -18221,14 +18221,17 @@ get_dwarf2_unsigned_rational_constant (struct die_info *die,
    of the corresponding TYPE by setting its TYPE_FIXED_POINT_INFO.
    CU is the DIE's CU.  */
 
-static void
-finish_fixed_point_type (struct type *type, struct die_info *die,
-			 struct dwarf2_cu *cu)
+static struct type *
+read_fixed_point_type (struct die_info *die, struct dwarf2_cu *cu)
 {
   struct attribute *attr;
+  int encoding = 0;
 
-  gdb_assert (type->code () == TYPE_CODE_FIXED_POINT
-	      && TYPE_SPECIFIC_FIELD (type) == TYPE_SPECIFIC_FIXED_POINT);
+  attr = dwarf2_attr (die, DW_AT_encoding, cu);
+  if (attr != nullptr && attr->form_is_constant ())
+    encoding = attr->constant_value (0);
+  gdb_assert (encoding == DW_ATE_signed_fixed
+	      || encoding == DW_ATE_unsigned_fixed);
 
   attr = dwarf2_attr (die, DW_AT_binary_scale, cu);
   if (!attr)
@@ -18289,10 +18292,25 @@ finish_fixed_point_type (struct type *type, struct die_info *die,
 		 sect_offset_str (die->sect_off));
     }
 
+  int bits = 0;
+
+  attr = dwarf2_attr (die, DW_AT_byte_size, cu);
+  if (attr != nullptr)
+    bits = attr->constant_value (0) * TARGET_CHAR_BIT;
+  const char *name = dwarf2_name (die, cu);
+  if (!name)
+    complaint (_("DW_AT_name missing from DW_TAG_base_type"));
+
+  struct type *type = init_fixed_point_type
+    (cu->per_objfile->objfile, bits, encoding == DW_ATE_unsigned_fixed,
+     name);
+
   gdb_mpq &scaling_factor = TYPE_FIXED_POINT_INFO (type)->scaling_factor;
   mpz_set (mpq_numref (scaling_factor.val), scale_num.val);
   mpz_set (mpq_denref (scaling_factor.val), scale_denom.val);
   mpq_canonicalize (scaling_factor.val);
+
+  return type;
 }
 
 /* Allocate a floating-point type of size BITS and name NAME.  Pass NAME_HINT
@@ -18574,12 +18592,8 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
 	}
 	break;
       case DW_ATE_signed_fixed:
-	type = init_fixed_point_type (objfile, bits, 0, name);
-	finish_fixed_point_type (type, die, cu);
-	break;
       case DW_ATE_unsigned_fixed:
-	type = init_fixed_point_type (objfile, bits, 1, name);
-	finish_fixed_point_type (type, die, cu);
+	type = read_fixed_point_type (die, cu);
 	break;
 
       default:
-- 
2.1.4