Re: [patch] add support for debugging fixed-point numbers

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Sean" == Sean D'Epagnier <geckosenator@gmail.com> writes:

Sean> Now I am able to debug applications with fixed point variables, and
Sean> all the usual features work, but I had to patch gcc also to provide
Sean> the right info in the dwarf output so that gdb knows where the decimal
Sean> place is (patch also attached)

Thanks for the patch.

Sean> Let me know if there is anything else I need to do to get this applied.

A patch this size will require a copyright assignment to the FSF.
One of the gdb maintainers should be able to send you the paperwork to
get you started.

I read through the patch.  There are a few things I did not
understand, comments below.  Also, there are some formatting nits.

In addition, patches need a ChangeLog entry (you'll want this for GCC
as well).  The GNU coding standards describe how to write one.

Some test cases would be nice.

Looking at GCC's extend.texi, I see there is new syntax to write
fixed-point literal constants, and some new keywords.  It would be
nice to have support for these in gdb as well -- but IMO, this is not
a requirement for this patch and if you want you can just file it as a
feature request in bugzilla.

Sean> +    case TYPE_CODE_FIXED:
Sean> +       /* convert to double and print that */

Style nit: GNU-style comments are full sentences, so start with a
capital and end with a period and two spaces.  There are a few
instances of this.

Sean> +  if (code == TYPE_CODE_FIXED) {

Braces go on their own lines in the GNU style.  There are a few
instances of this.

Sean> +    /* First extension operator.  Individual language modules define
Sean> +       extra operators they need as constants with values 
Sean> +       OP_LANGUAGE_SPECIFIC0 + k, for k >= 0, using a separate 
Sean> +       enumerated type definition:
Sean> +       enum foo_extension_operator {
Sean> +       BINOP_MOGRIFY = OP_EXTENDED0,
Sean> +       BINOP_FROB,
Sean> +       ...
Sean> +       };      */
Sean>      OP_EXTENDED0,

It appears that this comment was inadvertently reformatted.

Sean> +      /* convert fixed to double */
Sean> +      if(TYPE_CODE (type1) == TYPE_CODE_FIXED)
Sean> +         arg1 = value_cast(builtin_type_ieee_double, arg1);
Sean> +      if(TYPE_CODE (type2) == TYPE_CODE_FIXED)
Sean> +         arg2 = value_cast(builtin_type_ieee_double, arg2);

The GNU style puts spaces before parens.  There are a few instances of
this.

Sean> +  else if (TYPE_CODE (type) == TYPE_CODE_FIXED)
Sean> +    {
Sean> +      LONGEST unshiftedval = -extract_signed_integer (value_contents (arg1),
Sean> +                                                      TYPE_LENGTH (type));
Sean> +      return value_from_fixed (type, (gdb_byte*)&unshiftedval);

I don't understand something here.

>From the expression.h change:

Sean> +    gdb_byte fixedconst[16];

So, I assume that at least in some case, a fixed-point value will
require 16 bytes.  However, I don't think LONGEST is guaranteed to be
that big.  It may be as small as 'long'.

Basically I'm concerned about the case where sizeof(LONGEST) and
TYPE_LENGTH(type) are not the same, and I'm confused about the choice
of '16' as the size of the buffer.

Sean> +      if(ieee_double) {
Sean> +        LONGEST lvalue;
Sean> +        DOUBLEST value = value_as_double (ieee_double);
[...]
Sean> +        lvalue = value;
Sean> +
Sean> +        return value_from_fixed (type, (gdb_byte*)&lvalue);

The "lvalue = value" assignment looks fishy to me.  This will do
ordinary C double->integral conversion; is that what you intend?

Sean> +      error (_("Failed to convert to fixed-point."));
Sean> +      return 0;

This return is redundant; error does not return.

Sean>  struct value *
Sean>  value_one (struct type *type, enum lval_type lv)
Sean>  {
Sean> -  struct type *type1 = check_typedef (type);
Sean> +  CHECK_TYPEDEF(type);
Sean>    struct value *val = NULL; /* avoid -Wall warning */

Unless I'm misreading the patch, this looks like a declaration after a
statement.  The project's coding style doesn't allow that; I thought
the default compiler settings for gdb would error about this, but I am
not certain.

Sean> -  *invp = 0;			/* Assume valid.   */
Sean> +  if(invp)
Sean> +    *invp = 0;

Please update the function's introductory comment to explain that INVP
may now be NULL.


I am not really that familiar with the fixed-point extension to C.  My
understanding is that some of the types saturate -- but I didn't see
any code here related to saturation.  Am I missing something?

The reason I ask about this is that a goal of mine is to have gdb's C
expression evaluator be faithful to the C programming language.  IMO,
some extensions are ok, but giving different results in defined cases
is not.

Given this, I think the valarith.c change is probably not correct.
Instead it ought to implement the rules defined by the language
extension.  I don't have this document but I assume that converting to
floating point is not the way to go.

Tom