[PATCH] Don't inherit range-type signed-ness from underlying type

[PATCH] Don't inherit range-type signed-ness from underlying type

[Tom Tromey]

A recent commit changed gdb to inherit the signed-ness of a range type
from its underlying type:

    commit cfabbd351a174406fd5aa063303f5c8bf9266bbc
    Author: Tom Tromey <tom@tromey.com>
    Date:   Sat Oct 17 11:41:59 2020 -0600

      Make range types inherit signed-ness from base type

This passed testing -- but unfortunately, additional testing at
AdaCore showed that this change was incorrect.  GNAT, at least, can
emit an unsigned range type whose underlying type is signed.

This patch reverts the code change from the above.  I chose not to
reintroduce the FIXME comments, because now we know that they are
incorrect.  Instead, this patch also adds a comment to
create_range_type.

A new test case is included as well.

gdb/ChangeLog
2020-10-19  Tom Tromey  <tromey@adacore.com>

	* gdbtypes.c (create_range_type): Revert previous patch.  Add
	comment.

gdb/testsuite/ChangeLog
2020-10-19  Tom Tromey  <tromey@adacore.com>

	* gdb.ada/unsigned_range/foo.adb: New file.
	* gdb.ada/unsigned_range/pack.adb: New file.
	* gdb.ada/unsigned_range/pack.ads: New file.
	* gdb.ada/unsigned_range.exp: New file.
