[PATCH 0/4] Fixed-point ABI fixes

[PATCH 0/4] Fixed-point ABI fixes

[Tom Tromey]

This series fixes some ABI issues involving fixed-point types.

The patches weren't written by me -- I'm just submitting them.
They've been in use at AdaCore for a while, one of them since 2017,
with the last one being written in December 2020.

A fixed-point type is really just an integer with a special
interpretation.  I think targets should normally treat it like an
integer for ABI purposes.  A bit of care may be needed to ensure the
correct bits end up being passed, depending on how the
argument-passing code is written.

thanks,
Tom

[PATCH 1/4] (Ada/AArch64) fix fixed point argument passing in inferior funcall

[Tom Tromey]

From: Xavier Roirand <roirand@adacore.com>

Consider the following code:

   type FP1_Type is delta 0.1 range -1.0 .. +1.0; --  Ordinary

   function Call_FP1 (F : FP1_Type) return FP1_Type is
   begin
      return F;
   end Call_FP1;

When the default in GCC is to generate proper DWARF info for fixed point
types, then in gdb, printing the result of a call to call_fp1 with a
decimal parameter leads to:

  (gdb) p call_fp1(0.5)
  $1 = 0

The displayed value is wrong, and we actually expected:

  (gdb) p call_fp1(0.5)
  $1 = 0.5

What happened is that our fixed point type parameter got promoted to a
32bit integer because we detected that the length of that object was less
than 4 bytes. The compiler does not perform this promotion and therefore
GDB should not either.

This patch fixes the behavior described above.
---
 gdb/aarch64-tdep.c                            |  2 +-
 .../gdb.ada/fixed_points_function.exp         | 38 +++++++++++++++++++
 .../fixed_points_function.adb                 | 30 +++++++++++++++
 .../gdb.ada/fixed_points_function/pck.adb     | 22 +++++++++++
 .../gdb.ada/fixed_points_function/pck.ads     | 21 ++++++++++
 5 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.ada/fixed_points_function.exp
 create mode 100644 gdb/testsuite/gdb.ada/fixed_points_function/fixed_points_function.adb
 create mode 100644 gdb/testsuite/gdb.ada/fixed_points_function/pck.adb
 create mode 100644 gdb/testsuite/gdb.ada/fixed_points_function/pck.ads

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 4b5af4616af..44019b8768d 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1876,7 +1876,7 @@ aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 	case TYPE_CODE_CHAR:
 	case TYPE_CODE_RANGE:
 	case TYPE_CODE_ENUM:
-	  if (len < 4)
+	  if (len < 4 && !is_fixed_point_type (arg_type))
 	    {
 	      /* Promote to 32 bit integer.  */
 	      if (arg_type->is_unsigned ())
diff --git a/gdb/testsuite/gdb.ada/fixed_points_function.exp b/gdb/testsuite/gdb.ada/fixed_points_function.exp
new file mode 100644
index 00000000000..5a721fdf4fc
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/fixed_points_function.exp
@@ -0,0 +1,38 @@
+# Copyright 2021 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "ada.exp"
+
+if { [skip_ada_tests] } { return -1 }
+
+standard_ada_testfile fixed_points_function
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug]] != "" } {
+    return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "STOP" ${testdir}/fixed_points_function.adb]
+runto "fixed_points_function.adb:$bp_location"
+
+gdb_test "print call_fp1(1)" \
+         " = 1"
+
+gdb_test "print call_fp1(0.5)" \
+         " = 0.5"
+
+gdb_test "print call_fp1(-0.5)" \
+         " = -0.5"
diff --git a/gdb/testsuite/gdb.ada/fixed_points_function/fixed_points_function.adb b/gdb/testsuite/gdb.ada/fixed_points_function/fixed_points_function.adb
new file mode 100644
index 00000000000..4f82a277ff1
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/fixed_points_function/fixed_points_function.adb
@@ -0,0 +1,30 @@
+--  Copyright 2021 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+with Pck; use Pck;
+
+procedure Fixed_Points_Function is
+
+   ------------
+   -- Test 1 --
+   ------------
+
+   --  Fixed point funcall
+
+   F1 : FP1_Type := 1.0;
+
+begin
+   F1 := Call_FP1 (F1); -- STOP
+end Fixed_Points_Function;
diff --git a/gdb/testsuite/gdb.ada/fixed_points_function/pck.adb b/gdb/testsuite/gdb.ada/fixed_points_function/pck.adb
new file mode 100644
index 00000000000..d8766fe05f9
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/fixed_points_function/pck.adb
@@ -0,0 +1,22 @@
+--  Copyright 2021 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package body Pck is
+
+   function Call_FP1 (F : FP1_Type) return FP1_Type is
+   begin
+      return F;
+   end Call_FP1;
+end pck;
diff --git a/gdb/testsuite/gdb.ada/fixed_points_function/pck.ads b/gdb/testsuite/gdb.ada/fixed_points_function/pck.ads
new file mode 100644
index 00000000000..4ef29a8f0d8
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/fixed_points_function/pck.ads
@@ -0,0 +1,21 @@
+--  Copyright 2021 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package Pck is
+
+   type FP1_Type is delta 0.1 range -1.0 .. +1.0; --  Ordinary
+
+   function Call_FP1 (F : FP1_Type) return FP1_Type;
+end pck;
-- 
2.31.1

