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

Joel Brobecker
2020-11-14T10:48:40Z

[-- 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