[Bug c++/109339] New: stop_token compiled with -Og yields maybe-uninitialized

[Bug c++/109339] New: stop_token compiled with -Og yields maybe-uninitialized

[gcc at heine dot tech]

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

            Bug ID: 109339
           Summary: stop_token compiled with -Og yields
                    maybe-uninitialized
           Product: gcc
           Version: 12.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gcc at heine dot tech
  Target Milestone: ---

This happens since gcc 12.

This snippet compiled with -std=c++20 -Og -Wmaybe-uninitialized

```
#include <stop_token>

int main() {
    std::stop_source ss;
}
```

yields this warning

```
In constructor 'std::stop_source::stop_source()',
    inlined from 'int main()' at <source>:4:22:
/opt/compiler-explorer/gcc-12.2.0/include/c++/12.2.0/stop_token:480:21:
warning: 'ss' may be used uninitialized [-Wmaybe-uninitialized]
  480 |     stop_source() : _M_state(*this)
      |                     ^~~~~~~~~~~~~~~
/opt/compiler-explorer/gcc-12.2.0/include/c++/12.2.0/stop_token: In function
'int main()':
/opt/compiler-explorer/gcc-12.2.0/include/c++/12.2.0/stop_token:397:7: note: by
argument 2 of type 'const std::stop_source&' to
'std::stop_token::_Stop_state_ref::_Stop_state_ref(const std::stop_source&)'
declared here
  397 |       _Stop_state_ref(const stop_source&)
      |       ^~~~~~~~~~~~~~~
<source>:4:22: note: 'ss' declared here
    4 |     std::stop_source ss;
      |                      ^~
```

Godbolt: https://godbolt.org/z/KofErdEzY

[Bug c++/109339] stop_token compiled with -Og yields maybe-uninitialized

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2023-03-29
           Keywords|                            |diagnostic
     Ever confirmed|0                           |1