[PATCH 2/4] (ARM/fixed-point) wrong value shown by "finish" command:

[Tom Tromey]

From: Joel Brobecker <brobecker@adacore.com>

Consider the following Ada code:

   type FP1_Type is delta 0.1 range -1.0 .. +1.0; --  Ordinary
   FP1_Arg : FP1_Type := 0.0;

   function Call_FP1 (F : FP1_Type) return FP1_Type is
   begin
      FP1_Arg := F;
      return FP1_Arg;
   end Call_FP1;

After having stopped inside function Call_FP1 as follow:

    Breakpoint 1, pck.call_fp1 (f=1) at /[...]/pck.adb:5
    5             FP1_Arg := F;

Returning from that function call using "finish" should show
that the function return "1.0" (the same value as was passed
as an argument). However, this is not the case:

    (gdb) finish
    Run till exit from #0  pck.call_fp1 (f=1)
    [...]
    9          F1 := Call_FP1 (F1);
    Value returned is $1 = 0

This patch enhances the extraction of the return value to know about
fixed point types.
---
 gdb/arm-tdep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index d7493f15700..3927d6bc272 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -7987,7 +7987,8 @@ arm_extract_return_value (struct type *type, struct regcache *regs,
 	   || type->code () == TYPE_CODE_BOOL
 	   || type->code () == TYPE_CODE_PTR
 	   || TYPE_IS_REFERENCE (type)
-	   || type->code () == TYPE_CODE_ENUM)
+	   || type->code () == TYPE_CODE_ENUM
+	   || is_fixed_point_type (type))
     {
       /* If the type is a plain integer, then the access is
 	 straight-forward.  Otherwise we have to play around a bit
-- 
2.31.1

[PATCH 3/4] (RISCV) fix handling of fixed-point type return values

[Tom Tromey]

From: Joel Brobecker <brobecker@adacore.com>

This commit adds support for TYPE_CODE_FIXED_POINT types for
"finish" and "return" commands.

Consider the following Ada code...

   type FP1_Type is delta 0.1 range -1.0 .. +1.0; --  Ordinary
   function Call_FP1 (F : FP1_Type) return FP1_Type is
   begin
      FP1_Arg := F;
      return FP1_Arg;
   end Call_FP1;

... used as follow:

   F1 : FP1_Type := 1.0;
   F1 := Call_FP1 (F1);

"finish" currently behaves as follow:

    | (gdb) finish
    | [...]
    | Value returned is $1 = 0

We expect the returned value to be "1".

Similarly, "return" makes the function return the wrong value:

    | (gdb) return 1.0
    | Make pck.call_fp1 return now? (y or n) y
    | [...]
    | 9          F1 := Call_FP1 (F1);
    | (gdb) next
    | (gdb) print f1
    | $1 = 0.0625

(we expect it to print "1" instead).

This problem comes from the handling of integral return values
when the return value is actually fixed point type. Our type
here is actually a range of a fixed point type, but the same
principles should also apply to pure fixed-point types. For
the record, here is what the debugging info looks like:

 <1><238>: Abbrev Number: 2 (DW_TAG_subrange_type)
    <239>   DW_AT_lower_bound : -16
    <23a>   DW_AT_upper_bound : 16
    <23b>   DW_AT_name        : pck__fp1_type
    <23f>   DW_AT_type        : <0x248>

 <1><248>: Abbrev Number: 4 (DW_TAG_base_type)
    <249>   DW_AT_byte_size   : 1
    <24a>   DW_AT_encoding    : 13      (signed_fixed)
    <24b>   DW_AT_binary_scale: -4
    <24c>   DW_AT_name        : pck__Tfp1_typeB
    <250>   DW_AT_artificial  : 1

... where the scaling factor is 1/16.

Looking at the "finish" command, what happens is that riscv_arg_location
determines that our return value should be returned by parameter using
an integral convention (via builtin type long). And then,
riscv_return_value uses a cast to that builtin type long to
store the value of into a buffer with the right register size.
This doesn't work in our case, because the underlying value
returned by the function is unscaled, which means it is 16,
and thus the cast is like doing:

   arg_val = (FP1_Type) 16

... In other words, it is trying to create an FP1_Type enty whose
value is 16. Applying the scaling factor, that's 256, and because
the size of FP1_Type is 1 byte, we overflow and thus it ends up
being zero.

The same happen with the "return" function, but the other way around.

The fix consists in handling fixed-point types separately from
integral types.
---
 gdb/riscv-tdep.c | 52 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 3 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 4be89022437..bdbff183a55 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -2761,6 +2761,7 @@ riscv_arg_location (struct gdbarch *gdbarch,
     case TYPE_CODE_RANGE:
     case TYPE_CODE_ENUM:
     case TYPE_CODE_PTR:
+    case TYPE_CODE_FIXED_POINT:
       if (ainfo->length <= cinfo->xlen)
 	{
 	  ainfo->type = builtin_type (gdbarch)->builtin_long;
@@ -3135,8 +3136,32 @@ riscv_return_value (struct gdbarch  *gdbarch,
 	   buffers of sufficient size.  */
 	if (writebuf != nullptr)
 	  {
-	    struct value *arg_val = value_from_contents (arg_type, writebuf);
-	    abi_val = value_cast (info.type, arg_val);
+	    struct value *arg_val;
+
+	    if (is_fixed_point_type (arg_type))
+	      {
+		/* Convert the argument to the type used to pass
+		   the return value, but being careful to preserve
+		   the fact that the value needs to be returned
+		   unscaled.  */
+		gdb_mpz unscaled;
+
+		unscaled.read (gdb::make_array_view (writebuf,
+						     TYPE_LENGTH (arg_type)),
+			       type_byte_order (arg_type),
+			       arg_type->is_unsigned ());
+		abi_val = allocate_value (info.type);
+		unscaled.write
+		  (gdb::make_array_view (value_contents_raw (abi_val),
+					 TYPE_LENGTH (info.type)),
+		   type_byte_order (info.type),
+		   info.type->is_unsigned ());
+	      }
+	    else
+	      {
+		arg_val = value_from_contents (arg_type, writebuf);
+		abi_val = value_cast (info.type, arg_val);
+	      }
 	    writebuf = value_contents_raw (abi_val);
 	  }
 	else
@@ -3240,7 +3265,28 @@ riscv_return_value (struct gdbarch  *gdbarch,
 	   comment at the head of this block for more details.  */
 	if (readbuf != nullptr)
 	  {
-	    struct value *arg_val = value_cast (arg_type, abi_val);
+	    struct value *arg_val;
+
+	    if (is_fixed_point_type (arg_type))
+	      {
+		/* Convert abi_val to the actual return type, but
+		   being careful to preserve the fact that abi_val
+		   is unscaled.  */
+		gdb_mpz unscaled;
+
+		unscaled.read (gdb::make_array_view (value_contents (abi_val),
+						     TYPE_LENGTH (info.type)),
+			       type_byte_order (info.type),
+			       info.type->is_unsigned ());
+		arg_val = allocate_value (arg_type);
+		unscaled.write
+		  (gdb::make_array_view (value_contents_raw (arg_val),
+					 TYPE_LENGTH (arg_type)),
+		   type_byte_order (arg_type),
+		   arg_type->is_unsigned ());
+	      }
+	    else
+	      arg_val = value_cast (arg_type, abi_val);
 	    memcpy (old_readbuf, value_contents_raw (arg_val),
 		    TYPE_LENGTH (arg_type));
 	  }
-- 
2.31.1

[PATCH 4/4] (PPC64) fix handling of fixed-point values when using "return" command

[Tom Tromey]

From: Joel Brobecker <brobecker@adacore.com>

In the gdb.ada/fixed_points_function.exp testcase, we have the following
Ada code...

   type FP1_Type is delta 0.1 range -1.0 .. +1.0; --  Ordinary
   function Call_FP1 (F : FP1_Type) return FP1_Type is
   begin
      FP1_Arg := F;
      return FP1_Arg;
   end Call_FP1;

... used as follow:

   F1 : FP1_Type := 1.0;
   F1 := Call_FP1 (F1);

The testcase, among other things, verifies that "return" works
properly as follow:

    | (gdb) return 1.0
    | Make pck.call_fp1 return now? (y or n) y
    | [...]
    | 9          F1 := Call_FP1 (F1);
    | (gdb) next
    | (gdb) print f1
    | $1 = 0.0625

The output of the last command shows that we returned the wrong
value. The value printed gives a clue about the problem, since
it is 1/16th of the value we expected, where 1/16 is FP1_Type's
scaling factor.

The problem, here, comes from the fact that the function
handling return values for base types (ppc64_sysv_abi_return_value_base)
writes the return value using unpack_long which, upon seeing that
the value being unpacked is a fixed point type, applies the scaling
factor, to get the integer-representation of our fixed-point value
(similar to what it does with floats, for instance).

So, the fix consists in teaching ppc64_sysv_abi_return_value_base
about fixed-point types, and to avoid the unwanted application
of the scaling factor.

Note that the "finish" function, on the other hand, does not
suffer from this issue, simply becaue the value returned by
the function is read from register without the use of a type,
thus avoiding an unwanted application of a scaling factor.

No test added, as this change is already tested by
gdb.ada/fixed_points_function.exp.

Co-Authored-By: Tristan Gingold <gingold@adacore.com>
---
 gdb/ppc-sysv-tdep.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
index 3abb9877e80..8ae5f343526 100644
--- a/gdb/ppc-sysv-tdep.c
+++ b/gdb/ppc-sysv-tdep.c
@@ -1745,16 +1745,32 @@ ppc64_sysv_abi_return_value_base (struct gdbarch *gdbarch, struct type *valtype,
   if ((valtype->code () == TYPE_CODE_INT
        || valtype->code () == TYPE_CODE_ENUM
        || valtype->code () == TYPE_CODE_CHAR
-       || valtype->code () == TYPE_CODE_BOOL)
+       || valtype->code () == TYPE_CODE_BOOL
+       || valtype->code () == TYPE_CODE_RANGE
+       || is_fixed_point_type (valtype))
       && TYPE_LENGTH (valtype) <= 8)
     {
       int regnum = tdep->ppc_gp0_regnum + 3 + index;
 
       if (writebuf != NULL)
 	{
+	  LONGEST return_val;
+
+	  if (is_fixed_point_type (valtype))
+	    {
+	      /* Fixed point type values need to be returned unscaled.  */
+	      gdb_mpz unscaled;
+
+	      unscaled.read ({writebuf, TYPE_LENGTH (valtype)},
+			     type_byte_order (valtype),
+			     valtype->is_unsigned ());
+	      return_val = unscaled.as_integer<LONGEST> ();
+	    }
+	  else
+	    return_val = unpack_long (valtype, writebuf);
+
 	  /* Be careful to sign extend the value.  */
-	  regcache_cooked_write_unsigned (regcache, regnum,
-					  unpack_long (valtype, writebuf));
+	  regcache_cooked_write_unsigned (regcache, regnum, return_val);
 	}
       if (readbuf != NULL)
 	{
-- 
2.31.1

Re: [PATCH 0/4] Fixed-point ABI fixes

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> This series fixes some ABI issues involving fixed-point types.
Tom> The patches weren't written by me -- I'm just submitting them.
Tom> They've been in use at AdaCore for a while, one of them since 2017,
Tom> with the last one being written in December 2020.

Tom> A fixed-point type is really just an integer with a special
Tom> interpretation.  I think targets should normally treat it like an
Tom> integer for ABI purposes.  A bit of care may be needed to ensure the
Tom> correct bits end up being passed, depending on how the
Tom> argument-passing code is written.

I've rebased this and updated it where necessary (just riscv-tdep).
I'm checking it in now.

Tom