[Bug c++/113994] New: Probable C++ code generation bug with -O2 on s390x platform

[Bug c++/113994] New: Probable C++ code generation bug with -O2 on s390x platform

[thierry at lelegard dot fr]

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

            Bug ID: 113994
           Summary: Probable C++ code generation bug with -O2 on s390x
                    platform
           Product: gcc
           Version: 13.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: thierry at lelegard dot fr
  Target Milestone: ---

- GCC version: 13.2.0
- Operating system: Ubuntu 23.10
- Platform: IBM s390x (emulated on Qemu 8.2.1)

On s390x platform, with "g++ -O2", the following C++ sample test code aborts
with std::out_of_range exception in the instantiation of template
std::string::substr(). The parameters are valid (see below) and no exception
should be thrown.

The problem does not occur either:

1. when the std::cout trace line is uncommented (probably break some incorrect
optimization)
2. with -O1 or -O0
3. with clang
4. on other CPU architectures

For information, the original project (https://tsduck.io/) was tested on 16
different operating systems, 8 CPU architectures and 3 compilers (details here:
https://tsduck.io/doxy/building.html). The problem occurs on s390x only.

Demonstration:
------------------------------------------------------------
$ g++ -O2 test.cpp -o test
$ ./test
terminate called after throwing an instance of 'std::out_of_range'
  what():  basic_string::substr: __pos (which is 3) > this->size() (which is 3)
Aborted (core dumped)
$
$ g++ -O1 test.cpp -o test
$ ./test
ok
$
$ clang++ -O2 test.cpp -o test
$ ./test
ok
$
------------------------------------------------------------

Source code:
------------------------------------------------------------
#include <string>
#include <iostream>

void f(const std::string& path, size_t& index, std::string& next)
{
    size_t end = path.find(']');
    if (end >= path.size()) {
        return;
    }
    else if (end == 1) {
        index = std::string::npos;
    }
    while (++end < path.size() && path[end] == u'.') {
    }
    // If uncommented, display "end = 3" and there is no exception.
    // std::cout << "end = " << end << std::endl;
    next = path.substr(end);
}

int main(int argc, char* argv[])
{
    size_t index = 0;
    std::string next;
    f("[0]", index, next);
    std::cout << "ok" << std::endl;
}
------------------------------------------------------------

At the point of exception, end == path.size() == 3. This is case is permitted
by the C++ standard and should return the empty string. By the way, the
exception message "__pos (which is 3) > this->size() (which is 3)" is
inconsistent because "3 > 3" is false.

The problem is not in the source code if the C++ library but most probably in
the generated code for the instantiation of the template class
std::basic_string. If the code was in the C++ library, the problem should be
also present with the trace line, with other forms of optimization in the
compilation of the application or with clang.

Using gdb on the core file demonstrates that:

- the exception is thrown in the generated code for the instantiation of
std::__cxx11::basic_string::substr
- the __pos parameter of substr() is 3 (get the substring starting at index 3)
- the string is "[0]" (length is 3)

------------------------------------------------------------
$ gdb ./test core
GNU gdb (Ubuntu 14.0.50.20230907-0ubuntu1) 14.0.50.20230907-git
....
Core was generated by `./test'.
Program terminated with signal SIGABRT, Aborted.
#0  __pthread_kill_implementation (threadid=<optimized out>,
signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
44      pthread_kill.c: No such file or directory.
(gdb) bt
#0  __pthread_kill_implementation (threadid=<optimized out>,
signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
#1  0x000003ff89d9e106 in __pthread_kill_internal (signo=6, threadid=<optimized
out>) at pthread_kill.c:78
#2  0x000003ff89d4aa90 in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#3  0x000003ff89d2a4ec in __GI_abort () at abort.c:79
#4  0x000003ff8a0d28f2 in __gnu_cxx::__verbose_terminate_handler() () from
/lib/s390x-linux-gnu/libstdc++.so.6
#5  0x000003ff8a0cfc4e in ?? () from /lib/s390x-linux-gnu/libstdc++.so.6
#6  0x000003ff8a0cfcd8 in std::terminate() () from
/lib/s390x-linux-gnu/libstdc++.so.6
#7  0x000003ff8a0cffe6 in __cxa_throw () from
/lib/s390x-linux-gnu/libstdc++.so.6
#8  0x000003ff8a1053de in std::__throw_out_of_range_fmt(char const*, ...) ()
from /lib/s390x-linux-gnu/libstdc++.so.6
#9  0x000002aa3f8013ac in std::__cxx11::basic_string<char,
std::char_traits<char>, std::allocator<char> >::_M_check (
    __s=0x2aa3f8013f0 "basic_string::substr", __pos=3, this=0x3ffe9979cf8) at
/usr/include/c++/13/bits/basic_string.h:379
#10 std::__cxx11::basic_string<char, std::char_traits<char>,
std::allocator<char> >::substr (__n=18446744073709551615, __pos=3, 
    this=0x3ffe9979cf8) at /usr/include/c++/13/bits/basic_string.h:3153
#11 f (path="[0]", index=@0x3ffe9979cd0: 0, next="") at test.cpp:17
#12 0x000002aa3f800e48 in main (argc=<optimized out>, argv=<optimized out>) at
test.cpp:24
(gdb) f 9
#9  0x000002aa3f8013ac in std::__cxx11::basic_string<char,
std::char_traits<char>, std::allocator<char> >::_M_check (
    __s=0x2aa3f8013f0 "basic_string::substr", __pos=3, this=0x3ffe9979cf8) at
/usr/include/c++/13/bits/basic_string.h:379
379               __throw_out_of_range_fmt(__N("%s: __pos (which is %zu) > "
(gdb) p *this
$2 = "[0]"
(gdb) 
------------------------------------------------------------

Compilers and OS versions:
------------------------------------------------------------
$ gcc --version
gcc (Ubuntu 13.2.0-4ubuntu3) 13.2.0
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$
$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/s390x-linux-gnu/13/lto-wrapper
Target: s390x-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 13.2.0-4ubuntu3'
--with-bugurl=file:///usr/share/doc/gcc-13/README.Bugs
--enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++,m2 --prefix=/usr
--with-gcc-major-version-only --program-suffix=-13
--program-prefix=s390x-linux-gnu- --enable-shared --enable-linker-build-id
--libexecdir=/usr/libexec --without-included-gettext --enable-threads=posix
--libdir=/usr/lib --enable-nls --enable-bootstrap --enable-clocale=gnu
--enable-libstdcxx-debug --enable-libstdcxx-time=yes
--with-default-libstdcxx-abi=new --enable-gnu-unique-object
--disable-libquadmath --disable-libquadmath-support --enable-plugin
--enable-default-pie --with-system-zlib --enable-libphobos-checking=release
--with-target-system-zlib=auto --enable-objc-gc=auto --enable-multiarch
--disable-werror --with-arch=z13 --with-tune=z16
--enable-s390-excess-float-precision --with-long-double-128 --enable-multilib
--enable-checking=release --build=s390x-linux-gnu --host=s390x-linux-gnu
--target=s390x-linux-gnu --with-build-config=bootstrap-lto-lean
--enable-link-serialization=2
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 13.2.0 (Ubuntu 13.2.0-4ubuntu3) 
$
$ clang --version
Ubuntu clang version 16.0.6 (15)
Target: s390x-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
$ 
$ uname -a
Linux vms390x 6.5.0-17-generic #17-Ubuntu SMP Thu Jan 11 13:28:22 UTC 2024
s390x s390x s390x GNU/Linux
$ 
$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 23.10
Release:        23.10
Codename:       mantic
------------------------------------------------------------

[Bug target/113994] Probable C++ code generation bug with -O2 on s390x platform

[xry111 at gcc dot gnu.org]

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

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=112986
                 CC|                            |xry111 at gcc dot gnu.org

--- Comment #1 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
Hmm... There was some QEMU bug causing all integer comparison broken emulating
s390x but it's said to be fixed in QEMU 8.x.  But I'd still like someone having
the access to test this on a real hardware first to rule out any QEMU issue.

[Bug target/113994] Probable C++ code generation bug with -O2 on s390x platform

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I get
g++ -O2 -o pr113994{,.C}; ./pr113994
terminate called after throwing an instance of 'std::out_of_range'
  what():  basic_string::substr: __pos (which is 3) > this->size() (which is 3)
Aborted
both with current trunk and with g++ 13.2.1 20231205 on real hw with the Fedora
defaults, i.e. -march=z13 -mtune=z14.

[Bug target/113994] Probable C++ code generation bug with -O2 on s390x platform

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
           Keywords|                            |wrong-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2024-02-20

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
So confirmed.

[Bug target/113994] Probable C++ code generation bug with -O2 on s390x platform

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |krebbel at gcc dot gnu.org

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Reproduced on the trunk as well.  So, we have before cse1:
(insn 28 27 29 6 (set (reg:CCU 33 %cc)
        (compare:CCU (reg/v:DI 61 [ end ])
            (reg:DI 63 [ _15 ]))) "pr113994.C":13:32 discrim 7 1436
{*cmpdi_ccu}
     (nil))
(jump_insn 29 28 30 6 (set (pc)
        (if_then_else (geu (reg:CCU 33 %cc)
                (const_int 0 [0]))
            (label_ref 39)
            (pc))) "pr113994.C":13:32 discrim 7 2149 {*cjump_64}
     (int_list:REG_BR_PROB 536870916 (nil))
 -> 39)
;;  succ:       7 [50.0% (guessed)]  count:536870912 (estimated locally, freq
3.0000) (FALLTHRU)
;;              8 [50.0% (guessed)]  count:536870912 (estimated locally, freq
3.0000)
...
;; basic block 8, loop depth 0, count 536870912 (estimated locally, freq
3.0000), maybe hot
;;  prev block 7, next block 9, flags: (RTL, MODIFIED)
;;  pred:       6 [50.0% (guessed)]  count:536870912 (estimated locally, freq
3.0000)
;; bb 8 artificial_defs: { }
;; bb 8 artificial_uses: { u-1(11){ }u-1(15){ }u-1(32){ }u-1(34){ }}
(code_label 39 35 40 8 19 (nil) [1 uses])
(note 40 39 41 8 [bb 8] NOTE_INSN_BASIC_BLOCK)
(insn 41 40 42 8 (set (reg:CCU 33 %cc)
        (compare:CCU (reg/v:DI 61 [ end ])
            (reg:DI 63 [ _15 ])))
"/usr/include/c++/13/bits/basic_string.h":388:2 discrim 1 1436 {*cmpdi_ccu}
     (nil))
(jump_insn 42 41 43 8 (set (pc)
        (if_then_else (leu (reg:CCU 33 %cc)
                (const_int 0 [0]))
            (label_ref 53)
            (pc))) "/usr/include/c++/13/bits/basic_string.h":388:2 discrim 1
2149 {*cjump_64}
     (int_list:REG_BR_PROB 1073028868 (nil))
 -> 53)

Next cse1 decides that the insn 41 is redundant, because insn 28 performed the
same comparison, dominates insn 41 and %cc has not been modified.  REG_DEAD
note remains
on jump_insn 35, cse/gcse/postreload-cse doesn't remove REG_DEAD/REG_UNUSED
notes it invalidates.
ce1 pass then recomputes the notes and drops it, so fortunately this doesn't
seem to be about these notes.
%cc is even mentioned in
;; lr  out       11 [%r11] 15 [%r15] 32 [%ap] 33 [%cc] 34 [%fp] 61 63 82 84
;; live  out     11 [%r11] 15 [%r15] 32 [%ap] 33 [%cc] 34 [%fp] 61 63 82 84
after the jump_insn 29 (that actually already shows up that way in fwprop1, the
first time some pass did df_analyze after cse1).
But then something goes wrong during loop_invariant pass, jump_insn 29 above
gets (incorrect) REG_DEAD %cc note again and 33 [%cc] is dropped from lr
out/live out.
Sure, %cc is unused on one of the two successors, but on the other one is used.
And then comes loop_doloop and replaces that jump_insn 29 with
(jump_insn 226 28 225 6 (parallel [
            (set (pc)
                (if_then_else (ne (reg:DI 120)
                        (const_int 1 [0x1]))
                    (label_ref 225)
                    (pc)))
            (set (reg:DI 120)
                (plus:DI (reg:DI 120)
                    (const_int -1 [0xffffffffffffffff])))
            (clobber (scratch:DI))
            (clobber (reg:CC 33 %cc))
        ]) "pr113994.C":13:32 discrim 7 -1
     (int_list:REG_BR_PROB 536870916 (nil))
 -> 225)
(unsure whether because of the bogus REG_DEAD note, wrong df live out or
something else).  And then the cprop3 pass just removes the dead
(insn 28 27 226 8 (set (reg:CCU 33 %cc)
        (compare:CCU (reg/v:DI 61 [ end ])
            (reg:DI 63 [ _15 ]))) "pr113994.C":13:32 discrim 7 1436
{*cmpdi_ccu}
     (expr_list:REG_DEAD (reg:DI 63 [ _15 ])
        (nil)))
comparison because nothing really consumes %cc it sets (there is a user in
jump_insn 42, but there is jump_insn 226 in between which clobbers it).
And the above brctg then clobbers %cc and so we end up with
        brctg   %r10,.L40
.L19:
        jh      .L55
in the assembly.  If .L19 is reached from
        jhe     .L19
then it works fine, but if it is reached from falling through from brctg, %cc
is not what the code expects to (comparison of end with size()).

[Bug target/113994] [13/14 Regression] Probable C++ code generation bug with -O2 on s390x platform

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|Probable C++ code           |[13/14 Regression] Probable
                   |generation bug with -O2 on  |C++ code generation bug
                   |s390x platform              |with -O2 on s390x platform
   Target Milestone|---                         |13.3
           Priority|P3                          |P2

[Bug target/113994] [13/14 Regression] Probable C++ code generation bug with -O2 on s390x platform

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Bisected to r13-1268-g8c99e307b20c502e55c425897fb3884ba8f05882 .
But that just means the bug was latent before that.

[Bug target/113994] [13/14 Regression] Probable C++ code generation bug with -O2 on s390x platform

[stefansf at linux dot ibm.com]

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

--- Comment #6 from Stefan Schulze Frielinghaus <stefansf at linux dot ibm.com> ---
Looks like wrong liveness information.  The problem is that df_analyze_loop
only considers basic blocks which strictly belong to a loop which is not
enough.  During loop2_doloop basic block 9 (previously 8) embodies the CC
consumer jump_insn 42 and is not part of the loop and therefore does not
contribute to the liveness analysis.  A quick and dirty experiment by forcing a
merge with BB 9

diff --git a/gcc/df-core.cc b/gcc/df-core.cc
index f0eb4c93957..79f37e22ec1 100644
--- a/gcc/df-core.cc
+++ b/gcc/df-core.cc
@@ -957,9 +957,11 @@ df_worklist_propagate_backward (struct dataflow *dataflow,
   if (EDGE_COUNT (bb->succs) > 0)
     FOR_EACH_EDGE (e, ei, bb->succs)
       {
-       if (bbindex_to_postorder[e->dest->index] < last_change_age.length ()
+       if ((bbindex_to_postorder[e->dest->index] < last_change_age.length ()
            && age <= last_change_age[bbindex_to_postorder[e->dest->index]]
            && bitmap_bit_p (considered, e->dest->index))
+           || (strcmp ("loop2_doloop", current_pass->name) == 0
+               && e->src->index == 6 && e->dest->index == 9))
           changed |= dataflow->problem->con_fun_n (e);
       }
   else if (dataflow->problem->con_fun_0)

shows that, now, CC is live at BB 6 and therefore doloop performs no
transformation due to

bool fail = bitmap_intersect_p (df_get_live_out (loop_end), modified);
BITMAP_FREE (modified);

if (fail)
  {
    if (dump_file)
      fprintf (dump_file, "Doloop: doloop pattern clobbers live out\n");
    return false;
  }

In a first try I enlarged the set of basic blocks for which df_analyze_loop is
run to also include basic blocks which have a direct edge originating from a
basic block of a loop.  Of course, this solves this problem.  However, in
general this may not be enough.  I'm wondering what the IL allows.  Is it
possible to have a graph containing not only outgoing edges of a loop but also
ingoing?  If so I think we would need to compute the set of basic blocks which
are reachable from within the loop.  Any thoughts?

[Bug target/113994] [13/14 Regression] Probable C++ code generation bug with -O2 on s390x platform

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
CCing Richi and Honza.

[Bug target/113994] [13/14 Regression] Probable C++ code generation bug with -O2 on s390x platform

[rguenth at gcc dot gnu.org]

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

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
Created attachment 57574
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57574&action=edit
patch

A quick-and-dirty thing would be indeed to simply include all exit blocks
like with the attached.

I'll note that df_analyze_loop has serious scalability issues so IMO we
should try to get rid of it and its uses.  I don't know the doloop pass
at all (invariant motion also uses this function), but it would be best
if it could do df_analyze on the whole function and either decide it
doesn't need to update DF during transform since it doesn't effect other
loops it processes or it would update DF info manually.

[Bug target/113994] [13/14 Regression] Probable C++ code generation bug with -O2 on s390x platform

[rguenth at gcc dot gnu.org]

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

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
Ick, it's via iv_analysis_loop_init used from doloop and unroll.

[Bug target/113994] [13/14 Regression] Probable C++ code generation bug with -O2 on s390x platform

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #8)

flow_bb_inside_loop_p( has missing space before (.
Anyway, getting rid of its use would likely not be backportable...

[Bug target/113994] [13/14 Regression] Probable C++ code generation bug with -O2 on s390x platform

[rguenth at gcc dot gnu.org]

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

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #9)
> Ick, it's via iv_analysis_loop_init used from doloop and unroll.

In that context I'll note that if it is doloop itself that does the DF
live query then it's doloops fault to not operate within the constraints
of iv_analysis_loop_init.  It might be possible to add a flag to
iv_analysis_loop_init to elide its df_analyze stuff and instead have the
caller do a function-wide DF analysis.

I see doloop_optimize_loops does

  if (optimize == 1)
    {
      df_live_add_problem ();
      df_live_set_all_dirty ();
    }

  for (auto loop : loops_list (cfun, 0))
    doloop_optimize (loop);

but never calls df_analyze.  I'm not actually sure what happens if you
then do df_analyze_loops, whether that scraps all but the loop BB
info computed?  Anyway, I don't think that simply adding another problem
"around" iv_analysis_loop_init works as we can see here.  If
iv_analysis_loop_init were to compute liveness it would see that it needs
more that just the loop body blocks.

Also note that my patch while it might work in most cases, isn't fool-proof.
The actual use might be in the very last BB of the function (very unlikely
for CC, of course).  Doing an analysis of all reachable blocks perverts
the use of df_analyze_loop.

So.  It looks all broken design to me (in doloop).