[Bug c++/105373] New: miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[Bug c++/105373] New: miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

            Bug ID: 105373
           Summary: miscompile involving lambda coroutines and an object
                    bitwise copied instead of via the copy constructor
           Product: gcc
           Version: 11.3.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: avi at scylladb dot com
  Target Milestone: ---

This is a bug in a complex piece of code, so I'll need guidance on what further
information to provide (e.g. intermediate code dumps). It reproduces with
various levels of debug information, and the code works with clang.

At the heart there is a shared pointer, which I've enhanced so that all
pointers that point to the same object keep track of each other (also the
pointee points back to one of the pointers). The pointee keeps an incrementing
generation count. In the end I have two pointer objects that are bitwise equal,
even though they should have different generation counts and different
doubly-linked-list pointers. This proves that a bitwise copy happened. It
doesn't prove a miscompile, since my code could have decided to perform a
bitwise copy, but the fact that it works in clang indicates it's a gcc bug. The
bug happens with asan too, and with different sizeof(the smart pointer) so it's
not some stray write.


This is the snippet that causes the trouble:

0                tlogger.info("before updating cache {}",
fmt::ptr(old3.get()));
1                co_await
with_scheduling_group(_config.memtable_to_cache_scheduling_group, [this, old4 =
old3, &newtabs] () -> future<> {
2                    tlogger.info("updating cache {}", fmt::ptr(old4.get()));
3                    return update_cache(old4, newtabs);
4                });


old3 and old4 are all copies of the same smart pointer (there are also old and
old1 and old2 elsewhere, but they are correct). In the call to update_cache(),
we attempt to make a copy of old4, and an internal check finds the link list is
corrupted. Inspecting old4 in the debugger (from the printout in line 0) and
the source of the copy in line 3 shows they are the same, but have different
addresses:


0 INFO  2022-04-25 12:04:14,722 [shard 0] table - before updating cache
0x600000666c40
1 copying @0x60000520bef8 <- @0x6000052108f0 0x600000666c40 refcnt 6 gen 43
2 INFO  2022-04-25 12:04:14,722 [shard 0] table - updating cache 0x600000666c40
3 (gdb) p this
$1 = (const seastar::lw_shared_ptr<replica::memtable> * const) 0x60000520bed0
4 (gdb) p *this
5 $2 = {_ptr = 0x600000666c40, _next = 0x6000052108f0, _prev = 0x60000597ae48,
_generation = 43}
6 (gdb) x/4gx 0x60000520bef8
7 0x60000520bef8:       0x0000600000666c40      0x00006000052108f0
8 0x60000520bf08:       0x000060000597ae48      0x000000000000002b
9 (gdb) x/4gx this
10 0x60000520bed0:      0x0000600000666c40      0x00006000052108f0
11 0x60000520bee0:      0x000060000597ae48      0x000000000000002b

in line 3 I dump old4 (from the printout in line 1, where old3 is copied into
old4). But "this" doesn't point to old4., it points to a bitwise copy of old4,
as shown in lines 7-8 and 10-11.


Note that both old4 and the bad copy "this" are members of the coroutine frame.


I realize this isn't enough to analyze the situation, I'm happy to provide more
information if you direct me how.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #1 from Avi Kivity <avi at scylladb dot com> ---
I randomly looked at 023t.ssa (mainly because I recognized the acronym).

  _45 = frame_ptr_182(D)->__closure;
  _46 = _45->__this;
  frame_ptr_182(D)->D.2159984_4_7 =
_46->_config.memtable_to_cache_scheduling_group;
  _47 = &frame_ptr_182(D)->D.2159792_4_7;
  _48 = frame_ptr_182(D)->__closure;
  _49 = &_48->__old3;
  seastar::lw_shared_ptr<replica::memtable>::lw_shared_ptr (_47, _49);
  _50 = frame_ptr_182(D)->__closure;
  _51 = _50->__this;
  frame_ptr_182(D)->D.2159793_4_7.__this = _51;
  frame_ptr_182(D)->D.2159793_4_7.__old4 = frame_ptr_182(D)->D.2159792_4_7;
  _52 = frame_ptr_182(D)->__closure;
  _53 = _52->__newtabs;
  frame_ptr_182(D)->D.2159793_4_7.__newtabs = _53;
  _54 = &frame_ptr_182(D)->D.2159793_4_7;
  frame_ptr_182(D)->D.2160366_4_7 =
