[PATCH] Make range types inherit signed-ness from base type

[PATCH] Make range types inherit signed-ness from base type

[Tom Tromey]

I ran across this comment in valprint.c:

      /* FIXME: create_static_range_type does not set the unsigned bit in a
         range type (I think it probably should copy it from the target
         type), so we won't print values which are too large to
         fit in a signed integer correctly.  */

It seems to me that a range type ought to inherit its signed-ness from
the underlying type, so this patch implements this change, and removes
the comment.  (It was also copied into m2-valprint.c.)

I also remove the comment about handling ranges of enums, because I
think that comment is incorrect.

gdb/ChangeLog
2020-08-22  Tom Tromey  <tom@tromey.com>

	* valprint.c (generic_value_print): Remove comment.
	* m2-valprint.c (m2_value_print_inner): Remove comment.
	* gdbtypes.c (create_range_type): Set TYPE_UNSIGNED from base
	type.
---
 gdb/ChangeLog     |  7 +++++++
 gdb/gdbtypes.c    | 11 +----------
 gdb/m2-valprint.c |  7 -------
 gdb/valprint.c    | 10 ----------
 4 files changed, 8 insertions(+), 27 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index c1eb03d8984..d96e91aee7d 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -944,16 +944,7 @@ create_range_type (struct type *result_type, struct type *index_type,
 
   result_type->set_bounds (bounds);
 
-  if (low_bound->kind () == PROP_CONST && low_bound->const_val () >= 0)
-    TYPE_UNSIGNED (result_type) = 1;
-
-  /* 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)
-    TYPE_UNSIGNED (result_type) = 0;
-
+  TYPE_UNSIGNED (result_type) = TYPE_UNSIGNED (index_type);
   TYPE_ENDIANITY_NOT_DEFAULT (result_type)
     = TYPE_ENDIANITY_NOT_DEFAULT (index_type);
 
diff --git a/gdb/m2-valprint.c b/gdb/m2-valprint.c
index b0a3ce3ec3e..0a67223f79a 100644
--- a/gdb/m2-valprint.c
+++ b/gdb/m2-valprint.c
@@ -448,13 +448,6 @@ m2_value_print_inner (struct value *val, struct ui_file *stream, int recurse,
 	  m2_value_print_inner (v, stream, recurse, options);
 	  break;
 	}
-      /* FIXME: create_static_range_type does not set the unsigned bit in a
-         range type (I think it probably should copy it from the target
-         type), so we won't print values which are too large to
-         fit in a signed integer correctly.  */
-      /* FIXME: Doesn't handle ranges of enums correctly.  (Can't just
-         print with the target type, though, because the size of our type
-         and the target type might differ).  */
       /* FALLTHROUGH */
 
     case TYPE_CODE_REF:
diff --git a/gdb/valprint.c b/gdb/valprint.c
index db98ca2abc9..3ac23df5b12 100644
--- a/gdb/valprint.c
+++ b/gdb/valprint.c
@@ -869,16 +869,6 @@ generic_value_print (struct value *val, struct ui_file *stream, int recurse,
       break;
 
     case TYPE_CODE_RANGE:
-      /* FIXME: create_static_range_type does not set the unsigned bit in a
-         range type (I think it probably should copy it from the
-         target type), so we won't print values which are too large to
-         fit in a signed integer correctly.  */
-      /* FIXME: Doesn't handle ranges of enums correctly.  (Can't just
-         print with the target type, though, because the size of our
-         type and the target type might differ).  */
-
-      /* FALLTHROUGH */
-
     case TYPE_CODE_INT:
       generic_value_print_int (val, stream, options);
       break;
-- 
2.17.2

Re: [PATCH] Make range types inherit signed-ness from base type

[Simon Marchi]

On 2020-08-22 5:07 p.m., Tom Tromey wrote:
> I ran across this comment in valprint.c:
>
>       /* FIXME: create_static_range_type does not set the unsigned bit in a
>          range type (I think it probably should copy it from the target
>          type), so we won't print values which are too large to
>          fit in a signed integer correctly.  */
>
> It seems to me that a range type ought to inherit its signed-ness from
> the underlying type, so this patch implements this change, and removes
> the comment.  (It was also copied into m2-valprint.c.)
>
> I also remove the comment about handling ranges of enums, because I
> think that comment is incorrect.

This looks OK to me, although I have to admit I don't really understand
the impact.

Simon

Re: [PATCH] Make range types inherit signed-ness from base type

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> On 2020-08-22 5:07 p.m., Tom Tromey wrote:
>> I ran across this comment in valprint.c:
>> 
>> /* FIXME: create_static_range_type does not set the unsigned bit in a
>> range type (I think it probably should copy it from the target
>> type), so we won't print values which are too large to
>> fit in a signed integer correctly.  */
>> 
>> It seems to me that a range type ought to inherit its signed-ness from
>> the underlying type, so this patch implements this change, and removes
>> the comment.  (It was also copied into m2-valprint.c.)
>> 
>> I also remove the comment about handling ranges of enums, because I
>> think that comment is incorrect.

Simon> This looks OK to me, although I have to admit I don't really understand
Simon> the impact.

Thanks.  I re-regression-tested this, and I'm checking it in now.

Tom

Re: [PATCH] Make range types inherit signed-ness from base type

[Tom Tromey]

Tom> Thanks.  I re-regression-tested this, and I'm checking it in now.

Testing this using the AdaCore internal test suite shows it is flawed :(

In particular the compiler will sometimes emit a range that ought to be
interpreted as unsigned, but where the underlying type is signed:

 <1><15ea>: Abbrev Number: 3 (DW_TAG_subrange_type)
    <15eb>   DW_AT_byte_size   : 2
    <15ec>   DW_AT_lower_bound : 0
    <15ed>   DW_AT_upper_bound : 65535
    <15f0>   DW_AT_name        : (indirect string, offset: 0x2b19): example_data_types__integer16___XDLU_0__65535
    <15f4>   DW_AT_type        : <0x15fd>
[...]
 <1><15fd>: Abbrev Number: 5 (DW_TAG_base_type)
    <15fe>   DW_AT_byte_size   : 4
    <15ff>   DW_AT_encoding    : 5	(signed)
    <1600>   DW_AT_name        : (indirect string, offset: 0x2ccd): example_data_types__Tinteger16B
    <1604>   DW_AT_artificial  : 1


My plan now is to revert this once I write a test case.  I don't plan to
restore the old FIXME comments, but instead I'll add a comment in
create_range_type.

Tom