[patch] add support for debugging fixed-point numbers

[patch] add support for debugging fixed-point numbers

[Sean D'Epagnier]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

Hi,

Recently gcc has added support for fixed-point types.  gdb didn't
understand these types (until now) but did have some preliminary
support since it is specified in the dwarf spec.

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

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

Thanks,
Sean

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb-fixedpoint.patch --]
[-- Type: text/x-diff; name=gdb-fixedpoint.patch, Size: 13583 bytes --]

Index: gdb/c-valprint.c
===================================================================
RCS file: /cvs/src/src/gdb/c-valprint.c,v
retrieving revision 1.54
diff -a -u -r1.54 c-valprint.c
--- gdb/c-valprint.c	28 Oct 2008 17:19:56 -0000	1.54
+++ gdb/c-valprint.c	30 Dec 2008 00:16:27 -0000
@@ -492,6 +492,13 @@
 	print_decimal_floating (valaddr + embedded_offset, type, stream);
       break;
 
+    case TYPE_CODE_FIXED:
+       /* convert to double and print that */
+       return c_value_print (value_cast (builtin_type_ieee_double,
+                                         value_from_contents_and_address
+                                         (type, valaddr + embedded_offset, 0)),
+                             stream, options);
+
     case TYPE_CODE_VOID:
       fprintf_filtered (stream, "void");
       break;
Index: gdb/dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.291
diff -a -u -r1.291 dwarf2read.c
--- gdb/dwarf2read.c	15 Nov 2008 18:49:50 -0000	1.291
+++ gdb/dwarf2read.c	30 Dec 2008 00:16:35 -0000
@@ -5088,6 +5088,12 @@
 	  code = TYPE_CODE_CHAR;
 	type_flags |= TYPE_FLAG_UNSIGNED;
 	break;
+      case DW_ATE_unsigned_fixed:
+	type_flags |= TYPE_FLAG_UNSIGNED;
+
+      case DW_ATE_signed_fixed:
+        code = TYPE_CODE_FIXED;
+        break;
       default:
 	complaint (&symfile_complaints, _("unsupported DW_AT_encoding: '%s'"),
 		   dwarf_type_encoding_name (encoding));
@@ -5098,6 +5104,12 @@
   TYPE_NAME (type) = name;
   TYPE_TARGET_TYPE (type) = target_type;
 
+  if (code == TYPE_CODE_FIXED) {
+    attr = dwarf2_attr (die, DW_AT_binary_scale, cu);
+    if (attr)
+      TYPE_BINARYSCALE (type) = DW_SND (attr);
+  }
+
   if (name && strcmp (name, "char") == 0)
     TYPE_NOSIGN (type) = 1;
 
Index: gdb/eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.103
diff -a -u -r1.103 eval.c
--- gdb/eval.c	28 Dec 2008 14:14:19 -0000	1.103
+++ gdb/eval.c	30 Dec 2008 00:16:37 -0000
@@ -703,7 +703,10 @@
       (*pos) += 3;
       return value_from_decfloat (exp->elts[pc + 1].type,
 				  exp->elts[pc + 2].decfloatconst);
-
+    case OP_FIXED:
+      (*pos) += 3;
+      return value_from_fixed (exp->elts[pc + 1].type,
+			       exp->elts[pc + 2].fixedconst);
     case OP_VAR_VALUE:
       (*pos) += 3;
       if (noside == EVAL_SKIP)
Index: gdb/expression.h
===================================================================
RCS file: /cvs/src/src/gdb/expression.h,v
retrieving revision 1.29
diff -a -u -r1.29 expression.h
--- gdb/expression.h	11 Sep 2008 14:12:15 -0000	1.29
+++ gdb/expression.h	30 Dec 2008 00:16:37 -0000
@@ -334,15 +334,20 @@
        Then comes another OP_DECFLOAT.  */
     OP_DECFLOAT,
 
