[Bug target/99314] New: [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[Bug target/99314] New: [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[sinan.lin at aalto dot fi]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99314

            Bug ID: 99314
           Summary: [Patch] [RISC-V] g++.dg/opt/memcpy1.C
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: sinan.lin at aalto dot fi
  Target Milestone: ---

Created attachment 50272
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50272&action=edit
Patch for fixing negative length problem in riscv_expand_block_move

The problem was that the argument 'length' in function
'riscv_expand_block_move' can be a negative value, and this leads to a crash.

original code in riscv.c:

bool
riscv_expand_block_move (rtx dest, rtx src, rtx length)
{
  if (CONST_INT_P (length))
    {
      HOST_WIDE_INT factor, align;

      align = MIN (MIN (MEM_ALIGN (src), MEM_ALIGN (dest)), BITS_PER_WORD);
      factor = BITS_PER_WORD / align;
      ... ...

Bug shooting step in gdb for g++.dg/opt/memcpy1.C:

gdb$ break riscv_expand_block_move
gdb$ r
gdb$ print length->u.hwint[0] # INTVAL(length)
gdb$ $1 = -4

My patch for this testcase:

diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
index d489717b2a5..737902faea8 100644
--- a/gcc/config/riscv/riscv.c
+++ b/gcc/config/riscv/riscv.c
@@ -3234,7 +3234,7 @@ riscv_block_move_loop (rtx dest, rtx src, HOST_WIDE_INT
length,
 bool
 riscv_expand_block_move (rtx dest, rtx src, rtx length)
 {
-  if (CONST_INT_P (length))
+  if (CONST_INT_P (length) && INTVAL (length) >= 0)
     {
       HOST_WIDE_INT factor, align;


Result of regression test before my patch on riscv-gcc-10.2.0:

/home/open/riscv-gnu-toolchain/scripts/testsuite-filter gcc newlib
/home/open/riscv-gnu-toolchain/test/allowlist `find
build-gcc-newlib-stage2/gcc/testsuite/ -name *.sum |paste -sd "," -`
                === g++: Unexpected fails for rv32imac ilp32 medlow ===
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++14 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++14 (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++17 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++17 (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++2a (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++2a (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++98 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++98 (test for excess errors)
                === g++: Unexpected fails for rv32im ilp32 medlow ===
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++14 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++14 (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++17 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++17 (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++2a (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++2a (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++98 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++98 (test for excess errors)
                === g++: Unexpected fails for rv32imafc ilp32f medlow ===
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++14 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++14 (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++17 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++17 (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++2a (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++2a (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++98 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++98 (test for excess errors)
                === g++: Unexpected fails for rv32iac ilp32 medlow ===
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++14 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++14 (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++17 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++17 (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++2a (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++2a (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++98 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++98 (test for excess errors)
                === g++: Unexpected fails for rv32i ilp32 medlow ===
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++14 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++14 (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++17 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++17 (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++2a (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++2a (test for excess errors)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++98 (internal compiler error)
FAIL: g++.dg/opt/memcpy1.C  -std=gnu++98 (test for excess errors)

               ========= Summary of gcc testsuite =========
                            | # of unexpected case / # of unique unexpected
case
                            |          gcc |          g++ |     gfortran |
      rv32i/  ilp32/ medlow |    0 /     0 |    8 /     1 |      - |
    rv32iac/  ilp32/ medlow |    0 /     0 |    8 /     1 |      - |
   rv32imac/  ilp32/ medlow |    0 /     0 |    8 /     1 |      - |
     rv32im/  ilp32/ medlow |    0 /     0 |    8 /     1 |      - |
 rv64imafdc/  lp64d/ medlow |    0 /     0 |    0 /     0 |      - |
  rv32imafc/ ilp32f/ medlow |    0 /     0 |    8 /     1 |      - |
   rv64imac/   lp64/ medlow |    0 /     0 |    0 /     0 |      - |

Result of regression test after my patch on riscv-gcc-10.2.0:

/home/open/riscv-gnu-toolchain/scripts/testsuite-filter gcc newlib
/home/open/riscv-gnu-toolchain/test/allowlist `find
build-gcc-newlib-stage2/gcc/testsuite/ -name *.sum |paste -sd "," -`

               ========= Summary of gcc testsuite =========
                            | # of unexpected case / # of unique unexpected
case
                            |          gcc |          g++ |     gfortran |
      rv32i/  ilp32/ medlow |    0 /     0 |    0 /     0 |      - |
    rv32iac/  ilp32/ medlow |    0 /     0 |    0 /     0 |      - |
   rv32imac/  ilp32/ medlow |    0 /     0 |    0 /     0 |      - |
     rv32im/  ilp32/ medlow |    0 /     0 |    0 /     0 |      - |
 rv64imafdc/  lp64d/ medlow |    0 /     0 |    0 /     0 |      - |
  rv32imafc/ ilp32f/ medlow |    0 /     0 |    0 /     0 |      - |
   rv64imac/   lp64/ medlow |    0 /     0 |    0 /     0 |      - |


The attachment is my patch.

Sinan Lin
PLCT Lab
sinan@isrc.iscas.ac.cn

[Bug target/99314] [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[kito at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99314

--- Comment #1 from Kito Cheng <kito at gcc dot gnu.org> ---
I didn't see this testcase failed before, and I can't reproduce that on my work
environment, do you mind share your build environment, e.g. the version of gcc
or the distribution version?

[Bug target/99314] [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[sinan.lin at aalto dot fi]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99314

--- Comment #2 from Sinan Lin <sinan.lin at aalto dot fi> ---
(In reply to Kito Cheng from comment #1)
> I didn't see this testcase failed before, and I can't reproduce that on my
> work environment, do you mind share your build environment, e.g. the version
> of gcc or the distribution version?

Hi Kito, 

My build environment:
- Ubuntu 20.04
- gcc (Ubuntu 9.3.0-17ubuntu~20.04) 9.3.0

Here is my build script for testing rv32i:

# ~$ gcc --version
# ~$ gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0
# ~$ uname -a
# ~$ Linux lnan95-P65-67HSHP 5.8.0-44-generic #50~20.04.1-Ubuntu SMP Wed Feb 10
21:07:30 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

git clone https://github.com/riscv/riscv-gnu-toolchain
cd riscv-gnu-toolchain
git submodule update --init --recursive
cd riscv-gcc
git checkout riscv-gcc-10.2.0
cd ..

# only test on riscv32i
./configure --prefix=/opt/riscv --disable-linux --with-arch=rv32i
--with-abi=ilp32
sudo make -j24 newlib
sudo make -j24 report-newlib SIM=gdb


And, we can reproduce this report from our Jenkins server, and here are the
results:
https://ci.rvperf.org/job/riscv-gnu-rv32i-newlib-10.2.0-ilp32/2/console
It was built with the script (
https://github.com/isrc-cas/PLCT-Toolbox/blob/master/ci.rvperf.org/job-configs/riscv-gnu-rv32i-newlib-10.2.0-ilp32.sh
). 

Also, we did a multilib regression test, and the result is:
https://ci.rvperf.org/job/riscv-gnu-rv64gc-multilib-gcc-10.2.0/17/console
It was built with the script(
https://github.com/isrc-cas/PLCT-Toolbox/blob/master/ci.rvperf.org/job-configs/riscv-gnu-rv64gc-multilib-gcc-10.2.0-binutils-2.35.sh
)

[Bug target/99314] [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[kito at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99314

--- Comment #3 from Kito Cheng <kito at gcc dot gnu.org> ---
Thanks for providing environment info, I'll try that soon.

[Bug target/99314] [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[kito at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99314

Kito Cheng <kito at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2021-03-05
     Ever confirmed|0                           |1

--- Comment #4 from Kito Cheng <kito at gcc dot gnu.org> ---
I can reproduce that on Ubuntu 20.04 + gcc 9.3, it turns out the length is out
of range with `signed HOST_WIDE_INT`, but after few more investigate the upper
function is feed `unsigned HOST_WIDE_INT` (emit_block_move_via_pattern), and I
guess here is some code gen change for alloca, so it will ICE on Ubuntu 20.04 +
gcc 9.3, but not ICE on Ubuntu 18.04 + gcc 7.5.

Thanks Sinan, I've write second version of the patch, it's kind of very
different than your patch, but I think you deserved that credit, so I'll list
you as first author for that patch :)

[Bug target/99314] [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[sinan.lin at aalto dot fi]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99314

Sinan Lin <sinan.lin at aalto dot fi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #5 from Sinan Lin <sinan.lin at aalto dot fi> ---
(In reply to Kito Cheng from comment #4)
> I can reproduce that on Ubuntu 20.04 + gcc 9.3, it turns out the length is
> out of range with `signed HOST_WIDE_INT`, but after few more investigate the
> upper function is feed `unsigned HOST_WIDE_INT`
> (emit_block_move_via_pattern), and I guess here is some code gen change for
> alloca, so it will ICE on Ubuntu 20.04 + gcc 9.3, but not ICE on Ubuntu
> 18.04 + gcc 7.5.
> 
> Thanks Sinan, I've write second version of the patch, it's kind of very
> different than your patch, but I think you deserved that credit, so I'll
> list you as first author for that patch :)

Hi Kito,

I really appreciate your help with this problem. Have a great weekend!

[Bug target/99314] [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[kito at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99314

Kito Cheng <kito at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED

--- Comment #6 from Kito Cheng <kito at gcc dot gnu.org> ---
I would prefer keep this open until merged into master, gcc-10 and gcc-9
branch.

[Bug target/99314] [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99314

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Kito Cheng <kito@gcc.gnu.org>:

https://gcc.gnu.org/g:d9f0ade001533c9544bf2153b6baa8844ec0bee4

commit r11-7720-gd9f0ade001533c9544bf2153b6baa8844ec0bee4
Author: Sinan Lin <sinan@isrc.iscas.ac.cn>
Date:   Thu Mar 4 18:02:39 2021 +0800

    PR target/99314: Fix integer signedness issue for cpymem pattern expansion.

    Third operand of cpymem pattern is unsigned HOST_WIDE_INT, however we
    are interpret that as signed HOST_WIDE_INT, that not a problem in
    most case, but when the value is large than signed HOST_WIDE_INT, it
    might screw up since we have using that value to calculate the buffer
    size.

    2021-03-05  Sinan Lin  <sinan@isrc.iscas.ac.cn>
                Kito Cheng  <kito.cheng@sifive.com>

    gcc/ChangeLog:

            * config/riscv/riscv.c (riscv_block_move_straight): Change type
            to unsigned HOST_WIDE_INT for parameter and local variable with
            HOST_WIDE_INT type.
            (riscv_adjust_block_mem): Ditto.
            (riscv_block_move_loop): Ditto.
            (riscv_expand_block_move): Ditto.

[Bug target/99314] [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99314

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Kito Cheng <kito@gcc.gnu.org>:

https://gcc.gnu.org/g:f26015ef086f68d55f1f2ae293a99d5ad3736795

commit r10-9456-gf26015ef086f68d55f1f2ae293a99d5ad3736795
Author: Sinan Lin <sinan@isrc.iscas.ac.cn>
Date:   Thu Mar 4 18:02:39 2021 +0800

    PR target/99314: Fix integer signedness issue for cpymem pattern expansion.

    Third operand of cpymem pattern is unsigned HOST_WIDE_INT, however we
    are interpret that as signed HOST_WIDE_INT, that not a problem in
    most case, but when the value is large than signed HOST_WIDE_INT, it
    might screw up since we have using that value to calculate the buffer
    size.

    2021-03-05  Sinan Lin  <sinan@isrc.iscas.ac.cn>
                Kito Cheng  <kito.cheng@sifive.com>

    gcc/ChangeLog:

            * config/riscv/riscv.c (riscv_block_move_straight): Change type
            to unsigned HOST_WIDE_INT for parameter and local variable with
            HOST_WIDE_INT type.
            (riscv_adjust_block_mem): Ditto.
            (riscv_block_move_loop): Ditto.
            (riscv_expand_block_move): Ditto.

    (cherry picked from commit d9f0ade001533c9544bf2153b6baa8844ec0bee4)

[Bug target/99314] [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99314

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Kito Cheng <kito@gcc.gnu.org>:

https://gcc.gnu.org/g:becb26eb6ef42481014dc6fb24e8bfe7ec6f51d1

commit r9-9294-gbecb26eb6ef42481014dc6fb24e8bfe7ec6f51d1
Author: Sinan Lin <sinan@isrc.iscas.ac.cn>
Date:   Thu Mar 4 18:02:39 2021 +0800

    PR target/99314: Fix integer signedness issue for cpymem pattern expansion.

    Third operand of cpymem pattern is unsigned HOST_WIDE_INT, however we
    are interpret that as signed HOST_WIDE_INT, that not a problem in
    most case, but when the value is large than signed HOST_WIDE_INT, it
    might screw up since we have using that value to calculate the buffer
    size.

    2021-03-05  Sinan Lin  <sinan@isrc.iscas.ac.cn>
                Kito Cheng  <kito.cheng@sifive.com>

    gcc/ChangeLog:

            * config/riscv/riscv.c (riscv_block_move_straight): Change type
            to unsigned HOST_WIDE_INT for parameter and local variable with
            HOST_WIDE_INT type.
            (riscv_adjust_block_mem): Ditto.
            (riscv_block_move_loop): Ditto.
            (riscv_expand_block_move): Ditto.

    (cherry picked from commit d9f0ade001533c9544bf2153b6baa8844ec0bee4)

[Bug target/99314] [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[kito at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99314

Kito Cheng <kito at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #10 from Kito Cheng <kito at gcc dot gnu.org> ---
Committed fix into trunk, gcc-10 and gcc-9

[Bug target/99314] [Patch] [RISC-V] g++.dg/opt/memcpy1.C

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99314

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:8c114759b8c9c9e2ec90b82d92a24b5a71647017

commit r12-886-g8c114759b8c9c9e2ec90b82d92a24b5a71647017
Author: Jason Merrill <jason@redhat.com>
Date:   Tue May 18 12:18:56 2021 -0400

    c++: non-static member, decltype, {} [PR100205]

    This test was fixed by my second patch for PR93314, which distinguishes
    between constant-expression and potentially-constant-evaluated contexts in
a
    way that my first patch did not.

            PR c++/100205
            PR c++/99314

    gcc/testsuite/ChangeLog:

            * g++.dg/cpp0x/decltype-nonstatic1.C: New test.