[PATCH 1/5] Restore bitshift.exp tests

[PATCH 1/5] Restore bitshift.exp tests

[Hannes Domani]

Commit cdd4206647 unintentionally disabled all tests of bitshift.exp,
so it actually just does this:

Running /c/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/bitshift.exp ...
PASS: gdb.base/bitshift.exp: complete set language

		=== gdb Summary ===

 # of expected passes		1

It changed the 'continue' of unsupported languages to 'return', and
since ada is the first language and is unsupported, no tests were run.

This changes it back to 'continue', and the following patches fix
the regressions that were introduced since then unnoticed.
---
 gdb/testsuite/gdb.base/bitshift.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/bitshift.exp b/gdb/testsuite/gdb.base/bitshift.exp
index cfb1e7b9820..61c7eca2747 100644
--- a/gdb/testsuite/gdb.base/bitshift.exp
+++ b/gdb/testsuite/gdb.base/bitshift.exp
@@ -178,7 +178,7 @@ proc test_shifts {} {
 	    "unknown" "ada" "modula-2" "pascal" "fortran"
 	}
 	if {[lsearch -exact $skip_langs $lang] >= 0} {
-	    return
+	    continue
 	}
 
 	gdb_test_no_output "set language $lang"
-- 
2.35.1

[PATCH 2/5] Fix right shift of negative numbers

[Hannes Domani]

PR31590 shows that right shift of negative numbers doesn't work
correctly since GDB 14:

(gdb) p (-3) >> 1
$1 = -1

GDB 13 and earlier returned the correct value -2.
And there actually is one test that shows the failure:

print -1 >> 1
$84 = 0
(gdb) FAIL: gdb.base/bitshift.exp: lang=asm: rsh neg lhs: print -1 >> 1

The problem was introduced with the change to gmp functions in
commit 303a881f87.
It's wrong because gdb_mpz::operator>> uses mpz_tdif_q_2exp, which
always rounds toward zero, and the gmp docu says this:

For positive n both mpz_fdiv_q_2exp and mpz_tdiv_q_2exp are simple
bitwise right shifts.
For negative n, mpz_fdiv_q_2exp is effectively an arithmetic right shift
treating n as two¿s complement the same as the bitwise logical functions
do, whereas mpz_tdiv_q_2exp effectively treats n as sign and magnitude.

So this changes mpz_tdiv_q_2exp to mpz_fdiv_q_2exp, since it
does right shifts for both positive and negative numbers.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31590
---
 gdb/gmp-utils.h                     | 4 ++--
 gdb/testsuite/gdb.base/bitshift.exp | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/gmp-utils.h b/gdb/gmp-utils.h
index 51e06abc050..878ce1da43a 100644
--- a/gdb/gmp-utils.h
+++ b/gdb/gmp-utils.h
@@ -280,13 +280,13 @@ struct gdb_mpz
   gdb_mpz operator>> (unsigned long nbits) const
   {
     gdb_mpz result;
-    mpz_tdiv_q_2exp (result.m_val, m_val, nbits);
+    mpz_fdiv_q_2exp (result.m_val, m_val, nbits);
     return result;
   }
 
   gdb_mpz &operator>>= (unsigned long nbits)
   {
-    mpz_tdiv_q_2exp (m_val, m_val, nbits);
+    mpz_fdiv_q_2exp (m_val, m_val, nbits);
     return *this;
   }
 