-     /* First extension operator.  Individual language modules define
-        extra operators they need as constants with values 
-        OP_LANGUAGE_SPECIFIC0 + k, for k >= 0, using a separate 
-        enumerated type definition:
-           enum foo_extension_operator {
-             BINOP_MOGRIFY = OP_EXTENDED0,
- 	     BINOP_FROB,
- 	     ...
-           };      */
+    /* OP_FIXED is followed by by a type pointer in the next exp_element
+       and a fixed constant value in the following exp_element,
+       Then comes the binaryscale */
+    OP_FIXED,
+
+    /* First extension operator.  Individual language modules define
+       extra operators they need as constants with values 
+       OP_LANGUAGE_SPECIFIC0 + k, for k >= 0, using a separate 
+       enumerated type definition:
+       enum foo_extension_operator {
+       BINOP_MOGRIFY = OP_EXTENDED0,
+       BINOP_FROB,
+       ...
+       };      */
     OP_EXTENDED0,
   
     /* Last possible extension operator.  Defined to provide an
@@ -362,6 +367,7 @@
     LONGEST longconst;
     DOUBLEST doubleconst;
     gdb_byte decfloatconst[16];
+    gdb_byte fixedconst[16];
     /* Really sizeof (union exp_element) characters (or less for the last
        element of a string).  */
     char string;
Index: gdb/gdbtypes.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.h,v
retrieving revision 1.96
diff -a -u -r1.96 gdbtypes.h
--- gdb/gdbtypes.h	28 Dec 2008 14:14:19 -0000	1.96
+++ gdb/gdbtypes.h	30 Dec 2008 00:16:39 -0000
@@ -134,7 +134,8 @@
 
     TYPE_CODE_NAMESPACE,	/* C++ namespace.  */
 
-    TYPE_CODE_DECFLOAT		/* Decimal floating point.  */
+    TYPE_CODE_DECFLOAT,		/* Decimal floating point.  */
+    TYPE_CODE_FIXED		/* Fixed point type */
   };
 
 /* For now allow source to use TYPE_CODE_CLASS for C++ classes, as an
@@ -517,6 +518,9 @@
 
     const struct floatformat **floatformat;
 
+    /* BINARYSCALE is for TYPE_CODE_FIXED. */
+    int binaryscale;
+
     /* For TYPE_CODE_FUNC types, the calling convention for targets
        supporting multiple ABIs.  Right now this is only fetched from
        the Dwarf-2 DW_AT_calling_convention attribute.  */
@@ -838,6 +842,7 @@
 #define	TYPE_TYPE_SPECIFIC(thistype) TYPE_MAIN_TYPE(thistype)->type_specific
 #define TYPE_CPLUS_SPECIFIC(thistype) TYPE_MAIN_TYPE(thistype)->type_specific.cplus_stuff
 #define TYPE_FLOATFORMAT(thistype) TYPE_MAIN_TYPE(thistype)->type_specific.floatformat
+#define TYPE_BINARYSCALE(thistype) TYPE_MAIN_TYPE(thistype)->type_specific.binaryscale
 #define TYPE_CALLING_CONVENTION(thistype) TYPE_MAIN_TYPE(thistype)->type_specific.calling_convention
 #define TYPE_BASECLASS(thistype,index) TYPE_MAIN_TYPE(thistype)->fields[index].type
 #define TYPE_N_BASECLASSES(thistype) TYPE_CPLUS_SPECIFIC(thistype)->n_baseclasses