seastar::with_scheduling_group<replica::table::try_flush_memtable_to_sstable(replica::table::try_flush_memtable_to_sstable(seastar::lw_shared_ptr<replica::memtable>,
sstable_write_permit&&)::_ZN7replica5table29try_flush_memtable_to_sstableEN7seastar13lw_shared_ptrINS_8memtableEEEO20sstable_write_permit.Frame*)::<lambda()>::operator()(replica::table::try_flush_memtable_to_sstable(replica::table::try_flush_memtable_to_sstable(seastar::lw_shared_ptr<replica::memtable>,
sstable_write_permit&&)::_ZN7replica5table29try_flush_memtable_to_sstableEN7seastar13lw_shared_ptrINS_8memtableEEEO20sstable_write_permit.Frame*)::<lambda()>
mutable::_ZZN7replica5table29try_flush_memtable_to_sstableEN7seastar13lw_shared_ptrINS_8memtableEEEO20sstable_write_permitENUlvE_clEv.Frame*)::<lambda()>::operator()(replica::table::try_flush_memtable_to_sstable(replica::table::try_flush_memtable_to_sstable(seastar::lw_shared_ptr<replica::memtable>,
sstable_write_permit&&)::_ZN7replica5table29try_flush_memtable_to_sstableEN7seastar13lw_shared_ptrINS_8memtableEEEO20sstable_write_permit.Frame*)::<lambda()>::operator()(replica::table::try_flush_memtable_to_sstable(replica::table::try_flush_memtable_to_sstable(seastar::lw_shared_ptr<replica::memtable>,
sstable_write_permit&&)::_ZN7replica5table29try_flush_memtable_to_sstableEN7seastar13lw_shared_ptrINS_8memtableEEEO20sstable_write_permit.Frame*)::<lambda()>
mutable::_ZZN7replica5table29try_flush_memtable_to_sstableEN7seastar13lw_shared_ptrINS_8memtableEEEO20sstable_write_permitENUlvE_clEv.Frame*)::<lambda()>
mutable::_ZZZN7replica5table29try_flush_memtable_to_sstableEN7seastar13lw_shared_ptrINS_8memtableEEEO20sstable_write_permitENUlvE_clEvENUlvE1_clEv.Frame*)::<lambda()>
> (frame_ptr_182(D)->D.2159984_4_7, _54); [return slot optimization]
  _55 = &frame_ptr_182(D)->D.2160366_4_7;
  frame_ptr_182(D)->Aw5_4_7 = seastar::operator co_await<void> (_55); [return
slot optimization]


In the line

  frame_ptr_182(D)->D.2159793_4_7.__old4 = frame_ptr_182(D)->D.2159792_4_7;