---
 gdb/ChangeLog                                 |  5 +++
 gdb/gdbtypes.c                                | 16 +++++++-
 gdb/testsuite/ChangeLog                       |  7 ++++
 gdb/testsuite/gdb.ada/unsigned_range.exp      | 32 +++++++++++++++
 gdb/testsuite/gdb.ada/unsigned_range/foo.adb  | 39 +++++++++++++++++++
 gdb/testsuite/gdb.ada/unsigned_range/pack.adb | 23 +++++++++++
 gdb/testsuite/gdb.ada/unsigned_range/pack.ads | 19 +++++++++
 7 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.ada/unsigned_range.exp
 create mode 100644 gdb/testsuite/gdb.ada/unsigned_range/foo.adb
 create mode 100644 gdb/testsuite/gdb.ada/unsigned_range/pack.adb
 create mode 100644 gdb/testsuite/gdb.ada/unsigned_range/pack.ads

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index e7d9e4cef3e..2c3ef65e13d 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -950,7 +950,21 @@ create_range_type (struct type *result_type, struct type *index_type,
 
   result_type->set_bounds (bounds);
 
-  result_type->set_is_unsigned (index_type->is_unsigned ());
+  /* Note that the signed-ness of a range type can't simply be copied
+     from the underlying type.  GNAT, at least, can emit an unsigned
+     range whose underlying type is signed.  This matters if values
+     from the range are stored in a bitfield, because in this case
+     sign extension would result in the wrong value.  So, we have some
+     heuristics here instead.  */
+  if (low_bound->kind () == PROP_CONST && low_bound->const_val () >= 0)
+    result_type->set_is_unsigned (true);
+  /* Ada allows the declaration of range types whose upper bound is
+     less than the lower bound, so checking the lower bound is not
+     enough.  Make sure we do not mark a range type whose upper bound
+     is negative as unsigned.  */
+  if (high_bound->kind () == PROP_CONST && high_bound->const_val () < 0)
+    result_type->set_is_unsigned (false);
+
   result_type->set_endianity_is_not_default
     (index_type->endianity_is_not_default ());
 
diff --git a/gdb/testsuite/gdb.ada/unsigned_range.exp b/gdb/testsuite/gdb.ada/unsigned_range.exp
new file mode 100644
index 00000000000..476aa8ba3c9
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/unsigned_range.exp
@@ -0,0 +1,32 @@
+# Copyright 2020 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 foo
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable {debug}] != ""} {
+    return -1
+}
+
+clean_restart ${testfile}
+
+set bp_location [gdb_get_line_number "BREAK" ${testdir}/foo.adb]
+runto "foo.adb:$bp_location"
+
+gdb_test "print Value" \
+    [string_to_regexp " = (one => 8000, two => 51000)"]
diff --git a/gdb/testsuite/gdb.ada/unsigned_range/foo.adb b/gdb/testsuite/gdb.ada/unsigned_range/foo.adb
new file mode 100644
index 00000000000..52c669acc8b
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/unsigned_range/foo.adb
@@ -0,0 +1,39 @@
+--  Copyright 2020 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 Pack; use Pack;
+
+procedure Foo is
+   type U_W is range 0 .. 65535;
+   for  U_W'Size use 16;
+
+   type R_C is
+   record
+      One : U_W;
+      Two : U_W;
+   end record;
+
+   for R_C use
+   record at mod 2;
+      One at 0 range  0 .. 15;
+      Two at 2 range  0 .. 15;
+   end record;
+   for R_C'size use 2*16;
+
+   Value: R_C := (One => 8000, Two => 51000);
+
+begin
+   Do_Nothing (Value'Address); --  BREAK
+end Foo;
diff --git a/gdb/testsuite/gdb.ada/unsigned_range/pack.adb b/gdb/testsuite/gdb.ada/unsigned_range/pack.adb
new file mode 100644
index 00000000000..626ea6044ea
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/unsigned_range/pack.adb
@@ -0,0 +1,23 @@
+--  Copyright 2020 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 Pack is
+
+   procedure Do_Nothing (A : System.Address) is
+   begin
+      null;
+   end Do_Nothing;
+
+end Pack;
diff --git a/gdb/testsuite/gdb.ada/unsigned_range/pack.ads b/gdb/testsuite/gdb.ada/unsigned_range/pack.ads
new file mode 100644
index 00000000000..f744c538b47
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/unsigned_range/pack.ads
@@ -0,0 +1,19 @@
+--  Copyright 2020 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 System;
+package Pack is
+   procedure Do_Nothing (A : System.Address);
+end Pack;
-- 
2.26.2

Re: [PATCH] Don't inherit range-type signed-ness from underlying type

[Luis Machado]

Hi,

On 10/19/20 4:33 PM, Tom Tromey wrote:
> A recent commit changed gdb to inherit the signed-ness of a range type
> from its underlying type:
> 
>      commit cfabbd351a174406fd5aa063303f5c8bf9266bbc
>      Author: Tom Tromey <tom@tromey.com>
>      Date:   Sat Oct 17 11:41:59 2020 -0600
> 
>        Make range types inherit signed-ness from base type
> 
> This passed testing -- but unfortunately, additional testing at
> AdaCore showed that this change was incorrect.  GNAT, at least, can
> emit an unsigned range type whose underlying type is signed.

Is this correct compiler behavior though? It sounds inconsistent. If the 
compiler wants an unsigned type for a range, intuitively it should use 
an unsigned base type, no?

Otherwise this may lead to confusion.

Re: [PATCH] Don't inherit range-type signed-ness from underlying type

[Tom Tromey]

>> This passed testing -- but unfortunately, additional testing at
>> AdaCore showed that this change was incorrect.  GNAT, at least, can
>> emit an unsigned range type whose underlying type is signed.

Luis> Is this correct compiler behavior though? It sounds inconsistent. If
Luis> the compiler wants an unsigned type for a range, intuitively it should
Luis> use an unsigned base type, no?

Good question.  I'll update the comment in the patch with the answer.

The test case shows the problem -- it has a type whose range is 0..65535
(inclusive), but whose underlying type is a signed integer.  Also, the
program requests that values of this range type be stored in 16 bits.
This is fine because the range of values fits.

However, if the type inherits the sign from the underlying type, if we
fetch such a value, it will be incorrectly sign-extended.

I do think it's the case, though, that if the underlying type is
unsigned then the range type must be.  We could consider adding that.

Tom

Re: [PATCH] Don't inherit range-type signed-ness from underlying type

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> 2020-10-19  Tom Tromey  <tromey@adacore.com>

Tom> 	* gdbtypes.c (create_range_type): Revert previous patch.  Add
Tom> 	comment.

I'm checking this in, with an updated comment.

Tom