Index: gdb/valarith.c
===================================================================
RCS file: /cvs/src/src/gdb/valarith.c,v
retrieving revision 1.68
diff -a -u -r1.68 valarith.c
--- gdb/valarith.c	28 Dec 2008 14:14:19 -0000	1.68
+++ gdb/valarith.c	30 Dec 2008 00:16:40 -0000
@@ -881,9 +881,11 @@
 
   if ((TYPE_CODE (type1) != TYPE_CODE_FLT
        && TYPE_CODE (type1) != TYPE_CODE_DECFLOAT
+       && TYPE_CODE (type1) != TYPE_CODE_FIXED
        && !is_integral_type (type1))
       || (TYPE_CODE (type2) != TYPE_CODE_FLT
 	  && TYPE_CODE (type2) != TYPE_CODE_DECFLOAT
+	  && TYPE_CODE (type2) != TYPE_CODE_FIXED
 	  && !is_integral_type (type2)))
     error (_("Argument to arithmetic operation not a number or boolean."));
 
@@ -924,6 +926,21 @@
 
       val = value_from_decfloat (result_type, v);
     }
+  else if (TYPE_CODE (type1) == TYPE_CODE_FIXED
+      || TYPE_CODE (type2) == TYPE_CODE_FIXED)
+    {
+      struct value *result;
+
+      /* convert fixed to double */
+      if(TYPE_CODE (type1) == TYPE_CODE_FIXED)
+         arg1 = value_cast(builtin_type_ieee_double, arg1);
+      if(TYPE_CODE (type2) == TYPE_CODE_FIXED)
+         arg2 = value_cast(builtin_type_ieee_double, arg2);
+
+      /* use largest type for result */
+      result_type = (TYPE_LENGTH (type1) > TYPE_LENGTH (type2)) ? type1 : type2;
+      return value_cast (result_type, value_binop (arg1, arg2, op));
+    }
   else if (TYPE_CODE (type1) == TYPE_CODE_FLT
 	   || TYPE_CODE (type2) == TYPE_CODE_FLT)
     {
@@ -1485,6 +1502,8 @@
     return value_from_double (type, value_as_double (arg1));
   else if (TYPE_CODE (type) == TYPE_CODE_DECFLOAT)
     return value_from_decfloat (type, value_contents (arg1));
+  else if (TYPE_CODE (type) == TYPE_CODE_FIXED)
+    return value_from_fixed (type, value_contents (arg1));
   else if (is_integral_type (type))
     {
       return value_from_longest (type, value_as_long (arg1));
@@ -1520,6 +1539,12 @@
       memcpy (value_contents_raw (val), decbytes, len);
       return val;
     }
+  else if (TYPE_CODE (type) == TYPE_CODE_FIXED)
+    {
+      LONGEST unshiftedval = -extract_signed_integer (value_contents (arg1),
+                                                      TYPE_LENGTH (type));
+      return value_from_fixed (type, (gdb_byte*)&unshiftedval);
+    }
   else if (TYPE_CODE (type) == TYPE_CODE_FLT)
     return value_from_double (type, -value_as_double (arg1));
   else if (is_integral_type (type))
Index: gdb/valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.204
diff -a -u -r1.204 valops.c
--- gdb/valops.c	29 Dec 2008 06:02:06 -0000	1.204
+++ gdb/valops.c	30 Dec 2008 00:16:42 -0000
@@ -409,7 +409,7 @@
 
   scalar = (code2 == TYPE_CODE_INT || code2 == TYPE_CODE_FLT
 	    || code2 == TYPE_CODE_DECFLOAT || code2 == TYPE_CODE_ENUM
-	    || code2 == TYPE_CODE_RANGE);
+	    || code2 == TYPE_CODE_RANGE || code2 == TYPE_CODE_FIXED);
 
   if ((code1 == TYPE_CODE_STRUCT || code1 == TYPE_CODE_UNION)
       && (code2 == TYPE_CODE_STRUCT || code2 == TYPE_CODE_UNION)
@@ -438,6 +438,32 @@
 
       return value_from_decfloat (type, dec);
     }
