[PATCH][gdb/c] Fix printing -2147483648

[PATCH][gdb/c] Fix printing -2147483648

[Tom de Vries]

Hi,

Currently gdb prints for "p -2147483648":
...
$1 = 2147483648
...

The problem is that the type of 2147483648 is unsigned int instead of long.

Fix this by making sure the type of 2147483648 is long, such that we have instead:
$1 = -2147483648
...

Tested on x86_64-linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=16377

Any comments?

Thanks,
- Tom

[gdb/c] Fix printing -2147483648

---
 gdb/c-exp.y                             | 114 +++++++++++++++++---------------
 gdb/testsuite/gdb.base/parse_number.exp |  37 ++++++++++-
 2 files changed, 93 insertions(+), 58 deletions(-)

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 72f8dd32d93..a015103e2b1 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1917,6 +1917,40 @@ check_parameter_typelist (std::vector<struct type *> *params)
     }
 }
 
+static bool
+fits_in_type (int n_sign, ULONGEST n, int type_bits, bool type_signed_p)
+{
+  /* Normalize n_sign.  */
+  if (n == 0)
+    n_sign = 1;
+
+  if (n_sign == -1 && !type_signed_p)
+    /* Can't fit a negative number in an unsigned type.  */
+    return false;
+
+  if (type_bits > sizeof (ULONGEST) * 8)
+    return true;
+
+  ULONGEST smax = (ULONGEST)1 << (type_bits - 1);
+  if (n_sign == -1)
+    {
+      /* Negative number, signed type.  */
+      return (n <= smax);
+    }
+  else if (n_sign == 1 && type_signed_p)
+    {
+      /* Positive number, signed type.  */
+      return (n < smax);
+    }
+  else if (n_sign == 1 && !type_signed_p)
+    {
+      /* Positive number, unsigned type.  */
+      return ((n >> 1) >> (type_bits - 1)) == 0;
+    }
+  else
+    gdb_assert_not_reached ("");
+}
+
 /* Take care of parsing a number (anything that starts with a digit).
    Set yylval and return the token type; update lexptr.
    LEN is the number of characters in it.  */
