[RFC,13/19] Target FP: Perform Ada fixed-point scaling in target format

  [RFC][13/19] Target FP: Perform Ada fixed-point scaling in target format

One of the few still remaining uses of DOUBLEST in GDB is the Ada front-end
code that handles scaling of Ada fixed-point types.  The target format for
those types is some integer format; to convert those values to standard
floating-point representation, that integer needs to be multiplied by a
rational scale factor, given as a pair of numerator and denominator.

To avoid having to deal with long integer arithmetic, the current Ada
front-end code currently performs those scaling operations in host
DOUBLEST arithmetic.  To eliminate this use of DOUBLEST, this patch
changes the front-end to instead perform those operations in the
*target* floating-point format (chosing to use the target "long double").

The implementation is mostly straight-forward, using value_cast and
value_binop to perform the target operations.

Scanning in the scale numerator and denominator is now done into
a host "long long" instead of a DOUBLEST, which should be large
enough to hold all possible values.  (Otherwise, this can be replaced
by target-format target_float_from_string operations as well.)

Printing fixed-point types and values should be completely unchanges,
using target_float_to_string with the same format strings as current code.

Bye,
Ulrich

ChangeLog:

	* ada-lang.c (cast_to_fixed): Reimplement in target arithmetic.
	(cast_to_fixed): Likewise.
	(ada_scaling_type): New function.
	(ada_delta): Return value instead of DOUBLEST.  Perform target
	arithmetic instead of host arithmetic.
	(scaling_factor): Rename to ...
	(ada_scaling_factor) ... this.  Return value instead of DOUBLEST.
	Perform target arithmetic instead of host arithmetic.
	(ada_fixed_to_float): Remove.
	(ada_float_to_fixed): Remove.
	* ada-lang.h (ada_fixed_to_float): Remove.
	(ada_float_to_fixed): Remove.
	(ada_delta): Return value instead of DOUBLEST.
	(ada_scaling_factor): Add prototype.

	* ada-typeprint.c: Include "target-float.h".
	(print_fixed_point_type): Perform target arithmetic instead of
	host arithmetic.
	* ada-valprint.c: Include "target-float.h".
	(ada_val_print_num): Perform target arithmetic instead of
	host arithmetic for fixed-point types.
  

On Tue, Sep 05, 2017 at 08:21:35PM +0200, Ulrich Weigand wrote:
> [RFC][13/19] Target FP: Perform Ada fixed-point scaling in target format
> 
> One of the few still remaining uses of DOUBLEST in GDB is the Ada front-end
> code that handles scaling of Ada fixed-point types.  The target format for
> those types is some integer format; to convert those values to standard
> floating-point representation, that integer needs to be multiplied by a
> rational scale factor, given as a pair of numerator and denominator.
> 
> To avoid having to deal with long integer arithmetic, the current Ada
> front-end code currently performs those scaling operations in host
> DOUBLEST arithmetic.  To eliminate this use of DOUBLEST, this patch
> changes the front-end to instead perform those operations in the
> *target* floating-point format (chosing to use the target "long double").
> 
> The implementation is mostly straight-forward, using value_cast and
> value_binop to perform the target operations.
> 
> Scanning in the scale numerator and denominator is now done into
> a host "long long" instead of a DOUBLEST, which should be large
> enough to hold all possible values.  (Otherwise, this can be replaced
> by target-format target_float_from_string operations as well.)
> 
> Printing fixed-point types and values should be completely unchanges,
> using target_float_to_string with the same format strings as current code.
> 
> Bye,
> Ulrich
> 
> ChangeLog:
> 
> 	* ada-lang.c (cast_to_fixed): Reimplement in target arithmetic.
> 	(cast_to_fixed): Likewise.
         ^^^^^^^^^^^^^
         cast_from_fixed :)

> 	(ada_scaling_type): New function.
> 	(ada_delta): Return value instead of DOUBLEST.  Perform target
> 	arithmetic instead of host arithmetic.
> 	(scaling_factor): Rename to ...
> 	(ada_scaling_factor) ... this.  Return value instead of DOUBLEST.
> 	Perform target arithmetic instead of host arithmetic.

Maybe mention that we are making this function non-static too?

> 	(ada_fixed_to_float): Remove.
> 	(ada_float_to_fixed): Remove.
> 	* ada-lang.h (ada_fixed_to_float): Remove.
> 	(ada_float_to_fixed): Remove.
> 	(ada_delta): Return value instead of DOUBLEST.
> 	(ada_scaling_factor): Add prototype.
> 
> 	* ada-typeprint.c: Include "target-float.h".
> 	(print_fixed_point_type): Perform target arithmetic instead of
> 	host arithmetic.
> 	* ada-valprint.c: Include "target-float.h".
> 	(ada_val_print_num): Perform target arithmetic instead of
> 	host arithmetic for fixed-point types.