+  else if (code1 == TYPE_CODE_FIXED && scalar)
+    {
+      /* first convert to double */
+      struct value *ieee_double = value_cast (builtin_type_ieee_double, arg2);
+
+      if(ieee_double) {
+        LONGEST lvalue;
+        DOUBLEST value = value_as_double (ieee_double);
+        int binaryscale = TYPE_BINARYSCALE(type);
+
+        /* shift by the binary scale value */
+        while(binaryscale < 0) {
+          binaryscale++;
+          value *= 2;
+        }
+        while(binaryscale > 0) {
+          binaryscale--;
+          value /= 2;
+        }
+        lvalue = value;
+
+        return value_from_fixed (type, (gdb_byte*)&lvalue);
+      }
+      error (_("Failed to convert to fixed-point."));
+      return 0;
+    }
   else if ((code1 == TYPE_CODE_INT || code1 == TYPE_CODE_ENUM
 	    || code1 == TYPE_CODE_RANGE)
 	   && (scalar || code2 == TYPE_CODE_PTR
@@ -539,26 +565,14 @@
 struct value *
 value_one (struct type *type, enum lval_type lv)
 {
-  struct type *type1 = check_typedef (type);
+  CHECK_TYPEDEF(type);
   struct value *val = NULL; /* avoid -Wall warning */
 
-  if (TYPE_CODE (type1) == TYPE_CODE_DECFLOAT)
-    {
-      struct value *int_one = value_from_longest (builtin_type_int32, 1);
-      struct value *val;
-      gdb_byte v[16];
-
-      decimal_from_integral (int_one, v, TYPE_LENGTH (builtin_type_int32));
-      val = value_from_decfloat (type, v);
-    }
-  else if (TYPE_CODE (type1) == TYPE_CODE_FLT)
-    {
-      val = value_from_double (type, (DOUBLEST) 1);
-    }
-  else if (is_integral_type (type1))
-    {
-      val = value_from_longest (type, (LONGEST) 1);
-    }
+  if (TYPE_CODE (type) == TYPE_CODE_DECFLOAT
+      || TYPE_CODE (type) == TYPE_CODE_FLT
+      || TYPE_CODE (type) == TYPE_CODE_FIXED
+      || is_integral_type (type))
+    val = value_cast (type, value_from_longest (builtin_type_int32, 1));
   else
     {
       error (_("Not a numeric type."));
Index: gdb/value.c
===================================================================
RCS file: /cvs/src/src/gdb/value.c,v
retrieving revision 1.72
diff -a -u -r1.72 value.c
--- gdb/value.c	26 Nov 2008 16:27:27 -0000	1.72
+++ gdb/value.c	30 Dec 2008 00:16:44 -0000
@@ -1196,6 +1196,9 @@
 	 it doesn't work when the decimal number has a fractional part.  */
       return decimal_to_doublest (valaddr, len);
 
+    case TYPE_CODE_FIXED:
+      return unpack_double (type, valaddr, 0);
+
     case TYPE_CODE_PTR:
     case TYPE_CODE_REF:
       /* Assume a CORE_ADDR can fit in a LONGEST (for now).  Not sure
@@ -1221,7 +1224,8 @@
   int len;
   int nosign;
 
-  *invp = 0;			/* Assume valid.   */
+  if(invp)
+    *invp = 0;
   CHECK_TYPEDEF (type);
   code = TYPE_CODE (type);
   len = TYPE_LENGTH (type);
@@ -1247,7 +1251,8 @@
 
       if (!floatformat_is_valid (floatformat_from_type (type), valaddr))
 	{
-	  *invp = 1;
+          if(invp)
+            *invp = 1;
 	  return 0.0;
 	}
 
@@ -1255,6 +1260,26 @@
     }
   else if (code == TYPE_CODE_DECFLOAT)
     return decimal_to_doublest (valaddr, len);
+  else if (code == TYPE_CODE_FIXED)
+    {
+      DOUBLEST value;
+      int binaryscale = TYPE_BINARYSCALE(type);
+      if (nosign)
+        value = extract_unsigned_integer (valaddr, len);
+      else
+        value = extract_signed_integer (valaddr, len);
+      
+      /* shift by the binary scale value */
+      while(binaryscale < 0) {
+        binaryscale++;
+        value /= 2;
+      }
+      while(binaryscale > 0) {
+        binaryscale--;
+        value *= 2;
+      }
+      return value;
+    }
   else if (nosign)
     {
       /* Unsigned -- be sure we compensate for signed LONGEST.  */
@@ -1738,6 +1763,16 @@
 }
 
 struct value *
+value_from_fixed (struct type *type, const gdb_byte *value)
+{
+  struct value *val = allocate_value (type);
+
+  memcpy (value_contents_raw (val), value, TYPE_LENGTH (type));
+
+  return val;
+}
+
+struct value *
 coerce_ref (struct value *arg)
 {
   struct type *value_type_arg_tmp = check_typedef (value_type (arg));
Index: gdb/value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.126
diff -a -u -r1.126 value.h
--- gdb/value.h	22 Dec 2008 23:11:56 -0000	1.126
+++ gdb/value.h	30 Dec 2008 00:16:44 -0000
@@ -284,6 +284,9 @@
 extern struct value *value_from_double (struct type *type, DOUBLEST num);
 extern struct value *value_from_decfloat (struct type *type,
 					  const gdb_byte *decbytes);
+extern struct value *value_from_fixed (struct type *type,
+                                       const gdb_byte *value);
+
 extern struct value *value_from_string (char *string);
 
 extern struct value *value_at (struct type *type, CORE_ADDR addr);

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: dwarf2_binaryscale.patch --]
[-- Type: text/x-diff; name=dwarf2_binaryscale.patch, Size: 627 bytes --]

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 142945)
+++ gcc/dwarf2out.c	(working copy)
@@ -9344,6 +9344,12 @@
 
   add_AT_unsigned (base_type_result, DW_AT_byte_size,
 		   int_size_in_bytes (type));
+
+  /* version 3 dwarf specifies that for fixed-point types DW_AT_binary_scale
+     describes the location of the decimal place */
+  if (TREE_CODE (type) == FIXED_POINT_TYPE)
+    add_AT_int (base_type_result, DW_AT_binary_scale, -TYPE_FBIT (type));
+
   add_AT_unsigned (base_type_result, DW_AT_encoding, encoding);
 
   return base_type_result;

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

[Tom Tromey]

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

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

[Sean D'Epagnier]

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
>

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

[Joel Brobecker]

> I would be glad to fill anything out needed for copyright assignment.

For the record, I sent Sean the request form.

-- 
Joel

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

[Tom Tromey]

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

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

Oh, don't be sorry... the coding standard is just a little hurdle
everybody has to get over when they first contribute, not a big deal.

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

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

This sounds like a Dwarf oversight to me.  Perhaps we can either get
something officially defined here (I don't know how to do that,
though), or define a GNU extension.  I think that, at least, this
ought to be filed as a GCC bug, and maybe a GDB bug as well, once your
patch goes in.

IMO, the absence of this information should not block your patch.

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

Offhand, I have no idea what is going on here, sorry.

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

I also have no idea about this :).  I don't know anything about stabs,
I'm afraid.

Tom

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

[Joel Brobecker]

> Sean> I'm not sure which is best, to try to add fixed-point support to stabs
> Sean> format too, or to make gdb read the types from the dwarf format in
> Sean> this case (or maybe both)
> 
> I also have no idea about this :).  I don't know anything about stabs,
> I'm afraid.

As far as I can tell, stabs does not provide support for fixed point
types. But regardless of that, I wouldn't worry about stabs for new
features, as the use of this debugging format is getting closer to
zero. At AdaCore, we still use it because we support platforms such
as AIX where moving to DWARF didn't work, so you will see occasional
fixes from me, but that's as far as I will go with stabs.

-- 
Joel