[Bug libstdc++/98108] New: Broken Schwarz counter for iostreams initialization

[Bug libstdc++/98108] New: Broken Schwarz counter for iostreams initialization

[i.hamsa at gmail dot com]

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

            Bug ID: 98108
           Summary: Broken Schwarz counter for iostreams initialization
           Product: gcc
           Version: 10.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: i.hamsa at gmail dot com
  Target Milestone: ---

Iostreams initialization doesn't work in multithreaded environments.

To reproduce one needs two source files:

// file1.cc

  #include <thread>  

  void threadfunc();  

  struct StaticThread {
      std::thread t;  
      StaticThread() : t(threadfunc) {}
      ~StaticThread() { t.join(); }
  };  

  static StaticThread thread1, thread2;

//file2.cc

  #include <iostream>  

  void threadfunc() {
      std::cout << "Printing\n";
  }  

  int main() {}

//compile
g++ -pthread file1.cc file2.cc

It is important that <iostream> is NOT included in file1.cc.

The resulting executable always crashes on my machine (Gentoo Linux). Add
<iostream> to file1.cc, the crash disappears. Change the order of source files
in the command line, the crash disappears.

---

The culprit is ios_base::Init::_S_refcount. Basically it is an atomic nifty
counter. The problem is, the nifty counter idiom doesn't work with
multithreading. Making it atomic achieves absolutely nothing. If we have two or
more threads that can enter Init::Init() at the same time, one should proceed
and the rest must WAIT. It is the same thing as initialization of a static
object in a block scope, and should be interlocked in exactly the same way.

[Bug libstdc++/98108] Broken Schwarz counter for iostreams initialization

[rguenth at gcc dot gnu.org]

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

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
I think this is undefined since global object initialization order is not
well-defined between TUs so thread1/thread2 and std::cout construction are not
well-ordered.

It probably works (by accident) when doing

g++ -pthread file2.cc file1.cc

[Bug libstdc++/98108] Broken Schwarz counter for iostreams initialization

[i.hamsa at gmail dot com]

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

--- Comment #2 from i.hamsa at gmail dot com ---
The standard streams are not user defined objects. They need to be initialized
before any user code touches them, before `main`, before everything, no matter
what. This is exactly the purpose of the "nifty counter". It works perfectly
well in single-threaded environments.

The problem is that libstdc++ implementation of it purports to be thread-safe.
It isn't.

[Bug libstdc++/98108] Broken Schwarz counter for iostreams initialization

[redi at gcc dot gnu.org]

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I think it's QoI whether this works.

"The objects are constructed and the associations are established at some time
prior to or during the first time an object of class ios_base::Init is
constructed, ..."

"The results of including <iostream> in a translation unit shall be as if
<iostream> defined an instance of ios_base::Init with static storage duration."

So there's no guarantee that the global iostreams will be usable during the
static initialization phase, because there's no guarantee that a given
constructor will be before the first ios_base::Init constructor.

The problem is *not* that the counters use atomics rather than waiting.

Even if the initialization is done as in the patch below, it still crashes:

--- a/libstdc++-v3/src/c++98/ios_init.cc
+++ b/libstdc++-v3/src/c++98/ios_init.cc
@@ -77,7 +77,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION

   ios_base::Init::Init()
   {
-    if (__gnu_cxx::__exchange_and_add_dispatch(&_S_refcount, 1) == 0)
+    struct do_init
+    {
+      do_init()
       {
        // Standard streams default to synced with "C" operations.
        _S_synced_with_stdio = true;
@@ -116,8 +118,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
        // streams are not re-initialized with uses of ios_base::Init
        // besides <iostream> static object, ie just using <ios> with
        // ios_base::Init objects.
-       __gnu_cxx::__atomic_add_dispatch(&_S_refcount, 1);
+       _S_refcount = 1;
       }
+    };
+#ifndef __cpp_threadsafe_static_init
+# warning "ios_base::Init::Init() initialization is not thread-safe"
+#endif
+    static const do_init once __attribute__((unused)) = do_init();
+
+    __gnu_cxx::__exchange_and_add_dispatch(&_S_refcount, 1);
   }

   ios_base::Init::~Init()

This doesn't help, because the thread1 and thread2 objects in file1.cc are
still constructed before the static ios_base::Init object in file2.cc and so
they still try to use std::cout before the first _S_refcount increment has
happened.

Waiting for the iostream initialization to finish doesn't help if it hasn't
started yet. And isn't relevant in your program, because only one
ios_base::Init constructor ever runs in the whole program. Making the second
one wait for the first doesn't help if there is only one.

