[PATCH v1 0/2] Fix the right shift of negative numbers

[PATCH v1 0/2] Fix the right shift of negative numbers

[Sergey Kaplun]

The original issue is the incorrect truncated result of the right shift
for negative values:
```
(gdb) p /t -3
$1 = 11111111111111111111111111111101
(gdb) p /t -3 >> 1
$2 = 11111111111111111111111111111111
```
Where 11111111111111111111111111111110 is expected.

This is due to the use of `mpz_tdiv_q_2exp()` instead of
`mpz_fdiv_q_2exp()` for the right shift calculation.
According to the documentation of gmplib [1]:
```
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 twos complement the same as the
bitwise logical functions do, whereas mpz_tdiv_q_2exp effectively treats
n as sign and magnitude.
```

The second patch fixes the behaviour and adds the testcase above to
gdb.base/bitshift.exp.

But when I tried to add the test, I've noticed that all test cases from
this file are skipped due to using `return` instead of `continue` for
the skip list of unhandled languages.  So, I've enabled this test back
in the first patch and fixed the behaviour for the negative shift of
negative values.

Unfortunately, some test cases for "opencl" still fail, so I've added it
to the skip list for this version of the patch since I'm not very
familiar with it (I need some guidance here):

| make check TESTS="gdb.base/bitshift.exp" | grep FAIL
| WARNING: Couldn't find the global config file.
| FAIL: gdb.base/bitshift.exp: lang=opencl: 8-bit, promoted: print /x (signed char) 0x0f << 8
| FAIL: gdb.base/bitshift.exp: lang=opencl: 8-bit, promoted: print (signed char) 0x0f << 8
| FAIL: gdb.base/bitshift.exp: lang=opencl: 8-bit, invalid: print (signed char) 0x7f << -1
| FAIL: gdb.base/bitshift.exp: lang=opencl: 8-bit, invalid: print (signed char) 0x7f >> -1
| FAIL: gdb.base/bitshift.exp: lang=opencl: 8-bit, invalid: print/x (signed char) 0x7f << 8
| FAIL: gdb.base/bitshift.exp: lang=opencl: 8-bit, invalid: print/x (signed char) 0x7f >> 8
| FAIL: gdb.base/bitshift.exp: lang=opencl: 8-bit, invalid: print (signed char) 0x7f << 32
| FAIL: gdb.base/bitshift.exp: lang=opencl: 8-bit, invalid: print (signed char) 0x7f >> 32
| FAIL: gdb.base/bitshift.exp: lang=opencl: 8-bit, invalid: print (signed char) 0x7f << 33
| FAIL: gdb.base/bitshift.exp: lang=opencl: 8-bit, invalid: print (signed char) 0x7f >> 33

For other tests, I see no regressions (some 3 tests are flaky on master):

| -# of expected passes		112908
| -# of unexpected failures	1791
| +# of expected passes		112011
| +# of unexpected failures	1788
|  # of expected failures		219
|  # of known failures		122
|  # of untested testcases		22

Within enabled back gdb.base/bitshift.exp (and disabled opencl):

| make check TESTS="gdb.base/bitshift.exp"
| ...
| === gdb Summary ===
|
| # of expected passes            921

Tested on Linux 6.1.57-gentoo-x86_64 x86_64.
Configure command flags:
| --enable-build-warnings --enable-ubsan --enable-unit-tests=yes

[1]: https://gmplib.org/gmp-man-6.2.1.pdf#Integer%20Division

Sergey Kaplun (2):
  gdb/testsuite: enable back gdb.base/bitshift.exp
  Fix the right shift of negative numbers

 gdb/gmp-utils.h                     |  4 ++--
 gdb/testsuite/gdb.base/bitshift.exp |  7 +++++--
 gdb/valarith.c                      | 11 ++++++++++-
 3 files changed, 17 insertions(+), 5 deletions(-)

-- 
2.43.2

[PATCH v1 1/2] gdb/testsuite: enable back gdb.base/bitshift.exp

[Sergey Kaplun]

Since commit cdd4206647d0 ("gdb/testsuite: fix "continue outside of
loop" TCL errors"), all tests in gdb.base/bitshift.exp are skipped due
to an early return after matching the "ada" language.

This patch enables all tests and adds "opencl" to the skip list since
some test cases aren't handled correctly for it.

Also, since commit 303a881f8789 ("Use gdb_gmp for scalar arithmetic"),
0 instead of -1 is returned for signed negative values shifted by a
negative number, which contradicts Go semantics.  So, this patch fixes
the semantics and restores the deleted comment.
---
 gdb/testsuite/gdb.base/bitshift.exp |  4 ++--
 gdb/valarith.c                      | 11 ++++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.base/bitshift.exp b/gdb/testsuite/gdb.base/bitshift.exp
