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

["Sean D'Epagnier" <geckosenator at gmail dot com>]

Tom,

Thanks so much for reviewing my patch.  I am sorry for all the
inconsistencies, I will do my best to correct them, more comments
below.

On 12/30/08, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "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 would be glad to fill anything out needed for copyright assignment.

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

Yes, I thought about adding this support, but I decided to wait until
a later patch since it would require a bunch more changes.

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

Ok, I will triple check everything for formatting issues.

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

Ok, I will undo it.

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

The largest fixed point type I know of is "long long _Accum" which is
by default 16 bytes, however targets are free to redefine it to be
larger (none have done this yet)  It is a good point you made, LONGEST
might not be large enough in some of the cases I have used it.  If I
don't convert to double anymore, then hopefully this will be
corrected.

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

Yes, that is what I intended.

> Sean> +      error (_("Failed to convert to fixed-point."));
> Sean> +      return 0;
>
> This return is redundant; error does not return.
>

Ok, I will take note of that.

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

My mistake, I believe it is only a warning which is why I missed it.

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

Ok.

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

If the type is _Sat, then it is a saturating type.  I did not have any
support for this, however there is currently no way for me to know if
a variable is a saturating type (nothing in the dwarf format
specifying it)  So gdb might be inconsistent in the sense that if you
add a number to a saturating variable ie: "p x+.5" and x saturates,
then gdb won't know to saturate it, but unless we add more fields to
dwarf specifying this.  What do you suggest we do about this?  At
least you can examine saturating values correctly.

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

Ok, I was hoping to avoid this since it is so much simpler to just
convert to double to do all the operations.  I will have to implement
operations using only integer math and shifts to make it all work
correctly.  I should do this in the updated patch.



I also noticed what seems to be a quirk in gdb.  Maybe you have some
insight.  If you launch gdb and type something like:
(gdb) p ((unsigned _Accum)- 1)
$7 = -1
(gdb) p ((unsigned int)- 1)
$8 = 65535

It gets it incorrectly for _Accum, and unsigned accum should not be
negative.  This is because gdb gets the definition for the type from
the stabs data, not the dwarf data in this case (casting)  I'm not
sure why it does this.. but currently gcc outputs stabs data for
fixed-point which gdb parses as a float which is wrong, for example:

p ((_Fract) .1)
Unrecognized 16-bit floating-point type.

I added support for the decimal location in dwarf output, but not
stabs, because I'm not sure how to do it correctly.

I'm not sure which is best, to try to add fixed-point support to stabs
format too, or to make gdb read the types from the dwarf format in
this case (or maybe both)


I will try to get you an updated patch soon.

Thanks,
Sean

> Tom
>