What does make your program work is:

--- a/libstdc++-v3/include/std/iostream
+++ b/libstdc++-v3/include/std/iostream
@@ -71,7 +71,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
   //@}

   // For construction of filebuffers for cout, cin, cerr, clog et. al.
-  static ios_base::Init __ioinit;
+  static ios_base::Init __ioinit __attribute__((__init_priority__(50)));

 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace

And this works with or without changing the Schwarz counter.

[Bug libstdc++/98108] Broken Schwarz counter for iostreams initialization

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 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:cf4ed3b41594b6935a337fe0aaf8149eadf88751

commit r11-6072-gcf4ed3b41594b6935a337fe0aaf8149eadf88751
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Dec 15 11:40:06 2020 +0000

    libstdc++: Use init_priority attribute for Init object [PR 98108]

    This causes the global objects that run the <iostream> initialization
    code to be constructed earlier, which avoids some bugs in user code due
    to incorrectly relying on static initialization order.

    libstdc++-v3/ChangeLog:

            PR libstdc++/98108
            * include/std/iostream (__ioinit): Add init_priority attribute.

[Bug libstdc++/98108] Broken Schwarz counter for iostreams initialization

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.0
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I've added the init_priority attribute on trunk, which makes this (broken)
program work as expected.

[Bug libstdc++/98108] Broken Schwarz counter for iostreams initialization

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 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:433703843b3fa76bcbba4f1fd782c7ef319b64a8

commit r11-6091-g433703843b3fa76bcbba4f1fd782c7ef319b64a8
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Dec 15 18:40:28 2020 +0000

    libstdc++: Remove init_priority attribute for Init object [PR 98108]

    This reverts commit cf4ed3b41594b6935a337fe0aaf8149eadf88751.

    libstdc++-v3/ChangeLog:

        PR libstdc++/98108
        * include/std/iostream (__ioinit): Remove init_priority attribute.

[Bug libstdc++/98108] Broken Schwarz counter for iostreams initialization

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|FIXED                       |INVALID

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I've reverted the change because it breaks the build for some targes.

So you'll just have to fix your code. The simplest way is to ensure you include
<iostream> before defining a global that wants to use std::cout.

[Bug libstdc++/98108] Broken Schwarz counter for iostreams initialization

[i.hamsa at gmail dot com]

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

--- Comment #8 from i.hamsa at gmail dot com ---
After re-reading the relevant parts of the standard, I have convinced myself
that the program is indeed not required to work unless <iostreams> is included,
even in a single-threaded version. Even though the standard encourages
implementations to initialise the standard iostreams objects earlier than
required, it is not mandatory.

[Bug libstdc++/98108] Broken Schwarz counter for iostreams initialization

[redi at gcc dot gnu.org]

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

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Right, this actually has nothing to do with threads.

The behaviour is exactly the same as if file1.cc contained this instead:

  void threadfunc();  

  struct StaticThread {
      StaticThread() { threadfunc(); }
  };  

  static StaticThread thread1;

A global constructor calls threadfunc() before std::cout has been initialized.
It doesn't matter whether that happens in a separate thread or not, and in
either case the solution is to include <iostream> so that a std::ios_base::Init
object is constructed before your global.

[Bug libstdc++/98108] Broken Schwarz counter for iostreams initialization

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Patrick Palka <ppalka@gcc.gnu.org>:

https://gcc.gnu.org/g:4e4e3ffd10f53ef71696bc728ab40258751a2df4

commit r13-3707-g4e4e3ffd10f53ef71696bc728ab40258751a2df4
Author: Patrick Palka <ppalka@redhat.com>
Date:   Sun Nov 6 11:16:00 2022 -0500

    libstdc++: Move stream initialization into compiled library [PR44952]

    This patch moves the static object for constructing the standard streams
    out from <iostream> and into the compiled library on systems that support
    init priorities.  This'll mean <iostream> no longer introduces a separate
    global constructor in each TU that includes it.

    We can do this only if the init_priority attribute is supported because
    we need a way to ensure the stream initialization runs first before any
    user global initializer, particularly when linking with a static
    libstdc++.a.

            PR libstdc++/44952
            PR libstdc++/39796
            PR libstdc++/98108

    libstdc++-v3/ChangeLog:

            * include/std/iostream (__ioinit): No longer define here if
            the init_priority attribute is usable.
            * src/c++98/ios_init.cc (__ioinit): Define here instead if
            init_priority is usable, via ...
            * src/c++98/ios_base_init.h: ... this new file.