index cfb1e7b9820..806886cc1bf 100644
--- a/gdb/testsuite/gdb.base/bitshift.exp
+++ b/gdb/testsuite/gdb.base/bitshift.exp
@@ -175,10 +175,10 @@ proc test_shifts {} {
 
     foreach_with_prefix lang $supported_langs {
 	set skip_langs {
-	    "unknown" "ada" "modula-2" "pascal" "fortran"
+	    "ada" "fortran" "modula-2" "opencl" "pascal" "unknown"
 	}
 	if {[lsearch -exact $skip_langs $lang] >= 0} {
-	    return
+	    continue
 	}
 
 	gdb_test_no_output "set language $lang"
diff --git a/gdb/valarith.c b/gdb/valarith.c
index 6b152cadcac..50ef6d2b92e 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -1302,7 +1302,16 @@ 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.  */
+	      v = (!result_type->is_unsigned () && v1 < 0) ? -1 : 0;
 	    else
 	      v = v1 >> nbits;
 	  }
-- 
2.43.2

[PATCH v1 2/2] Fix the right shift of negative numbers

[Sergey Kaplun]

Since commit 303a881f8789 ("Use gdb_gmp for scalar arithmetic"), the
incorrect values are returned for the right shift of negative numbers,
which are truncated during the division.
```
(gdb) p /t -3
$1 = 11111111111111111111111111111101
(gdb) p /t -3 >> 1
$2 = 11111111111111111111111111111111
```

According to the documentation of gmplib [1]:
```
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 twos complement the same as the
bitwise logical functions do, whereas mpz_tdiv_q_2exp effectively treats
n as sign and magnitude.
```

So, use the mpz_fdiv_q_2exp variant to avoid truncation for negative
values since the behaviour for positive values is equivalent.

[1]: https://gmplib.org/gmp-man-6.2.1.pdf#Integer%20Division
---
 gdb/gmp-utils.h                     | 4 ++--
 gdb/testsuite/gdb.base/bitshift.exp | 3 +++
 2 files changed, 5 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 806886cc1bf..0bc99cc438f 100644
--- a/gdb/testsuite/gdb.base/bitshift.exp
+++ b/gdb/testsuite/gdb.base/bitshift.exp
@@ -346,6 +346,9 @@ proc test_shifts {} {
 	    test_shift $lang "print -1 >> 1" " = -1"
 	    test_shift $lang "print -8 >> 1" " = -4"
 	    test_shift $lang "print [make_int64 $lang -8] >> 1" " = -4"
+	    # Make sure an negative value isn't truncated by shift
+	    # operator.
+	    test_shift $lang "print -3 >> 1" " = -2"
 	}
 
 	# Make sure an unsigned 64-bit value with high bit set isn't
-- 
2.43.2

[PING] [PATCH v1 0/2] Fix the right shift of negative numbers

[Sergey Kaplun]

Hi, folks!
I am looking forward to your reply!

Should I create an issue related to this patchset?

Also, for these 2 weeks, I've dived a bit into the problems around
OpenCL and CI failures (see links below) and need help with the
following questions:

Since commit 63c457b91104 ("gmp-utils: protect gdb_mpz exports against
out-of-range values"), the subtest max-uint64 of the enabled test fails
[1] on a 32-bit arm since "the 0xffffffffffffffff value cannot be
represented as 32-bit unsigned integer". Should I disable this test for
the 32-bit arm, or skip it somehow?

Also, on the aarch64, the subtest "-1 >> 1" fails [2] before the fix in
the second commit. Should I squash the commits to avoid regressions
between commits?

Plus, I've dived a bit into OpenCL failures. After the commit
55fc1623f942 ("Add name canonicalization for C") the corresponding
OpenCL cast fails with the error:

| gdb -ex 'set language opencl' -ex 'p (signed char) 1' -ex 'quit'
| ...
| No type named signed char.

Since for OpenCL "signed char" is valid [3] and looks like it is the
same type as just "char" [4], it should be fixed like the following:

===================================================================
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 1e1a7e9ce61..8dc1872849d 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -1684,8 +1684,9 @@ lookup_unsigned_typename (const struct language_defn *language,
 struct type *
 lookup_signed_typename (const struct language_defn *language, const char *name)
 {
-  /* In C and C++, "char" and "signed char" are distinct types.  */
-  if (streq (name, "char"))
+  /* In C and C++, "char" and "signed char" are distinct types.
+     But not in OpenCL.  */
+  if (streq (name, "char") && language->la_language != language_opencl)
     name = "signed char";
   return lookup_typename (language, name, NULL, 0);
 }
diff --git a/gdb/testsuite/gdb.opencl/datatypes.cl b/gdb/testsuite/gdb.opencl/datatypes.cl
index 999defa38b4..d390eb02b3f 100644
--- a/gdb/testsuite/gdb.opencl/datatypes.cl
+++ b/gdb/testsuite/gdb.opencl/datatypes.cl
@@ -46,6 +46,8 @@ __kernel void testkernel (__global int *data)
   char8  c8  = (char8) (1, 2, 3, 4, 5, 6, 7, 8);
   char16 c16 = (char16)(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16);
 
+  signed char c_signed = 1;
+
   uchar   uc   = 1;
   uchar2  uc2  = (uchar2) (1, 2);
 #ifdef CL_VERSION_1_1
diff --git a/gdb/testsuite/gdb.opencl/datatypes.exp b/gdb/testsuite/gdb.opencl/datatypes.exp
index 16243ad3cfb..c47c4873421 100644
--- a/gdb/testsuite/gdb.opencl/datatypes.exp
+++ b/gdb/testsuite/gdb.opencl/datatypes.exp
@@ -53,6 +53,9 @@ gdb_test "p sizeof(char8)" " = 8"
 gdb_test "whatis char16" "type = char16"
 gdb_test "p sizeof(char16)" " = 16"
 
+gdb_test "whatis signed char" "type = char"
+gdb_test "p sizeof(signed char)" " = 1"
+
 gdb_test "whatis unsigned char" "type = unsigned char"
 gdb_test "p sizeof(unsigned char)" " = 1"
 gdb_test "whatis uchar" "type = uchar"
@@ -247,6 +250,10 @@ gdb_test "whatis c16" "type = char16"
 gdb_test "p sizeof(c16)" " = 16"
 gdb_test "print c16" " = \\{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16\\}"
 
+gdb_test "whatis c_signed" "type = char"
+gdb_test "p sizeof(c_signed)" " = 1"
+gdb_test "print/d c_signed" " = 1"
+
 gdb_test "whatis uc" "type = uchar"
 gdb_test "p sizeof(uc)" " = 1"
 gdb_test "print/d uc" " = 1"
===================================================================

Also, locally, I've tracked down that the OpenCL tests are skipped due
to the following warnings:

| /home/burii/binutils-gdb-master/gdb/testsuite/lib/cl_util.c:371:70: warning: format '%d' expects argument of type 'int', but argument 2 has type 'size_t' {aka 'long unsigned int'} [-Wformat=]
|   371 |               printf ("    CL_DEVICE_IMAGE3D_MAX_DEPTH:             %d\n", sizet);
|       |                                                                     ~^     ~~~~~
|       |                                                                      |     |
|       |                                                                      int   size_t {aka long unsigned int}
|       |                                                                     %ld
| In file included from /usr/include/CL/cl.h:20,
|                  from /home/burii/binutils-gdb-master/gdb/testsuite/lib/opencl_hostapp.c:28:
| /usr/include/CL/cl_version.h:22:9: note: '#pragma message: cl_version.h: CL_TARGET_OPENCL_VERSION is not defined. Defaulting to 300 (OpenCL 3.0)'
|    22 | #pragma message("cl_version.h: CL_TARGET_OPENCL_VERSION is not defined. Defaulting to 300 (OpenCL 3.0)")
|       |         ^~~~~~~
| /home/burii/binutils-gdb-master/gdb/testsuite/lib/opencl_hostapp.c: In function 'main':
| /home/burii/binutils-gdb-master/gdb/testsuite/lib/opencl_hostapp.c:81:3: warning: 'clCreateCommandQueue' is deprecated [-Wdeprecated-declarations]
|    81 |   queue = clCreateCommandQueue (context, device, 0, &err);
|       |   ^~~~~
| /usr/include/CL/cl.h:1920:1: note: declared here
|  1920 | clCreateCommandQueue(cl_context                     context,
|       | ^~~~~~~~~~~~~~~~~~~~

Since the output of the CC command isn't empty, the OpenCL suite is
declared unsupported. I added the following hotfix to run tests:

===================================================================
diff --git a/gdb/testsuite/lib/opencl.exp b/gdb/testsuite/lib/opencl.exp
index caa0e1e2911..7d2316edad7 100644
--- a/gdb/testsuite/lib/opencl.exp
+++ b/gdb/testsuite/lib/opencl.exp
@@ -23,6 +23,7 @@ proc gdb_compile_opencl_hostapp {clsource executable options} {
     set src "${srcdir}/lib/cl_util.c ${srcdir}/lib/opencl_hostapp.c"
     set binfile [standard_output_file ${executable}]
     set compile_flags [concat additional_flags=-I${srcdir}/lib/ additional_flags=-DCL_SOURCE=$clsource]
+    set compile_flags [concat $compile_flags additional_flags=-DCL_TARGET_OPENCL_VERSION=300 additional_flags=-w]
     set options_opencl [concat {debug} $compile_flags $options [list libs=-lOpenCL]]
     return [gdb_compile ${src} ${binfile} "executable" ${options_opencl}]
 }
===================================================================

Then I run the testcase like the following, with the result:
| make check TESTS="gdb.opencl/datatypes.exp" | grep FAIL
| WARNING: Couldn't find the global config file.
| FAIL: gdb.opencl/datatypes.exp: whatis char16
| FAIL: gdb.opencl/datatypes.exp: p sizeof(char16)
| FAIL: gdb.opencl/datatypes.exp: whatis uchar16
| FAIL: gdb.opencl/datatypes.exp: p sizeof(uchar16)
| ... all 16-bit values fail
| ERROR: (datatypes) No such file or directory
| FAIL: gdb.opencl/datatypes.exp: run
| .. all after it fail

To make this work, I've added the following fix:
===================================================================
diff --git a/gdb/testsuite/gdb.opencl/datatypes.exp b/gdb/testsuite/gdb.opencl/datatypes.exp
index 16243ad3cfb..539099d9c0f 100644
--- a/gdb/testsuite/gdb.opencl/datatypes.exp
+++ b/gdb/testsuite/gdb.opencl/datatypes.exp
@@ -22,10 +22,11 @@ load_lib opencl.exp
 require allow_opencl_tests
 
 set testfile "datatypes"
-set clprogram [remote_download target ${srcdir}/${subdir}/${testfile}.cl]
+set clsource ${srcdir}/${subdir}/${testfile}.cl
+set clprogram [remote_download target ${clsource}]
 
 # Compile the generic OpenCL host app
-if { [gdb_compile_opencl_hostapp "${clprogram}" "${testfile}" "" ] != "" } {
+if { [gdb_compile_opencl_hostapp "${clsource}" "${testfile}" "" ] != "" } {
     untested "failed to compile"
     return -1
 }
@@ -197,7 +201,7 @@ gdb_test_no_output "set language auto" "no prompt when setting the language to a
 
 # Load the OpenCL app
 gdb_reinitialize_dir $srcdir/$subdir
-gdb_load ${objdir}/${subdir}/${testfile}
+gdb_load ${objdir}/outputs/${subdir}/${testfile}/${testfile}
 
 # Set breakpoint at the OpenCL kernel
 gdb_test "tbreak testkernel" \
===================================================================

But even if I ran the executable from the directory with the
datatypes.cl file, I got the following errors:
| clBuildProgram failed with:
| 1:116:16: error: double precision constant requires cl_khr_fp64, casting to single precision
|   half   h   = 1.0;
|                ^
| 1:117:25: error: double precision constant requires cl_khr_fp64, casting to single precision
|   half2  h2  = (half2) (1.0, 2.0);
|                         ^
| ...

If I remove the ".0" parts, then it runs OK, but I still can't set a
breakpoint on the `testkernel`. I'm kind of stuck here and don't know
what to do.

[1]: https://ci.linaro.org/job/tcwg_gdb_check--master-arm-precommit/2073/artifact/artifacts/artifacts.precommit/notify/
[2]: https://ci.linaro.org/job/tcwg_gdb_check--master-aarch64-precommit/2072/artifact/artifacts/artifacts.precommit/notify/
[3]: https://godbolt.org/z/obPGdses6
[4]: https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_C.html#built-in-scalar-data-types

Re: [PING*2][PATCH v1 0/2] Fix the right shift of negative numbers

[Sergey Kaplun]

Hi, guys!
I see that my patch fixes the 31590 issue [1].

[1]: https://sourceware.org/bugzilla/show_bug.cgi?id=31590

Re: [PING*2][PATCH v1 0/2] Fix the right shift of negative numbers

[Tom Tromey]

>>>>> "Sergey" == Sergey Kaplun <sergey_v_kaplun@mail.ru> writes:

Sergey> Hi, guys!
Sergey> I see that my patch fixes the 31590 issue [1].

Sergey> [1]: https://sourceware.org/bugzilla/show_bug.cgi?id=31590

Hi.  Do you have a copyright assignment in place?
I think we'll probably need one for this series.

Tom

Re: [PING*2][PATCH v1 0/2] Fix the right shift of negative numbers

[Sergey Kaplun]

On 23.04.24, Tom Tromey wrote:
> >>>>> "Sergey" == Sergey Kaplun <sergey_v_kaplun@mail.ru> writes:
> 
> Sergey> Hi, guys!
> Sergey> I see that my patch fixes the 31590 issue [1].
> 
> Sergey> [1]: https://sourceware.org/bugzilla/show_bug.cgi?id=31590
> 
> Hi.  Do you have a copyright assignment in place?
> I think we'll probably need one for this series.
> 
> Tom

Hi, Tom.
No, I don't have a copyright assignment yet. What do I need to do?

-- 
Best regards,
Sergey Kaplun