This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] add support for debugging fixed-point numbers
- From: Tom Tromey <tromey at redhat dot com>
- To: "Sean D'Epagnier" <geckosenator at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 30 Dec 2008 13:00:28 -0700
- Subject: Re: [patch] add support for debugging fixed-point numbers
- References: <b9d4feda0812291740s62b913dbwee970d9c406d9d40@mail.gmail.com>
- Reply-to: 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