[Bug c++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|                            |11.3.1
                 CC|                            |rguenth at gcc dot gnu.org
      Known to fail|                            |12.1.0, 13.0
            Summary|stop_token compiled with    |[12/13 Regression]
                   |-Og yields                  |stop_token compiled with
                   |maybe-uninitialized         |-Og yields
                   |                            |maybe-uninitialized

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The warning started with r12-6677

    ipa/103989 - avoid IPA inlining of small functions with -Og

    The following change avoids doing IPA inlining of small functions
    into functions compiled with -Og - those functions will see almost no
    followup scalar cleanups so that the benefit anticipated by the
    inliner will not be realized and instead the late diagnostic code
    will be confused by dead code that is left around.

[Bug libstdc++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|tree-optimization           |libstdc++

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
The diagnostic is intentional I think.  We see

<bb 2> [local count: 1073741824]:
ss ={v} {CLOBBER};
std::stop_token::_Stop_state_ref::_Stop_state_ref (&ss._M_state, &ss);
std::stop_source::~stop_source (&ss);
ss ={v} {CLOBBER(eol)};
return 0;

and the call to _Stop_state_ref passes references to uninitialized 'ss'
(tree-ssa-uninit.cc:maybe_warn_pass_by_reference).  The &ss argument
is a const reference and the function does

      _Stop_state_ref(const _Stop_state_ref& __other) noexcept
      : _M_ptr(__other._M_ptr)
      {
 if (_M_ptr)
   _M_ptr->_M_add_owner();
      }

so it inspects __other._M_ptr.  It looks like for some reason the NSDMI
init of _M_ptr isn't happening?

The FE emits

;; Function std::stop_source::stop_source() (null)
;; enabled by -tree-original


<<cleanup_point <<< Unknown tree: expr_stmt
  *(struct stop_source *) this = {CLOBBER} >>>>>;
{
  <<cleanup_point <<< Unknown tree: expr_stmt
    std::stop_token::_Stop_state_ref::_Stop_state_ref (&((struct stop_source *)
this)->_M_state, (const struct stop_source &) this) >>>>>;
  try
    {

    }
  catch
    {
      std::stop_token::_Stop_state_ref::~_Stop_state_ref (&((struct stop_source
*) this)->_M_state);
    }
}

but it's all a bit convoluted?  To me it looks like a library issue.  Yes,
with more inlining we optimize everything away.

[Bug libstdc++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Other than that in principle maybe_warn_pass_by_reference could be enhanced
by realizing a (sub-)object is passed both as const reference and
non-const reference as in this case, but it would be still an odd API.

[Bug libstdc++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.3

[Bug libstdc++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[redi at gcc dot gnu.org]

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

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #2)
> The diagnostic is intentional I think.  We see
> 
> <bb 2> [local count: 1073741824]:
> ss ={v} {CLOBBER};
> std::stop_token::_Stop_state_ref::_Stop_state_ref (&ss._M_state, &ss);
> std::stop_source::~stop_source (&ss);
> ss ={v} {CLOBBER(eol)};
> return 0;
> 
> and the call to _Stop_state_ref passes references to uninitialized 'ss'
> (tree-ssa-uninit.cc:maybe_warn_pass_by_reference).  The &ss argument
> is a const reference and the function does
> 
>       _Stop_state_ref(const _Stop_state_ref& __other) noexcept

That function is never used in this code though. Why is code being emitted for
it?

We don't copy a _Stop_state_ref, we construct one using this constructor:

      _Stop_state_ref(const stop_source&)
      : _M_ptr(new _Stop_state_t())
      { }

This has a reference to ss but doesn't inspect it at all.


>       : _M_ptr(__other._M_ptr)
>       {
>  if (_M_ptr)
>    _M_ptr->_M_add_owner();
>       }
> 
> so it inspects __other._M_ptr.  It looks like for some reason the NSDMI
> init of _M_ptr isn't happening?

Because an NSDMI is not used if the constructor inits it explicitly, like this
one does. But this constructor is never used anyway, it's an unused inline
function so should not be emitted, and we should not get warnings for it.

[Bug libstdc++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[redi at gcc dot gnu.org]

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

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #4)
> (In reply to Richard Biener from comment #2)
> > The diagnostic is intentional I think.  We see
> > 
> > <bb 2> [local count: 1073741824]:
> > ss ={v} {CLOBBER};
> > std::stop_token::_Stop_state_ref::_Stop_state_ref (&ss._M_state, &ss);
> > std::stop_source::~stop_source (&ss);
> > ss ={v} {CLOBBER(eol)};
> > return 0;
> > 
> > and the call to _Stop_state_ref passes references to uninitialized 'ss'
> > (tree-ssa-uninit.cc:maybe_warn_pass_by_reference).  The &ss argument
> > is a const reference and the function does
> > 
> >       _Stop_state_ref(const _Stop_state_ref& __other) noexcept
> 
> That function is never used in this code though. Why is code being emitted
> for it?

Actually I don't think any code is being emitted for it, so that's fine. That
function is a red herring, but not involved here at all.

The warning is for this one:

> We don't copy a _Stop_state_ref, we construct one using this constructor:
> 
>       _Stop_state_ref(const stop_source&)
>       : _M_ptr(new _Stop_state_t())
>       { }
> 
> This has a reference to ss but doesn't inspect it at all.

It can't inspect it, because it's not even named. The function has no way to
possibly access the uninitialized object through that reference.

[Bug libstdc++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
OK, I suppose I was misled by trying to trace how the construction works.  The
first half of my comment#2 still stands - we diagnose

  ss ={v} {CLOBBER};
  std::stop_token::_Stop_state_ref::_Stop_state_ref (&ss._M_state, &ss);

by (maybe broken) design, the &ss argument is a const reference which
we decide implies read. In this case we could even use modref to
see the parameter is unused - but the call happens through an alias
and in the -Og pipeline we do not run late modref.

The called body is

void std::stop_token::_Stop_state_ref::_Stop_state_ref (struct _Stop_state_ref
* const this, const struct stop_source & D.79270)
{
  void * _5;

  <bb 2> [local count: 1073741824]:
  _5 = operator new (24);
  MEM[(struct _Stop_state_t *)_5]._M_owners.D.16955._M_i = 0;
  MEM[(struct _Stop_state_t *)_5]._M_value.D.16955._M_i = 0;
  MEM[(struct _Stop_state_t *)_5]._M_head = 0B;
  MEM[(struct _Stop_state_t *)_5]._M_requester._M_thread = 0;
  MEM[(struct __atomic_base *)_5]._M_i = 1;
  MEM[(struct __atomic_base *)_5 + 4B]._M_i = 4;
  MEM[(struct id *)_5 + 16B]._M_thread = 0;
  this_2(D)->_M_ptr = _5;
  return;

}

but since this function can be interposed even modref doesn't help (when
scheduled and enabled) since it throws away this knowledge :/  Maybe
we need some optimistic mode for diagnostic code (or add
EAF_LIKELY_UNUSED).

But as said (late) modref isn't in the -Og pipeline and it's only enabled
with -O2+ anyway.

The other possible heuristic adjustment would be noticing &ss is also
passed as first non-const reference argument.

[Bug libstdc++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[redi at gcc dot gnu.org]

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

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
OK I suppose we can change the library to avoid passing a reference there.

[Bug libstdc++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
What is the
      explicit
      _Stop_state_ref(const stop_source&)
      : _M_ptr(new _Stop_state_t())
      { }
there for, can't you use some completely unrelated type there instead (bool,
stop_source*, some enum, whatever), or drop the const at least?
The passing pointer to uninitialized const object or reference to uninitialized
const object to function we don't know anything about is I think one of the
design goals of Martin's change which probably not everybody will agree with,
but it is true that it can sometimes find bugs in code.

[Bug libstdc++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[redi at gcc dot gnu.org]

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

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yes, we can pass something else there instead.

It would be nice if this worked to silence the warning though:

--- a/libstdc++-v3/include/std/stop_token
+++ b/libstdc++-v3/include/std/stop_token
@@ -395,10 +395,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     {
       _Stop_state_ref() = default;

+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
+      __attribute__((__access__(__none__, 1)))
       explicit
       _Stop_state_ref(const stop_source&)
       : _M_ptr(new _Stop_state_t())
       { }
+#pragma GCC diagnostic pop

       _Stop_state_ref(const _Stop_state_ref& __other) noexcept
       : _M_ptr(__other._M_ptr)

It has no effect at all (putting the pragmas around the caller works though).

Now that we have the access attribute, why doesn't access(none, N) help here?
It seems to express exactly what I want to express here, but the warning
doesn't care.

[Bug libstdc++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
__attribute__((__access__(__none__, 2))) on the ctor works, no need to add
pragmas.

[Bug libstdc++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Oh good! Testing that now then ...

[Bug libstdc++/109339] [12/13 Regression] stop_token compiled with -Og yields maybe-uninitialized

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

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

commit r13-6958-ga35e8042fbc7a3eb9cece1fba4cdd3b6cdfb906f
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Mar 31 13:38:14 2023 +0100

    libstdc++: Avoid -Wmaybe-uninitialized warning in std::stop_source
[PR109339]

    We pass a const-reference to *this before it's constructed, and GCC
    assumes that all const-references are accessed. Add the access attribute
    to say it's not accessed.

    libstdc++-v3/ChangeLog:

            PR libstdc++/109339
            * include/std/stop_token (_Stop_state_ptr(const stop_source&)):
            Add attribute access with access-mode 'none'.
            * testsuite/30_threads/stop_token/stop_source/109339.cc: New test.

[Bug libstdc++/109339] [12 Regression] stop_token compiled with -Og yields maybe-uninitialized

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[12/13 Regression]          |[12 Regression] stop_token
                   |stop_token compiled with    |compiled with -Og yields
                   |-Og yields                  |maybe-uninitialized
                   |maybe-uninitialized         |

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk.

[Bug libstdc++/109339] [12 Regression] stop_token compiled with -Og yields maybe-uninitialized

[cvs-commit at gcc dot gnu.org]

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

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:47880309516fd5c913102eb4c52dc86da7051983

commit r12-9486-g47880309516fd5c913102eb4c52dc86da7051983
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Mar 31 13:38:14 2023 +0100

    libstdc++: Avoid -Wmaybe-uninitialized warning in std::stop_source
[PR109339]

    We pass a const-reference to *this before it's constructed, and GCC
    assumes that all const-references are accessed. Add the access attribute
    to say it's not accessed.

    libstdc++-v3/ChangeLog:

            PR libstdc++/109339
            * include/std/stop_token (_Stop_state_ptr(const stop_source&)):
            Add attribute access with access-mode 'none'.
            * testsuite/30_threads/stop_token/stop_source/109339.cc: New test.

    (cherry picked from commit a35e8042fbc7a3eb9cece1fba4cdd3b6cdfb906f)

[Bug libstdc++/109339] [12 Regression] stop_token compiled with -Og yields maybe-uninitialized

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

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

--- Comment #15 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for 12.3