@@ -1929,7 +1963,6 @@ parse_number (struct parser_state *par_state,
 {
   ULONGEST n = 0;
   ULONGEST prevn = 0;
-  ULONGEST un;
 
   int i = 0;
   int c;
@@ -1945,9 +1978,6 @@ parse_number (struct parser_state *par_state,
   /* We have found a "L" or "U" (or "i") suffix.  */
   int found_suffix = 0;
 
-  ULONGEST high_bit;
-  struct type *signed_type;
-  struct type *unsigned_type;
   char *p;
 
   p = (char *) alloca (len);
@@ -2123,58 +2153,32 @@ parse_number (struct parser_state *par_state,
      or gdbarch_long_bit will be that big, sometimes not.  To deal with
      the case where it is we just always shift the value more than
      once, with fewer bits each time.  */
-
-  un = n >> 2;
-  if (long_p == 0
-      && (un >> (gdbarch_int_bit (par_state->gdbarch ()) - 2)) == 0)
-    {
-      high_bit
-	= ((ULONGEST)1) << (gdbarch_int_bit (par_state->gdbarch ()) - 1);
-
-      /* A large decimal (not hex or octal) constant (between INT_MAX
-	 and UINT_MAX) is a long or unsigned long, according to ANSI,
-	 never an unsigned int, but this code treats it as unsigned
-	 int.  This probably should be fixed.  GCC gives a warning on
-	 such constants.  */
-
-      unsigned_type = parse_type (par_state)->builtin_unsigned_int;
-      signed_type = parse_type (par_state)->builtin_int;
-    }
-  else if (long_p <= 1
-	   && (un >> (gdbarch_long_bit (par_state->gdbarch ()) - 2)) == 0)
-    {
-      high_bit
-	= ((ULONGEST)1) << (gdbarch_long_bit (par_state->gdbarch ()) - 1);
-      unsigned_type = parse_type (par_state)->builtin_unsigned_long;
-      signed_type = parse_type (par_state)->builtin_long;
-    }
+  int int_bits = gdbarch_int_bit (par_state->gdbarch ());
+  int long_bits = gdbarch_long_bit (par_state->gdbarch ());
+  int long_long_bits = gdbarch_long_long_bit (par_state->gdbarch ());
+  bool have_signed = !unsigned_p;
+  bool have_unsigned = unsigned_p || base != 10;
+  bool have_int = long_p == 0;
+  bool have_long = long_p <= 1;
+  if (have_int && have_signed && fits_in_type (1, n, int_bits, 1))
+    putithere->typed_val_int.type = parse_type (par_state)->builtin_int;
+  else if (have_int && have_unsigned && fits_in_type (1, n, int_bits, 0))
+    putithere->typed_val_int.type
+      = parse_type (par_state)->builtin_unsigned_int;
+  else if (have_long && have_signed && fits_in_type (1, n, long_bits, 1))
+    putithere->typed_val_int.type = parse_type (par_state)->builtin_long;
+  else if (have_long && have_unsigned && fits_in_type (1, n, long_bits, 0))
+    putithere->typed_val_int.type
+      = parse_type (par_state)->builtin_unsigned_long;
+  else if (have_signed && fits_in_type (1, n, long_long_bits, 1))
+    putithere->typed_val_int.type
+      = parse_type (par_state)->builtin_long_long;
+  else if (have_unsigned && fits_in_type (1, n, long_long_bits, 0))
+    putithere->typed_val_int.type
+      = parse_type (par_state)->builtin_unsigned_long_long;
   else
-    {
-      int shift;
-      if (sizeof (ULONGEST) * HOST_CHAR_BIT
-	  < gdbarch_long_long_bit (par_state->gdbarch ()))
-	/* A long long does not fit in a LONGEST.  */
-	shift = (sizeof (ULONGEST) * HOST_CHAR_BIT - 1);
-      else
-	shift = (gdbarch_long_long_bit (par_state->gdbarch ()) - 1);
-      high_bit = (ULONGEST) 1 << shift;
-      unsigned_type = parse_type (par_state)->builtin_unsigned_long_long;
-      signed_type = parse_type (par_state)->builtin_long_long;
-    }
-
-   putithere->typed_val_int.val = n;
-
-   /* If the high bit of the worked out type is set then this number
-      has to be unsigned. */
-
-   if (unsigned_p || (n & high_bit))
-     {
-       putithere->typed_val_int.type = unsigned_type;
-     }
-   else
-     {
-       putithere->typed_val_int.type = signed_type;
-     }
+    error (_("Numeric constant too large."));
+  putithere->typed_val_int.val = n;
 
    if (imaginary_p)
      putithere->typed_val_int.type
diff --git a/gdb/testsuite/gdb.base/parse_number.exp b/gdb/testsuite/gdb.base/parse_number.exp
index 444f5d0534b..b28b203c24c 100644
--- a/gdb/testsuite/gdb.base/parse_number.exp
+++ b/gdb/testsuite/gdb.base/parse_number.exp
@@ -25,6 +25,19 @@ proc hex_for_lang { lang val } {
     return $val
 }
 
+proc c_like { lang } {
+    set res 0
+    switch $lang {
+	c
+	- c++
+	- asm
+	- objective-c
+	- opencl
+	- minimal {set res 1}
+    }
+    return $res
+}
+
 # Test parsing numbers.  Several language parsers had the same bug
 # around parsing large 64-bit numbers, hitting undefined behavior, and
 # thus crashing a GDB built with UBSan.  This testcase goes over all
@@ -62,19 +75,27 @@ proc test_parse_numbers {arch} {
     }
 
     foreach_with_prefix lang $::all_languages {
-	if { $lang == "unknown" } {
+	if { $lang == "unknown" || $lang == "auto" || $lang == "local" } {
 	    continue
 	}
 
 	gdb_test_no_output "set language $lang"
 
+	if { $lang == "modula-2" || $lang == "fortran" } {
+	    set re_overflow "Overflow on numeric constant\\."
+	} elseif { $lang == "ada" } {
+	    set re_overflow "Integer literal out of range"
+	} else {
+	    set re_overflow "Numeric constant too large\\."
+	}
+
 	set val "0xffffffffffffffff"
 	set val [hex_for_lang $lang $val]
 	if {$lang == "fortran"} {
 	    gdb_test "p/x $val" " = $fortran_value"
 	    gdb_test "ptype $val" " = $fortran_type"
-	} elseif {$lang == "modula-2"} {
-	    gdb_test "p/x $val" "Overflow on numeric constant\\."
+	} elseif {$lang == "modula-2" || ($sizeof_long_long == 4 && [c_like $lang]) } {
+	    gdb_test "p/x $val" $re_overflow
 	} else {
 	    # D and Rust define their own built-in 64-bit types, and
 	    # are thus always able to parse/print 64-bit values.
@@ -98,6 +119,16 @@ proc test_parse_numbers {arch} {
 		gdb_test "ptype $val" " = $8B_type"
 	    }
 	}
+
+	if { [c_like $lang] } {
+	    set val -2147483648
+	    if { $sizeof_long_long == 4 } {
+		set re $re_overflow
+	    } else {
+		set re $val
+	    }
+	    gdb_test "p $val" $re
+	}
     }
 }
 

Re: [PATCH][gdb/c] Fix printing -2147483648

[Andreas Schwab]

On Mai 18 2022, Tom de Vries via Gdb-patches wrote:

> +static bool
> +fits_in_type (int n_sign, ULONGEST n, int type_bits, bool type_signed_p)

n_sign is always 1, you can just remove it.

> +  if (have_int && have_signed && fits_in_type (1, n, int_bits, 1))

type_signed_p is a bool, so you should use true and false.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH][gdb/c] Fix printing -2147483648

[Pedro Alves]

On 2022-05-18 18:38, Tom de Vries wrote:

> +	if { [c_like $lang] } {
> +	    set val -2147483648
> +	    if { $sizeof_long_long == 4 } {
> +		set re $re_overflow
> +	    } else {
> +		set re $val
> +	    }
> +	    gdb_test "p $val" $re
> +	}

Could you add ptype tests too?  As is, is plausible for GDB to manage to print
the expected value, but incorrectly pick long long instead of long, for example.

Re: [PATCH][gdb/c] Fix printing -2147483648

[Tom de Vries]

On 5/18/22 21:00, Andreas Schwab wrote:
> On Mai 18 2022, Tom de Vries via Gdb-patches wrote:
> 
>> +static bool
>> +fits_in_type (int n_sign, ULONGEST n, int type_bits, bool type_signed_p)
> 
> n_sign is always 1, you can just remove it.
> 

I ended up using this function here ( 
https://sourceware.org/pipermail/gdb-patches/2022-May/189313.html ) with 
n_sign also being -1.

>> +  if (have_int && have_signed && fits_in_type (1, n, int_bits, 1))
> 
> type_signed_p is a bool, so you should use true and false.
> 

Ack, fixed in resubmission here ( 
https://sourceware.org/pipermail/gdb-patches/2022-May/189311.html ).

Thanks,
- Tom

Re: [PATCH][gdb/c] Fix printing -2147483648

[Tom de Vries]

On 5/18/22 21:15, Pedro Alves wrote:
> On 2022-05-18 18:38, Tom de Vries wrote:
> 
>> +	if { [c_like $lang] } {
>> +	    set val -2147483648
>> +	    if { $sizeof_long_long == 4 } {
>> +		set re $re_overflow
>> +	    } else {
>> +		set re $val
>> +	    }
>> +	    gdb_test "p $val" $re
>> +	}
> 
> Could you add ptype tests too?  As is, is plausible for GDB to manage to print
> the expected value, but incorrectly pick long long instead of long, for example.

Indeed, good point.

I've ended up extending the test-case in a more structural way, 
submitted here ( 
https://sourceware.org/pipermail/gdb-patches/2022-May/189309.html ), and 
that should address your comment.

Thanks,
- Tom