Generally speaking, the patch looks good to me, and the only remark
I have is actually an trivial C++ question which you probably had
covered.

We have a fixed point tests in the testsuite (gdb.ada/fixed_points.exp),
so having it run and pass after your change should be a very good sanity
check on its own.

Thanks for doing that!

[...]
> Index: binutils-gdb/gdb/ada-typeprint.c
> ===================================================================
> --- binutils-gdb.orig/gdb/ada-typeprint.c
> +++ binutils-gdb/gdb/ada-typeprint.c
[...]
> @@ -360,16 +361,23 @@ print_enum_type (struct type *type, stru
>  static void
>  print_fixed_point_type (struct type *type, struct ui_file *stream)
>  {
> -  DOUBLEST delta = ada_delta (type);
> -  DOUBLEST small = ada_fixed_to_float (type, 1);
> +  struct value *delta = ada_delta (type);
> +  struct value *small = ada_scaling_factor (type);
>  
> -  if (delta < 0.0)
> +  if (delta == nullptr)
>      fprintf_filtered (stream, "delta ??");
>    else
>      {
> -      fprintf_filtered (stream, "delta %g", (double) delta);
> -      if (delta != small)
> -	fprintf_filtered (stream, " <'small = %g>", (double) small);
> +      std::string str;
> +      str = target_float_to_string (value_contents (delta),
> +				    value_type (delta), "%g");

This is a C++ question, really. Does it make any difference if you
declare the std::string first, and then only set its contents in
a second statement? I can't remember the details, but it has to do
with initialization vs assignment. I _really_ hope that the string
class is sufficiently well designed that the two are really equivalent
in practice?
  

> > This is a C++ question, really. Does it make any difference if you
> > declare the std::string first, and then only set its contents in
> > a second statement? I can't remember the details, but it has to do
> > with initialization vs assignment. I _really_ hope that the string
> > class is sufficiently well designed that the two are really equivalent
> > in practice?
> 
> Huh.  Indeed I see worse code when doing the assignment as a separate
> statement, at least with GCC 4.8.  I'll make sure to use initialization
> instead wherever possible.  Thanks for pointing this out!

Let's wait for people who really know better about C++ to tell us
whether it makes a difference. I was amazed as how careful you have
to be when using C++ to avoid inefficiencies, but perhaps I am simply
being paranoid in this case... That's why I tried to phrase this as
a question.
  

On 2017-10-09 02:09 PM, Joel Brobecker wrote:
>>> This is a C++ question, really. Does it make any difference if you
>>> declare the std::string first, and then only set its contents in
>>> a second statement? I can't remember the details, but it has to do
>>> with initialization vs assignment. I _really_ hope that the string
>>> class is sufficiently well designed that the two are really equivalent
>>> in practice?
>>
>> Huh.  Indeed I see worse code when doing the assignment as a separate
>> statement, at least with GCC 4.8.  I'll make sure to use initialization
>> instead wherever possible.  Thanks for pointing this out!
> 
> Let's wait for people who really know better about C++ to tell us
> whether it makes a difference. I was amazed as how careful you have
> to be when using C++ to avoid inefficiencies, but perhaps I am simply
> being paranoid in this case... That's why I tried to phrase this as
> a question.
> 

Indeed, it's preferable to use

  std::string foo = returns_string ();

than

  std::string foo;
  foo = returns_string ();

The reason being that in the second case, the default constructor (the
one with no params) is called to construct an empty string, and then that
work is scrapped because we assign a new value.  The first form constructs
the string right away with the right content.

While it's preferable to use the first one, the earth wouldn't stop spinning
if you used the second form.  In the particular case of std::string, the
default constructor is basically assigning a few fields.

Simon
  

On Mon, 9 Oct 2017, Simon Marchi wrote:

> > Let's wait for people who really know better about C++ to tell us
> > whether it makes a difference. I was amazed as how careful you have
> > to be when using C++ to avoid inefficiencies, but perhaps I am simply
> > being paranoid in this case... That's why I tried to phrase this as
> > a question.
> > 
> 
> Indeed, it's preferable to use
> 
>   std::string foo = returns_string ();
> 
> than
> 
>   std::string foo;
>   foo = returns_string ();
> 
> The reason being that in the second case, the default constructor (the
> one with no params) is called to construct an empty string, and then that
> work is scrapped because we assign a new value.  The first form constructs
> the string right away with the right content.

 Hmm, wouldn't a half-decent compiler notice that the result produced by 
the default constructor is discarded and optimise the call away?  Also has 
there actually been an assertion in (the current definition of C++) that 
all declared objects have also been initialised?

  Maciej the curious