Re: [RFC] decimal float point patch based on libdecnumber: gdb patch

Wu Zhou
2006-07-23T05:48:12Z

Hi Daniel,

Thanks for reviewing this patch.

Quoting Daniel Jacobowitz <drow@false.org>:

> On Thu, Jun 22, 2006 at 07:36:17AM +0800, Wu Zhou wrote:
>>
>> Appended is the patch. I had tested it with the latest gcc-4.2 cvs tree
>> on x86, with option "--enable-decimal-float". All the 121 tests passed.
>> Please review and comment. Thanks a lot.
>
> Sometimes you use DECDBL and sometimes you use DECDOUBLE and sometimes
> you use DECFLOAT and sometimes you use DECFLT. Let's pick one or the
> other and use it everywhere. This makes it easier to search for
> code that knows about decimal float. "decfloat" might be more
> readable?

I was using these different names because I wa simulating what
float/double/long double are using.

DECDBL is not used anywhere, but decdbl is. :-)
It is only used in write_exp_elt_decdblcst and its reference. That is
to simulate write_exp_elt_dblcst. I am not very sure why gdb named it
dbl though.

DECDOUBLE is used in OP_DECDOUBLE and value_from_decdouble. This is
to simulate OP_DOUBLE and value_from_double. All three kinds of float
constants and variables are handled by them. So I am using them for
DECDOUBLE too.

DECFLT is only used in TYPE_CODE_DECFLT. This is to simulate
TYPE_CODE_FLT, which is used to refer to types of float, double, long
double at the same time. The length can differentiate them.

DECFLOAT is used in parsing and fundamental type representation
dwarf2read.c and C language handling. such as FT_DECFLOAT,
FT_DBL_PREC_DECFLOAT and FT_EXT_PREC_DECFLOAT. This is the same what
FLOAT is used in these scanario.

My point is to keep these usage so as they are consistent with how
flaot/double is handled. What is your idea on this?

> Why is reverse_dfp necessary? It says GDB uses the opposite endianness
> from decNumber, but I don't see why. This might be a related problem,
> but are you accomodating target endianness when you read values from
> the target? If not, a cross debugger will do the wrong thing.

We are using an array of gdb_byte to represent decimal values. The
first byte is the most significant. This might be not the same as the
underlying byte order of the architecuture. In this kind of
situation, reverse_dfp is needed to do conversion.

But in big-endian machine, this might be not needed. I will try a test
on ppc64.

I am now also thinking that the place where reverse_dfp is defined is
not very good. Maybe we can put them into decimal_to_string /
decimal_from_string. Then other gdb code don't need to know the
existence of that.

BTW, I am not very sure what you means by saying "are you accomodating
target endianness when you read values from the target?". I guess you
mean that I _need_ to accommodate the endianness when reading values
from the target, right? Then putting some code into
decimal_from_string / decimal_to_string to convert the byte order if
needed is right what you want to see, right? Any advices?

>> Index: expression.h
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/expression.h,v
>> retrieving revision 1.18
>> diff -u -r1.18 expression.h
>> --- expression.h 17 Dec 2005 22:33:59 -0000 1.18
>> +++ expression.h 21 Jun 2006 23:08:51 -0000
>> @@ -327,6 +327,11 @@
>> /* A F90 array range operator (for "exp:exp", "exp:", ":exp"
>> and ":"). */
>> OP_F90_RANGE,
>>
>> + /* OP_DECDOUBLE is followed by a type pointer in the next exp_element
>> + and a dec long constant value in the following exp_element.
>> + Then comes another OP_DECDOUBLE. */
>> + OP_DECDOUBLE,
>> +
>
> A comment on the format of the data would be nice here. Is it in
> target byte order?

The first byte is the most significant byte. I don't have code to
make it into the target byte order yet. reverse_dfp can partially do
that. But it need to be more general. And I am planning to add some
code into decimal_to_string / decimal_from_string to test the target
byte order and determine if the conversion is needed. What is your
thought?

>> Index: c-valprint.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/c-valprint.c,v
>> retrieving revision 1.39
>> diff -u -r1.39 c-valprint.c
>> --- c-valprint.c 18 Jan 2006 21:24:19 -0000 1.39
>> +++ c-valprint.c 21 Jun 2006 23:08:52 -0000
>> @@ -442,6 +442,17 @@
>> }
>> break;
>>
>> + case TYPE_CODE_DECFLT:
>> + if (format)
>> + {
>> + print_scalar_formatted (valaddr + embedded_offset, type,
>> format, 0, stream);
>> + }
>> + else
>> + {
>> + print_decimal_floating (valaddr + embedded_offset, type, stream);
>> + }
>> + break;
>> +
>> case TYPE_CODE_METHOD:
>> {
>> struct value *v = value_at (type, address);
>
> You don't need the braces in this; please omit them when you don't need
> to. It saves a level of indentation, which generally makes them easier
> to read.

Thanks. I will delete them when I update my patch.

Regards
- Wu Zhou