Is this not a bitwise copy, which should be instead a call to the copy
constructor?

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #2 from Avi Kivity <avi at scylladb dot com> ---
I see it in 006t.gimple too:

                        try
                          {
                            _50 = frame_ptr->__closure;
                            _51 = _50->__this;
                            frame_ptr->D.2159793_4_7.__this = _51;
                            frame_ptr->D.2159793_4_7.__old4 =
frame_ptr->D.2159792_4_7;
                            _52 = frame_ptr->__closure;
                            _53 = _52->__newtabs;
                            frame_ptr->D.2159793_4_7.__newtabs = _53;


newtabs is captured by reference, so the copy from _53 is reasonable. But old4
is captured by value, and so should go through the copy constructor.

Looking at references to old3, it always has its address taken and then passed
to the copy constructor.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #3 from Avi Kivity <avi at scylladb dot com> ---
I see this alleged copy in gcc 10.2.1 too.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[iains at gcc dot gnu.org]

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

--- Comment #4 from Iain Sandoe <iains at gcc dot gnu.org> ---
I'm guessing that a reproducer is going to be hard to arrange (from the
"complex piece of code") even though the failing point is well-defined?

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #5 from Avi Kivity <avi at scylladb dot com> ---
Can you confirm my suspicion about the copying of __old4 in gimple? If that's
the culprit I believe I can mechanically reduce the reproducer to something
easily manageable.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #6 from Avi Kivity <avi at scylladb dot com> ---
Some more findings: if I replace the lambda (which is not a coroutine, but is
contained in a coroutine lambda) with an equivalent struct, the problem goes
away, both at runtime and in terms of an __old4 copy. 
+                struct inner_lambda {
+                    table* zis;
+                    lw_shared_ptr<memtable> old4;
+                    std::vector<sstables::shared_sstable>& newtabs;
+                    inner_lambda(table* zis, lw_shared_ptr<memtable>& old3,
std::vector<sstables::shared_sstable>& newtabs)
+                            : zis(zis), old4(old3), newtabs(newtabs) {}
+                    future<> operator()() {
+                        tlogger.info("updating cache {}",
fmt::ptr(old4.get()));
+                        return zis->update_cache(old4, newtabs);
+                    }
+                };
+
                 tlogger.info("before updating cache {}",
fmt::ptr(old3.get()));
-                co_await
with_scheduling_group(_config.memtable_to_cache_scheduling_group, [this, old4 =
old3, &newtabs] () -> future<> {
+                co_await
with_scheduling_group(_config.memtable_to_cache_scheduling_group, /* [this,
old4 = old3, &newtabs] () mutable -> future<> {
                     tlogger.info("updating cache {}", fmt::ptr(old4.get()));
                     return update_cache(old4, newtabs);
-                });
+                } */ inner_lambda(this, old3, newtabs));
                 tlogger.info("updating cache {} done", fmt::ptr(old3.get()));
                 _memtables->erase(old3);
                 tlogger.debug("Memtable for {}.{} replaced, into {} sstables",
old3->schema()->ks_name(), old3->schema()->cf_name(), newtabs.size());
                 tlogger.info("try_flush_memtable_to_sstable post_flush:end old
{} refcnt {}", fmt::ptr(old3.get()), old3.use_count());
                 co_return stop_iteration::yes;

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #7 from Avi Kivity <avi at scylladb dot com> ---
To give more context:

    struct ._anon_973 D.2159793 [value-expr: frame_ptr->D.2159793_4_7];
    struct lw_shared_ptr D.2159792 [value-expr: frame_ptr->D.2159792_4_7];

(I determined that _anon_973 is the non-coroutine-lambda that is giving me
grief)

                            frame_ptr->D.2159793_4_7.__this = _51;
                            frame_ptr->D.2159793_4_7.__old4 =
frame_ptr->D.2159792_4_7;
                            _52 = frame_ptr->__closure;
                            _53 = _52->__newtabs;
                            frame_ptr->D.2159793_4_7.__newtabs = _53;


Here I suspect copying __old4 is bypassing the copy constructor. The source of
the copy (D.2159792) has lw_shared_ptr type.

I'm not fluent in gimple so maybe I'm misinterpreting it.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #8 from Avi Kivity <avi at scylladb dot com> ---
Hoping this is a duplicate of PR105287. Checking.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #9 from Avi Kivity <avi at scylladb dot com> ---
I think it makes it worse, now seeing similar crashes, but earlier.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #10 from Avi Kivity <avi at scylladb dot com> ---
I have something like this:

│     2680  future<> system_keyspace_make(distributed<replica::database>&
dist_db, distributed<service::storage_service>& dist_ss,
sharded<gms::gossiper>& dist_gossiper, db::config& cfg) {               │
...
                                                                               
                                        │
│     2687      for (auto&& table : system_keyspace::all_tables(db_config)) {   

...

│     2695              co_await db.create_keyspace(ksm,
dist_ss.local().get_erm_factory(), true,
replica::database::system_keyspace::yes);   


And I see a crash as if the std::vector returned by all_tables() is not
persisted. This worked in previous gcc versions and works in clang.

I don't now what the standard says, but it really should persist temporaries in
range for that contains a suspension point, or a lot of buggy code will be
written.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #11 from Avi Kivity <avi at scylladb dot com> ---
Created attachment 52899
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52899&action=edit
simple(r) reproducer

I was able to reduce it to something simple (102 lines), but I was only able to
verify that wrong code is generated on the gimple level (where my understanding
is very limited). Still, to my inexperienced eyes it looks like an object with
a copy constructor is being copied bitwise.

Compile with (gcc 11.3.1)

    g++ -Wfatal-errors -w -std=gnu++20  -march=westmere -fvisibility=hidden  -c
-o table.o table.i -fdump-tree-gimple -g 

and observe in table.i.006t.gimple:

    struct lw_shared_ptr D.2740 [value-expr: frame_ptr->D.2740_4_5];

and then

    frame_ptr->D.2741_4_5.__old4 = frame_ptr->D.2740_4_5;


in other places, variables of the same type are copied using the copy
constructor.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[iains at gcc dot gnu.org]

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

--- Comment #12 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Avi Kivity from comment #8)
> Hoping this is a duplicate of PR105287. Checking.

I doubt it, it would seem more likely that the temporary value representing the
non-coroutine lambda object is not being promoted to a frame copy.  The
additional tightening of constraints in the PR105287 fix (which I've now made a
patch to back out of) could well be the cause of the earlier fails you now see.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[iains at gcc dot gnu.org]

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

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-04-29
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #13 from Iain Sandoe <iains at gcc dot gnu.org> ---
I can repeat your observation on master; but at the moment not sure where the
problem lies.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #14 from Avi Kivity <avi at scylladb dot com> ---
Do you confirm that my observation about the generated gimple corresponds to
the bug?

If so, is there a central point where this anonymous variables are named?
Perhaps we can breakpoint on D.2741 and see where the copy is generated.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #15 from Avi Kivity <avi at scylladb dot com> ---
I see that both sides of the assignment are created in flatten_await_stmt, case
      case TARGET_EXPR.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #16 from Avi Kivity <avi at scylladb dot com> ---
Bug hasn't fixed itself as of ccb9c7b129206209cfc315ab1a0432b5f517bdd9.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[avi at scylladb dot com]

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

--- Comment #17 from Avi Kivity <avi at scylladb dot com> ---
This was apparently fixed by 58a7b1e354530d8dfe7d8fb859c8b8b5a9140f1f. At
least, I no longer observe copying of __old4 in gimple. Instead, I see
addresses taken, and if I squint I can see that object properly destroyed later
on.

I can't test on my full-scale program due to
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98056.

I note that one of the duplicates is titled "When constructing object, calling
function and performing co_await in same statement, temporary is erroneously
moved trivially", which is exactly what this PR is about, so it can be safely
marked as a duplicate as well.

Please consider backporting https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576,
this is a subtle code generation bug.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[iains at gcc dot gnu.org]

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

Iain Sandoe <iains at gcc dot gnu.org> changed:

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

--- Comment #18 from Iain Sandoe <iains at gcc dot gnu.org> ---
reported fixed by r13-4479-g58a7b1e354530d.

[Bug c++/105373] miscompile involving lambda coroutines and an object bitwise copied instead of via the copy constructor

[iains at gcc dot gnu.org]

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

--- Comment #19 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Avi Kivity from comment #17)

> Please consider backporting
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99576, this is a subtle code
> generation bug.

Noted, that a back-port is needed, unfortunately the one-line fix for master is
not applicable to the branches (they do not have the code that marks
TARGET_EXPRESSIONS that will be elided).  We will have to figure out how to
make a stand-alone implementation of that for GCC-12, 11 etc.