diff --git a/gdb/testsuite/gdb.base/bitshift.exp b/gdb/testsuite/gdb.base/bitshift.exp
index 61c7eca2747..17f6b78fed2 100644
--- a/gdb/testsuite/gdb.base/bitshift.exp
+++ b/gdb/testsuite/gdb.base/bitshift.exp
@@ -344,6 +344,8 @@ proc test_shifts {} {
 	with_test_prefix "rsh neg lhs" {
 	    test_shift $lang "print -1 >> 0" " = -1"
 	    test_shift $lang "print -1 >> 1" " = -1"
+	    test_shift $lang "print -2 >> 1" " = -1"
+	    test_shift $lang "print -3 >> 1" " = -2"
 	    test_shift $lang "print -8 >> 1" " = -4"
 	    test_shift $lang "print [make_int64 $lang -8] >> 1" " = -4"
 	}
-- 
2.35.1

[PATCH 3/5] Fix too-large or negative right shift of negative numbers

[Hannes Domani]

As seen in these test failures:

print -1 >> -1
warning: right shift count is negative
$N = 0
(gdb) FAIL: gdb.base/bitshift.exp: lang=c: neg lhs/rhs: print -1 >> -1
print -4 >> -2
warning: right shift count is negative
$N = 0
(gdb) FAIL: gdb.base/bitshift.exp: lang=c: neg lhs/rhs: print -4 >> -2

Fixed by restoring the logic from before the switch to gmp.
---
 gdb/testsuite/gdb.base/bitshift.exp |  1 +
 gdb/valarith.c                      | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/bitshift.exp b/gdb/testsuite/gdb.base/bitshift.exp
index 17f6b78fed2..d6e252102d9 100644
--- a/gdb/testsuite/gdb.base/bitshift.exp
+++ b/gdb/testsuite/gdb.base/bitshift.exp
@@ -348,6 +348,7 @@ proc test_shifts {} {
 	    test_shift $lang "print -3 >> 1" " = -2"
 	    test_shift $lang "print -8 >> 1" " = -4"
 	    test_shift $lang "print [make_int64 $lang -8] >> 1" " = -4"
+	    test_rshift_tl $lang "print -8 >> 100" " = -1"
 	}
 
 	# Make sure an unsigned 64-bit value with high bit set isn't
diff --git a/gdb/valarith.c b/gdb/valarith.c
index 7034fa6096b..6160b0e4438 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -1303,7 +1303,21 @@ scalar_binop (struct value *arg1, struct value *arg2, enum exp_opcode op)
 	  {
 	    unsigned long nbits;
 	    if (!check_valid_shift_count (op, result_type, type2, v2, nbits))
-	      v = 0;
+	      {
+		/* Pretend the too-large shift was decomposed in a
+		   number of smaller shifts.  An arithmetic signed
+		   right shift of a negative number always yields -1
+		   with such semantics.  This is the right thing to
+		   do for Go, and we might as well do it for
+		   languages where it is undefined.  Also, pretend a
+		   shift by a negative number was a shift by the
+		   negative number cast to unsigned, which is the
+		   same as shifting by a too-large number.  */
+		if (v1 < 0 && !result_type->is_unsigned ())
+		  v = -1;
+		else
+		  v = 0;
+	      }
 	    else
 	      v = v1 >> nbits;
 	  }
-- 
2.35.1

[PATCH 4/5] Fix 64-bit shifts where long only has 32-bit size

[Hannes Domani]

On systems where long has 32-bit size you get these failures:

print 1 << (unsigned long long) 0xffffffffffffffff
Cannot export value 18446744073709551615 as 32-bits unsigned integer (must be between 0 and 4294967295)
(gdb) FAIL: gdb.base/bitshift.exp: lang=c: max-uint64: print 1 << (unsigned long long) 0xffffffffffffffff
print 1 >> (unsigned long long) 0xffffffffffffffff
Cannot export value 18446744073709551615 as 32-bits unsigned integer (must be between 0 and 4294967295)
(gdb) FAIL: gdb.base/bitshift.exp: lang=c: max-uint64: print 1 >> (unsigned long long) 0xffffffffffffffff
print -1 << (unsigned long long) 0xffffffffffffffff
Cannot export value 18446744073709551615 as 32-bits unsigned integer (must be between 0 and 4294967295)
(gdb) FAIL: gdb.base/bitshift.exp: lang=c: max-uint64: print -1 << (unsigned long long) 0xffffffffffffffff
print -1 >> (unsigned long long) 0xffffffffffffffff
Cannot export value 18446744073709551615 as 32-bits unsigned integer (must be between 0 and 4294967295)
(gdb) FAIL: gdb.base/bitshift.exp: lang=c: max-uint64: print -1 >> (unsigned long long) 0xffffffffffffffff

Fixed by changing number-of-bits variable to ULONGEST.
---
 gdb/testsuite/gdb.base/bitshift.exp | 12 ++++++++++++
 gdb/valarith.c                      |  8 ++++----
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/gdb.base/bitshift.exp b/gdb/testsuite/gdb.base/bitshift.exp
index d6e252102d9..cab82e1971e 100644
--- a/gdb/testsuite/gdb.base/bitshift.exp
+++ b/gdb/testsuite/gdb.base/bitshift.exp
@@ -363,6 +363,18 @@ proc test_shifts {} {
 	    test_rshift_tl $lang \
 		"print -1 >> [make_uint64 $lang 0xffffffffffffffff]" " = -1"
 	}
+
+	# Check if shift value isn't silently truncated to 32bit.
+	with_test_prefix "lower-32bit-zero" {
+	    test_lshift_tl $lang \
+		"print 1 << [make_uint64 $lang 0x100000000]" " = 0"
+	    test_rshift_tl $lang \
+		"print 1 >> [make_uint64 $lang 0x100000000]" " = 0"
+	    test_lshift_tl $lang \
+		"print -1 << [make_uint64 $lang 0x100000000]" " = 0"
+	    test_rshift_tl $lang \
+		"print -1 >> [make_uint64 $lang 0x100000000]" " = -1"
+	}
     }
 }
 
