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

Tom Tromey
2008-12-30T20:01:11Z

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