Re: [RFC] Decimal Floating Point support for GDB (Part 1: patch)

[Wu Zhou <woodzltc at cn dot ibm dot com>]

Daniel,  thanks for your comments.  Please see below for my responses.

On Sun, 2 Oct 2005, Daniel Jacobowitz wrote:

> I'm afraid I am not very happy with this approach :-(
> 
> 	* c-exp.y (parse_number): Parse the decimal floating point, which 
> 	has a suffix ('df', 'dd' or 'dl') and return STRING here.

In fact.  I am not very happy with that either.  But this is what came out 
of my mind at this time, provided that gcc and glibc support for DFP is 
not ready yet.  If the gdb-building compiler already support dfp types 
(_Decimal32, _Decimal64 and _Decimal64) and sscanf (GDB uses it to parse 
floating point number) could recognize and scan decimal types, I won't 
adopt this method.

Lookin at this code again, it is more like a proof-of-concept.

> This certainly isn't right.  It produces several problems:
> 
> (gdb) p 1.2df
> evaluation of this expression requires the target program to be active

Didn't predict that.  So all string expression handling need the target 
program to be active?

> [Side effect of string handling]
>  
> (gdb) p 1.2df
> $1 = "1.2"

Predicted this.  It seems that it is still acceptable. Right? 
:-)

> 
> (gdb) p $1[1]
> $2 = 46 '.'
> 
> (gdb) p (1.2df)
> A syntax error in expression, near `'.

Didn't predict this.

> If we have a GDB type system representation for decimal floats, the C
> parser should use it, and for lexing purposes it should probably be a
> FLOAT.

Okay. I didn't thought of adding a gdb type for decimal floats.  Maybe I 
should (It seems to be a good approach).  But I am not very familar 
with GDB's type system.  Any clue about how to start?  adding a 
builtin_type for DFP and letting it have three or more fields (like sign, 
coefficient, exponent...)?  I will take some more looks into the code. 

> I'm not familiar with the proposed C language changes, but GDB should
> plan to support them properly; the same set of implicit or invalid
> conversions, supported operations, et cetera.  We certainly don't need
> to implement all that; the C interpreter is pretty casual.  But let's
> consider them at least.  And maybe issue intelligent error messages.

Our original plan is to only support part of these c99-enhancement 
proposal in GDB, mainly printing and setting of DFP types.  To support all 
these changes (including implicit/explicit conversion, arithmetic 
operation...), we may need to introduce decNumber library in (which we 
had thought that it won't be acceptable for GDB community).  If you think 
it is okay, maybe we can reconsider this method. 

> > 	* dfp.h: New header file for decimal floating point support in GDB.
> > 	main functions is decimal_from_string and decimal_to_string.
> > 	* dfp.c: New source file for decimal floating point support in GDB.
> > 	Implement decimal_from_string and decimal_to_strin.
> 
> The decNumber library is going to be (or already is) contributed to the
> FSF and GPL-licensed. In that case, perhaps we can move it to the top
> level of the repository, and use it to implement support in GDB.  The
> alternative is adding a lot of sensitive numeric code to GDB that the
> GDB developers won't know much about.
> 
> > 	* valops.c (value_set_dfp): New function, calling decimal_from_string
> > 	to set the value of decimal floating point types.
> 
> This is wrong for a couple of reasons.  I think you're doing this to
> avoid the cast to arg1's type in evaluate_subexp_standard; but in the
> process you've ignored "noside" and binop_user_defined_p (assuming
> eventual C++ support), and you're assuming that fromval is a string.
> Which means that 'dfp_var1 = dfp_var2' is going to crash.  And
> 'dfp_var1 = "1.2"' would probably work, which is unlikely to be what
> you wanted.

you are right.  I didn't thought of that.

> I recommend representing decimal floating point numbers in GDB as
> decimal floating point numbers, not as strings.  Then it will be pretty
> clear where in e.g. valarith.c to handle it.  Or, initially, reject it.

Okay, I will consider this seriously.  Thanks for your kind suggestion.

> Oh, and in a couple of places your code says:
> 
> +    Contributed by Cygnus Support, using pieces from other GDB modules.
> 
> which is not true since you're contributing it :-)

Stupid.  I forgot to change that.  Thanks for your reminder.

Best Regards
- Wu Zhou