diff --git a/gdb/valarith.c b/gdb/valarith.c
index 6160b0e4438..a95d1088133 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -1086,7 +1086,7 @@ type_length_bits (type *type)
 static bool
 check_valid_shift_count (enum exp_opcode op, type *result_type,
 			 type *shift_count_type, const gdb_mpz &shift_count,
-			 unsigned long &nbits)
+			 ULONGEST &nbits)
 {
   if (!shift_count_type->is_unsigned ())
     {
@@ -1112,7 +1112,7 @@ check_valid_shift_count (enum exp_opcode op, type *result_type,
 	}
     }
 
-  nbits = shift_count.as_integer<unsigned long> ();
+  nbits = shift_count.as_integer<ULONGEST> ();
   if (nbits >= type_length_bits (result_type))
     {
       /* In Go, shifting by large amounts is defined.  Be silent and
@@ -1291,7 +1291,7 @@ scalar_binop (struct value *arg1, struct value *arg2, enum exp_opcode op)
 
 	case BINOP_LSH:
 	  {
-	    unsigned long nbits;
+	    ULONGEST nbits;
 	    if (!check_valid_shift_count (op, result_type, type2, v2, nbits))
 	      v = 0;
 	    else
@@ -1301,7 +1301,7 @@ scalar_binop (struct value *arg1, struct value *arg2, enum exp_opcode op)
 
 	case BINOP_RSH:
 	  {
-	    unsigned long nbits;
+	    ULONGEST nbits;
 	    if (!check_valid_shift_count (op, result_type, type2, v2, nbits))
 	      {
 		/* Pretend the too-large shift was decomposed in a
-- 
2.35.1

[PATCH 5/5] Fix cast types for opencl

[Hannes Domani]

The bitshift tests for opencl have these failures:

print /x (signed char) 0x0f << 8
No type named signed char.
(gdb) FAIL: gdb.base/bitshift.exp: lang=opencl: 8-bit, promoted: print /x (signed char) 0x0f << 8
print (signed char) 0x0f << 8
No type named signed char.
(gdb) FAIL: gdb.base/bitshift.exp: lang=opencl: 8-bit, promoted: print (signed char) 0x0f << 8

Apparently opencl doesn't have the 'signed' modifier for types, only
the 'unsigned' modifier.
Even 'char' is guaranteed to be signed if no modifier is used, so
this changes the casts to match this logic.
---
 gdb/testsuite/gdb.base/bitshift.exp | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.base/bitshift.exp b/gdb/testsuite/gdb.base/bitshift.exp
index cab82e1971e..dccc36b20ad 100644
--- a/gdb/testsuite/gdb.base/bitshift.exp
+++ b/gdb/testsuite/gdb.base/bitshift.exp
@@ -123,10 +123,12 @@ proc make_val_cast {lang signed bits val} {
 	return "$val as ${sign_prefix}$bits"
     } else {
 	# C-like cast.
-	if {$signed} {
+	if {!$signed} {
+	    set sign_prefix "unsigned "
+	} elseif {$lang == "opencl"} {
 	    set sign_prefix ""
 	} else {
-	    set sign_prefix "un"
+	    set sign_prefix "signed "
 	}
 	if {$bits == 8} {
 	    set type "char"
@@ -143,7 +145,7 @@ proc make_val_cast {lang signed bits val} {
 	} else {
 	    error "$lang: unsupported bits"
 	}
-	return "(${sign_prefix}signed $type) $val"
+	return "(${sign_prefix}$type) $val"
     }
 }
 
-- 
2.35.1

Re: [PATCH 1/5] Restore bitshift.exp tests

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:

Hannes> Commit cdd4206647 unintentionally disabled all tests of bitshift.exp,
Hannes> so it actually just does this:

...
Hannes> This changes it back to 'continue', and the following patches fix
Hannes> the regressions that were introduced since then unnoticed.

Ok.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 2/5] Fix right shift of negative numbers

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:

Hannes> So this changes mpz_tdiv_q_2exp to mpz_fdiv_q_2exp, since it
Hannes> does right shifts for both positive and negative numbers.

Hannes> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31590

Ok.  Thank you.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 3/5] Fix too-large or negative right shift of negative numbers

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:

Hannes> Fixed by restoring the logic from before the switch to gmp.

Ok.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 4/5] Fix 64-bit shifts where long only has 32-bit size

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:

Hannes> Fixed by changing number-of-bits variable to ULONGEST.

Thanks, this is ok.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 5/5] Fix cast types for opencl

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:

Hannes> Apparently opencl doesn't have the 'signed' modifier for types, only
Hannes> the 'unsigned' modifier.
Hannes> Even 'char' is guaranteed to be signed if no modifier is used, so
Hannes> this changes the casts to match this logic.

I don't know opencl, so I'm not sure if I should approve this or not.
However I think it seems harmless.

Reviewed-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 1/5] Restore bitshift.exp tests

[Hannes Domani]

 Am Dienstag, 11. Juni 2024 um 20:11:35 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:
>
> Hannes> Commit cdd4206647 unintentionally disabled all tests of bitshift.exp,
> Hannes> so it actually just does this:
>
> ...
> Hannes> This changes it back to 'continue', and the following patches fix
> Hannes> the regressions that were introduced since then unnoticed.
>
> Ok.
> Approved-By: Tom Tromey <tom@tromey.com>

Pushed the first 4 patches, thanks.


Hannes

Re: [PATCH 5/5] Fix cast types for opencl

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:

Hannes> Apparently opencl doesn't have the 'signed' modifier for types, only
Hannes> the 'unsigned' modifier.
Hannes> Even 'char' is guaranteed to be signed if no modifier is used, so
Hannes> this changes the casts to match this logic.

I ran "make check" and see these opencl failures, so I think this ought
to go in.

Approved-By: Tom Tromey <tom@tromey.com>

We were talking about landing this on gdb-15 as well... I'm in favor but
perhaps others may comment as well.

Tom

Re: [PATCH 5/5] Fix cast types for opencl

[Hannes Domani]

 Am Dienstag, 11. Juni 2024 um 21:31:48 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:
>
> Hannes> Apparently opencl doesn't have the 'signed' modifier for types, only
> Hannes> the 'unsigned' modifier.
> Hannes> Even 'char' is guaranteed to be signed if no modifier is used, so
> Hannes> this changes the casts to match this logic.
>
> I ran "make check" and see these opencl failures, so I think this ought
> to go in.
>
> Approved-By: Tom Tromey <tom@tromey.com>
>
> We were talking about landing this on gdb-15 as well... I'm in favor but
> perhaps others may comment as well.

Pushed on master, thanks.


Hannes