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

[Simon Marchi <simark@simark.ca>]

[Sorry Joel, I failed to reply-all, so I am sending it again including
 gdb-patches]

I did not do an in-depth review of the functionality, because I don't
have that much time right now (and coming from AdaCore I trust that it's
quite well tested), so the comments are mostly coding-style related.

> This commit introduces a new kind of type, meant to describe
> fixed-point types, using a new code added specifically for
> this purpose (TYPE_CODE_FIXED_POINT).
>
> It then adds handling of fixed-point base types in the DWARF reader.
>
> And finally, as a first step, this commit adds support for printing
> the value of fixed-point type objects.
>
> Note that this commit has a known issue: Trying to print the value
> of a fixed-point object with a format letter (e.g. "print /x NAME")
> causes the wrong value to be printed because the scaling factor
> is not applied. Since the fix for this issue is isolated, and
> this is not a regression, the fix will be made in a pach of its own.
> This is meant to simplify review and archeology.
>
> Also, other functionalities related to fixed-point type handling
> (ptype, arithmetics, etc), will be added piecemeal as well, for
> the same reasons (faciliate reviews and archeology). Related to this,
> the testcase gdb.ada/fixed_cmp.exp is adjusted to compile the test
> program with -fgnat-encodings=all, so as to force the use of GNAT
> encodings, rather than rely on the compiler's default to use them.
> The intent is to enhance this testcase to also test the pure DWARF
> approach using -fgnat-encodings=minimal as soon as the corresponding
> suport gets added in. Thus, the modification to the testcase is made
> in a way that it prepares this testcase to be tested in both modes.

Since it's an easy change to do, I'd suggest to change NULL for nullptr
in all the new code.  Also, declare variable on first use when possible.

> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index dbf0a3e..1f5152d 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -18130,6 +18130,157 @@ read_typedef (struct die_info *die, struct dwarf2_cu *cu)
>    return this_type;
>  }
>
> +/* Assuming DIE is a rational DW_TAG_constant, read the DIE's
> +   numerator and denominator into NUMERATOR and DENOMINATOR (resp).
> +
> +   If the numerator and/or numerator attribute is missing,
> +   a complaint is filed, and NUMERATOR and DENOMINATOR are left
> +   untouched.  */
> +
> +static void
> +get_dwarf2_rational_constant (struct die_info *die, struct dwarf2_cu *cu,
> +			      LONGEST *numerator, LONGEST *denominator)
> +{
> +  struct attribute *num_attr, *denom_attr;
> +
> +  num_attr = dwarf2_attr (die, DW_AT_GNU_numerator, cu);
> +  if (num_attr == NULL)
> +    complaint (_("DW_AT_GNU_numerator missing in %s DIE at %s"),
> +	       dwarf_tag_name (die->tag), sect_offset_str (die->sect_off));
> +
> +  denom_attr = dwarf2_attr (die, DW_AT_GNU_denominator, cu);
> +  if (denom_attr == NULL)
> +    complaint (_("DW_AT_GNU_denominator missing in %s DIE at %s"),
> +	       dwarf_tag_name (die->tag), sect_offset_str (die->sect_off));
> +
> +  if (num_attr == NULL || denom_attr == NULL)
> +    return;
> +
> +  *numerator = num_attr->constant_value (1);
> +  *denominator = denom_attr->constant_value (1);
> +}
> +
> +/* Same as get_dwarf2_rational_constant, but extracting an unsigned
> +   rational constant, rather than a signed one.
> +
> +   If the rational constant is has a negative value, a complaint

"is has"

> +   is filed, and NUMERATOR and DENOMINATOR are left untouched.  */
> +
> +static void
> +get_dwarf2_unsigned_rational_constant (struct die_info *die,
> +				       struct dwarf2_cu *cu,
> +				       ULONGEST *numerator,
> +				       ULONGEST *denominator)
> +{
> +  LONGEST num = 1, denom = 1;
> +
> +  get_dwarf2_rational_constant (die, cu, &num, &denom);
> +  if (num < 0 && denom < 0)
> +    {
> +      num = -num;
> +      denom = -denom;
> +    }
> +  else if (num < 0)
> +    {
> +      complaint (_("unexpected negative value for DW_AT_GNU_numerator"
> +		   " in DIE at %s"),
> +		 sect_offset_str (die->sect_off));
> +      return;
> +    }
> +  else if (denom < 0)
> +    {
> +      complaint (_("unexpected negative value for DW_AT_GNU_denominator"
> +		   " in DIE at %s"),
> +		 sect_offset_str (die->sect_off));
> +      return;
> +    }
> +
> +  *numerator = num;
> +  *denominator = denom;
> +}
> +
> +/* Assiuming DIE corresponds to a fixed point type, finish the creation

"Assiuming"

> +   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.

> +{
> +  struct attribute *attr;
> +  /* Numerator and denominator of our fixed-point type's scaling factor.
> +     The default is a scaling factor of 1, which we use as a fallback
> +     when we are not able to decode it (problem with the debugging info,
> +     unsupported forms, bug in GDB, etc...).  Using that as the default
> +     allows us to at least print the unscaled value, which might still
> +     be useful to a user.  */
> +  ULONGEST scale_num = 1;
> +  ULONGEST scale_denom = 1;
> +
> +  gdb_assert (type->code () == TYPE_CODE_FIXED_POINT
> +	      && TYPE_SPECIFIC_FIELD (type) == TYPE_SPECIFIC_FIXED_POINT);
> +
> +  attr = dwarf2_attr (die, DW_AT_binary_scale, cu);
> +  if (!attr)
> +    attr = dwarf2_attr (die, DW_AT_decimal_scale, cu);
> +  if (!attr)
> +    attr = dwarf2_attr (die, DW_AT_small, cu);
> +
> +  if (attr == NULL)
> +    {
> +      /* Scaling factor not found.  Assume a scaling factor of 1,
> +	 and hope for the best.  At least the user will be able to see
> +	 the encoded value.  */
> +      complaint (_("no scale found for fixed-point type (DIE at %s)"),
> +		 sect_offset_str (die->sect_off));
> +    }
> +  else if (attr->name == DW_AT_binary_scale)
> +    {
> +      LONGEST scale_exp = attr->constant_value (0);
> +      ULONGEST *num_or_denom = scale_exp > 0 ? &scale_num : &scale_denom;
> +
> +      *num_or_denom = 1 << abs (scale_exp);
> +    }
> +  else if (attr->name == DW_AT_decimal_scale)
> +    {
> +      LONGEST scale_exp = attr->constant_value (0);
> +      ULONGEST *num_or_denom = scale_exp > 0 ? &scale_num : &scale_denom;
> +
> +      *num_or_denom = uinteger_pow (10, abs (scale_exp));
> +    }
> +  else if (attr->name == DW_AT_small)
> +    {
> +      struct die_info *scale_die;
> +      struct dwarf2_cu *scale_cu = cu;
> +
> +      scale_die = follow_die_ref (die, attr, &scale_cu);
> +      if (scale_die->tag == DW_TAG_constant)
> +	get_dwarf2_unsigned_rational_constant (scale_die, scale_cu,
> +					       &scale_num, &scale_denom);
> +      else
> +	complaint (_("%s DIE not supported as target of DW_AT_small attribute"
> +		     " (DIE at %s)"),
> +		   dwarf_tag_name (die->tag), sect_offset_str (die->sect_off));
> +    }
> +  else
> +    {
> +      complaint (("unsupported scale attribute %s for fixed-point type"
> +		   " (DIE at %s)"),

Missing _()?  Also, the second line has one space too much.

> @@ -1821,6 +1845,9 @@ extern void set_type_vptr_basetype (struct type *, struct type *);
>    (TYPE_CPLUS_SPECIFIC(thistype)->virtual_field_bits == NULL ? 0 \
>      : B_TST(TYPE_CPLUS_SPECIFIC(thistype)->virtual_field_bits, (index)))
>
> +#define TYPE_FIXED_POINT_INFO(thistype) \
> +  (TYPE_MAIN_TYPE(thistype)->type_specific.fixed_point_info)

Do you think you can make this macro a method on struct type in the
style of the ones that were added recently?  It will be one less macro
to convert later :).  That method should contain an assert that the type
code is indeed TYPE_CODE_FIXED_POINT.  I think it could return a
`fixed_point_info &`, since there's no way it can fail and need to
return nullptr (now I see that type::bounds should probably return a
`range_bounds &` instead of a `range_bounds *`).

> +
>  #define FIELD_NAME(thisfld) ((thisfld).name)
>  #define FIELD_LOC_KIND(thisfld) ((thisfld).loc_kind)
>  #define FIELD_BITPOS_LVAL(thisfld) ((thisfld).loc.bitpos)
> @@ -2192,6 +2219,8 @@ extern struct type *init_decfloat_type (struct objfile *, int, const char *);
>  extern struct type *init_complex_type (const char *, struct type *);
>  extern struct type *init_pointer_type (struct objfile *, int, const char *,
>  				       struct type *);
> +extern struct type *init_fixed_point_type (struct objfile *, int, int,
> +					   const char *);
>
>  /* Helper functions to construct architecture-owned types.  */
>  extern struct type *arch_type (struct gdbarch *, enum type_code, int,
> @@ -2529,6 +2558,26 @@ extern int type_not_allocated (const struct type *type);
>
>  extern int type_not_associated (const struct type *type);
>
> +/* Return True if TYPE is a TYPE_CODE_FIXED_POINT or if TYPE is
> +   range whose base type is a TYPE_CODE_FIXED_POINT.  */
> +extern int is_fixed_point_type (struct type *type);

int -> bool

> +
> +/* Assuming that TYPE is a fixed point type, return its base type.
> +
> +   In other words, this returns the type after having peeled all
> +   intermediate type layers (such as TYPE_CODE_RANGE, for instance).
> +   The TYPE_CODE of the type returned is guaranteed to be
> +   a TYPE_CODE_FIXED_POINT.  */
> +extern struct type *fixed_point_type_base_type (struct type *type);

I think it would make sense to make this a `struct type` method.

> +
> +/* Given TYPE, which is a fixed point type, return its scaling factor.  */
> +extern const gdb_mpq &fixed_point_scaling_factor (struct type *type);

I think this would belong in fixed_point_type_info, so you'd access it
using:

  type->fixed_point_info ().scaling_factor ()

> diff --git a/gdb/testsuite/gdb.ada/fixed_cmp.exp b/gdb/testsuite/gdb.ada/fixed_cmp.exp
> index 38e41c4..10e2c9a 100644
> --- a/gdb/testsuite/gdb.ada/fixed_cmp.exp
> +++ b/gdb/testsuite/gdb.ada/fixed_cmp.exp
> @@ -19,25 +19,29 @@ if { [skip_ada_tests] } { return -1 }
>
>  standard_ada_testfile fixed
>
> -if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug ]] != "" } {
> -  return -1
> -}
> +foreach_with_prefix scenario {all} {

I suggest naming this variable gnat-encodings, just because the
resulting test name will be a bit clearer.

Simon