[Bug c++/101355] New: compiling coroutines with ubsan emits bogus -Wmaybe-uninitialized warnings

[Bug c++/101355] New: compiling coroutines with ubsan emits bogus -Wmaybe-uninitialized warnings

[daklishch at gmail dot com]

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

            Bug ID: 101355
           Summary: compiling coroutines with ubsan emits bogus
                    -Wmaybe-uninitialized warnings
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: daklishch at gmail dot com
  Target Milestone: ---

Created attachment 51111
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51111&action=edit
preprocessed source

Compiling the following program with UBSan emits -Wmaybe-uninitialized on
`<anonymous>' variables.
The compiler and runtime behavior is somewhat inconsistent. Normally, there is
only one warning per function and the program is not affected. However, for
example, compiling provided program with -O2 emits two identical warnings and
makes the program crash with `member call on null pointer of type 'struct a''.


$ cat testcode.cc
#include <coroutine>

struct coro {
        struct promise_type {
                coro get_return_object() { return {}; }
                std::suspend_never initial_suspend() noexcept { return {}; }
                std::suspend_never final_suspend() noexcept { return {}; }
                void unhandled_exception() {}
                void return_void() {}
        };

        bool await_ready() { return true; }
        void await_resume() {}
        template <typename U>
        void await_suspend(U &) {}
};

struct b {
        ~b() {}
};

struct a {
        a(b) {}
        ~a() {}
};

coro f(b obj) {
        auto obj2 = a{obj};
        co_return;
}

int main() {
        f({});
}
$ ~/gcc-dev/bin/gccdev -O2 -std=c++20 -Wall -fsanitize=undefined testcode.cc
-lstdc++
testcode.cc: In function ‘void _Z1f1b.actor(f(b)::_Z1f1b.frame*)’:
testcode.cc:30:1: warning: ‘<anonymous>’ may be used uninitialized
[-Wmaybe-uninitialized]
   30 | }
      | ^
testcode.cc:30:1: note: ‘<anonymous>’ was declared here
   30 | }
      | ^
In function ‘void _Z1f1b.actor(f(b)::_Z1f1b.frame*)’,
    inlined from ‘coro f(b)’ at testcode.cc:27:6:
testcode.cc:30:1: warning: ‘<anonymous>’ is used uninitialized
[-Wuninitialized]
   30 | }
      | ^
testcode.cc: In function ‘coro f(b)’:
testcode.cc:30:1: note: ‘<anonymous>’ was declared here
   30 | }
      | ^
$ ./a.out
testcode.cc:30:1: runtime error: member call on null pointer of type 'struct a'

[Bug c++/101355] compiling coroutines with ubsan emits bogus -Wmaybe-uninitialized warnings

[daklishch at gmail dot com]

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

--- Comment #1 from Dan Klishch <daklishch at gmail dot com> ---
Created attachment 51112
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51112&action=edit
gcc with -v option output

[Bug c++/101355] compiling coroutines with ubsan emits bogus -Wmaybe-uninitialized warnings

[daklishch at gmail dot com]

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

--- Comment #2 from Dan Klishch <daklishch at gmail dot com> ---
GCC incorrectly gimplifies the program. The code that is causing the warning is
in the coroutine's actor function:
              try
                {
                  D.9829 = &frame_ptr->__p;
                  .UBSAN_NULL (D.9829, 4B, 0);
                  coro::promise_type::return_void (D.9829);
                  goto final.suspend;
                }
              finally
                {
                  .UBSAN_NULL (D.9828, 4B, 0); // here
                  a::~a (D.9828);
                }
Obviously, an assignment to D.9828 is missing. However, a little bit earlier a
similar destruction of `struct a' is handled correctly:
                      try
                        {
                          b::~b (&D.9562);
                        }
                      catch
                        {
                          D.9828 = &frame_ptr->__obj.2.3;
                          .UBSAN_NULL (D.9828, 4B, 0);
                          a::~a (D.9828);
                        }

I guess one of this destructor calls is a copy of another and this might be the
root of the problem. After ubsan instrumentation the call to the destructor
looks like this:
        a::~a (.UBSAN_NULL (SAVE_EXPR <&frame_ptr->__obj.2.3>, 4B, 0);,
SAVE_EXPR <&frame_ptr->__obj.2.3>;);

I believe the same SAVE_EXPR is copied to the second invocation of the
destructor but the enclosed expression evaluation is placed only before the
first use of SAVE_EXPR and the control flow does not reach it before a call to
the actual (second) destructor.

I guess this can be fixed by instrumenting the calls to the destructors using
temporary variable and not SAVE_EXPR, like this:
  void *ptr = &frame_ptr->__obj.2.3;
  .UBSAN_NULL (ptr, 4B, 0);
  a::~a (ptr);

But I don't have a solid understanding of GCC internals, so I'm not sure if it
is right.

[Bug c++/101355] incorrect `this' in destructor calls when compiling coroutines with ubsan

[iains at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=95137
           Keywords|                            |wrong-code
     Ever confirmed|0                           |1
                 CC|                            |iains at gcc dot gnu.org
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2021-08-30

--- Comment #3 from Iain Sandoe <iains at gcc dot gnu.org> ---
the two cases seem likely related - if not actually the same.  At present, I
was not sure if this is a coroutines bug (and have not had time to look at it
in detail).

The root cause does appear to be the same - that the sanitiser is expecting the
temp (D.9828 in your case) to be valid - but there is some path through the
code where a suspension occurs so that this is not true.

Unfortunately, I'm not familiar enough with the sanitizer code to comment on
whether the proposed fix is the right one.

[Bug c++/101355] incorrect `this' in destructor calls when compiling coroutines with ubsan

[iains at gcc dot gnu.org]

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

--- Comment #4 from Iain Sandoe <iains at gcc dot gnu.org> ---
patch proposed here:
https://gcc.gnu.org/pipermail/gcc-patches/2021-August/578401.html

[Bug c++/101355] incorrect `this' in destructor calls when compiling coroutines with ubsan

[daklishch at gmail dot com]

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

Dan Klishch <daklishch at gmail dot com> changed:

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

--- Comment #5 from Dan Klishch <daklishch at gmail dot com> ---
GCC stopped instrumenting destructors in this particular case, so I guess the
bug is fixed.

https://godbolt.org/z/KGa6aGf5x

[Bug c++/101355] incorrect `this' in destructor calls when compiling coroutines with ubsan

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

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

--- Comment #6 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Would it be possible to backport this to gcc 11 too?

[Bug c++/101355] incorrect `this' in destructor calls when compiling coroutines with ubsan

[iains at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.3

--- Comment #7 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Marek Polacek from comment #6)
> Would it be possible to backport this to gcc 11 too?

it's on my TODO to back port the correctness fixes to 11 (and where feasible
10).