[Bug libstdc++/12855] New: Thread safety problems in ios_base::Init

[Bug libstdc++/12855] New: Thread safety problems in ios_base::Init

[peturr02 at ru dot is]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855

           Summary: Thread safety problems in ios_base::Init
           Product: gcc
           Version: 3.4
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: libstdc++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: peturr02 at ru dot is
                CC: gcc-bugs at gcc dot gnu dot org
 GCC build triplet: i686-pc-linux-gnu
  GCC host triplet: i686-pc-linux-gnu
GCC target triplet: i686-pc-linux-gnu

ios_base::Init::Init()
  {
    if (_S_ios_base_init == 0)
      {

        _S_ios_base_init = 1;
      }
    ++_S_ios_base_init;
  }

 ios_base::Init::~Init()
  {
    if (--_S_ios_base_init == 1)
      {
        // Catch any exceptions thrown by basic_ostream::flush()
        try
          { 
            // Flush standard output streams as required by 27.4.2.1.6
            cout.flush();
            cerr.flush();
            clog.flush();
    
#ifdef _GLIBCXX_USE_WCHAR_T
            wcout.flush();
            wcerr.flush();
            wclog.flush();    
#endif
          }
        catch (...)
          { }
      }
  }

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[peturr02 at ru dot is]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855



------- Additional Comments From peturr02 at ru dot is  2003-10-31 09:48 -------
(I somehow hit commit before it was ready)

ios_base::Init is not threadsafe:

ios_base::Init::Init()
{
  if (_S_ios_base_init == 0)
    {
      // Lots of stuff that initializes cin, cout etc.

      _S_ios_base_init = 1;
    }
  ++_S_ios_base_init;
}

ios_base::Init::~Init()
{
  if (--_S_ios_base_init == 1)
    {
       // Flush cout, cerr etc.
    }
}

If two threads run Init::Init() or Init::~Init() at the same time, both will
access the standard streams, which are not threadsafe themselves. Also,
_S_ios_base_init is not updated atomically, so it may be corrupted.

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[rittle at latour dot rsch dot comm dot mot dot com]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855



------- Additional Comments From rittle at latour dot rsch dot comm dot mot dot com  2003-11-03 23:52 -------
Subject: Re:  New: Thread safety problems in ios_base::Init


>http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855
>
>           Summary: Thread safety problems in ios_base::Init

Is this a hypothetical PR or actually seen in practice?

Given the usage of std::ios_base::Init by the library, I'd need more
information on how this could possibly fail in practice.  The
constructor (for a global static) will run once before threading could
possibly have started.  Likewise, there is only one thread in the
system which may run destructors (for any global statics).

Please describe the actual system setup where you see these
assumptions violated.  std::ios_base::Init should never be used
outside global objects.

Regards,
Loren

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[pinskia at gcc dot gnu dot org]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855


pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|                            |1
   Last reconfirmed|0000-00-00 00:00:00         |2003-11-04 04:43:52
               date|                            |


------- Additional Comments From pinskia at gcc dot gnu dot org  2003-11-04 04:43 -------
Say two threads dlopen two different shared library at the same time, the initializers will 
then be exucted at the same time not know and might be over writing the value as ++ is 
not atomic.

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[peturr02 at ru dot is]

PLEASE REPLY TO gcc-bugzilla@gcc.gnu.org ONLY, *NOT* gcc-bugs@gcc.gnu.org.

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855



------- Additional Comments From peturr02 at ru dot is  2003-11-04 09:17 -------
> std::ios_base::Init should never be used outside global objects.

What about logging? Consider:

Foo.h:
============================

class Foo
{
public:
  Foo();
  ~Foo():
};

============================
foo.cc:
============================
#include <iostream>

Foo::Foo()
{
  std::clog << "Foo constructed\n";
}

Foo::~Foo()
{
  std::clog << "Foo destroyed\n";
}

============================

If we have another source file:

a.cc:
============================
#include "foo.h"

static Foo foo;
============================

This is unsafe, because foo may be constructed before the ios_base::Init
object in foo.cc, and Foo::Foo() may access clog before it is constructed.

This can be fixed by modifying foo.cc:
============================
#include <iostream>

Foo::Foo()
{
  std::ios_base::Init init;
  std::clog << "Foo constructed\n";
}

Foo::~Foo()
{
  std::ios_base::Init init;
  std::clog << "Foo destroyed\n";
}

============================
The standard guarantees that clog will be constructed before the constructor
of init finishes, so the problem has been fixed. However, unless
ios_base::Init is threadsafe, this means that Foo can no longer be used safely
in threaded programs.

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[rittle at latour dot rsch dot comm dot mot dot com]

------- Additional Comments From rittle at latour dot rsch dot comm dot mot dot com  2003-11-11 08:16 -------
Subject: Re:  Thread safety problems in ios_base::Init

>> std::ios_base::Init should never be used outside global objects.

> What about logging? Consider:

>static Foo foo;
[...]
>Foo::Foo()
>{
>  std::ios_base::Init init;
>  std::clog << "Foo constructed\n";
>}
>
>Foo::~Foo()
>{
>  std::ios_base::Init init;
>  std::clog << "Foo destroyed\n";
>}

On what non-hypothetical system does this code, precisely as shown,
fail to work in threaded cases?

Consider precisely when Foo::Foo or Foo::~Foo may run.  Notice, they
don't run at random, non-predictable points.

Consider precisely which threads of control they may run upon.  For
the case of libstdc++.so and any additional C++ shared libraries
linked against a program image, the only possible thread is the main
thread of control (thus, there is no multi-threading issue).

Consider footnote 265 on page 602 of 14882 which strongly implies
that it is a misuse for users to insert:

  std::ios_base::Init init;

willy-nilly into any other methods and into constructors and
destructors which are not used to manage global objects (Stroustrup
offers stronger words about this).  I hereby grant that footnotes are
not part of the standard.  I also observe that our library has its own
definition of thread safety which again is not part of the standard.

BTW, I might buy the more complex case posted where dlopen was used...
Checking...  Humm, on FreeBSD4 (in dlopen code inside the ELF RT
loader):

    /* Call the init functions with no locks held. */
    wlock_release();
    objlist_call_init(&initlist);
    wlock_acquire();

A patch for this issue would have to come at a point when we can break
the ABI since I don't think it will be safe to mix code.  I'm still a
tad skeptical that this corner is a correct thing to "fix" (i.e. do
people really create C++ systems which allow any random thread to call
dlopen?).  Oh well, opinions based on this analysis?

Regards,
Loren


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[bkoz at gcc dot gnu dot org]

------- Additional Comments From bkoz at gcc dot gnu dot org  2003-12-10 00:17 -------

This would at least remove the corruption of _S_ios_base_init. I figure this is
less useful than the _S_once approach, but necessary no matter what.

Thoughts? I would like to fix this up for 3.4, regardless of how hypothetical
this issue is, or appears to be. I think this issue could, in fact, appear in
practice, either with dlopen or inadvertent (but legal) use of ios_base::Init.

-benjamin

-----

2003-12-09  Benjamin Kosnik  <bkoz@redhat.com>

	PR libstdc++/12855
	* include/bits/ios_base.h (_Callback_list): _M_refcount to
	_M_references.
	(Init::_S_ios_base_init): Change to _S_references, make atomic.
	* src/ios.cc: Adjust definition.
	* src/ios_init.cc (ios_base::Init::Init): Use __exchange_and_add.
	(ios_base::Init::~Init): Same.
	* testsuite/27_io/ios_base/cons/assign_neg.cc: Adjust line numbers.
	* testsuite/27_io/ios_base/cons/copy_neg.cc: Same.
	
Index: include/bits/ios_base.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/include/bits/ios_base.h,v
retrieving revision 1.34
diff -c -p -r1.34 ios_base.h
*** include/bits/ios_base.h	12 Oct 2003 10:12:08 -0000	1.34
--- include/bits/ios_base.h	10 Dec 2003 00:14:44 -0000
*************** namespace std
*** 428,445 ****
        _Callback_list* 		_M_next;
        ios_base::event_callback 	_M_fn;
        int 			_M_index;
!       _Atomic_word		_M_refcount;  // 0 means one reference.
      
        _Callback_list(ios_base::event_callback __fn, int __index, 
  		     _Callback_list* __cb)
!       : _M_next(__cb), _M_fn(__fn), _M_index(__index), _M_refcount(0) { }
        
        void 
!       _M_add_reference() { __atomic_add(&_M_refcount, 1); }
  
        // 0 => OK to delete.
        int 
!       _M_remove_reference() { return __exchange_and_add(&_M_refcount, -1); }
      };
  
       _Callback_list*  	_M_callbacks;
--- 428,445 ----
        _Callback_list* 		_M_next;
        ios_base::event_callback 	_M_fn;
        int 			_M_index;
!       _Atomic_word		_M_references;  // 0 means one reference.
      
        _Callback_list(ios_base::event_callback __fn, int __index, 
  		     _Callback_list* __cb)
!       : _M_next(__cb), _M_fn(__fn), _M_index(__index), _M_references(0) { }
        
        void 
!       _M_add_reference() { __atomic_add(&_M_references, 1); }
  
        // 0 => OK to delete.
        int 
!       _M_remove_reference() { return __exchange_and_add(&_M_references, -1); }
      };
  
       _Callback_list*  	_M_callbacks;
*************** namespace std
*** 493,506 ****
        ~Init();
        
        // NB: Allows debugger applications use of the standard streams
!       // from operator new. _S_ios_base_init must be incremented in
!       // _S_ios_create _after_ initialization is completed.
        static bool
!       _S_initialized() { return _S_ios_base_init; }
  
      private:
!       static int 	_S_ios_base_init;
!       static bool	_S_synced_with_stdio;
      };
  
      // [27.4.2.2] fmtflags state functions
--- 493,505 ----
        ~Init();
        
        // NB: Allows debugger applications use of the standard streams
!       // from operator new. 
        static bool
!       _S_initialized() { return _S_references > 0; }
  
      private:
!       static _Atomic_word	_S_references;
!       static bool		_S_synced_with_stdio;
      };
  
      // [27.4.2.2] fmtflags state functions
Index: src/ios.cc
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/src/ios.cc,v
retrieving revision 1.51
diff -c -p -r1.51 ios.cc
*** src/ios.cc	17 Oct 2003 14:47:30 -0000	1.51
--- src/ios.cc	10 Dec 2003 00:14:44 -0000
*************** namespace std 
*** 107,113 ****
  
    const int ios_base::_S_local_word_size;
  
!   int ios_base::Init::_S_ios_base_init = 0;
  
    bool ios_base::Init::_S_synced_with_stdio = true;
  
--- 107,113 ----
  
    const int ios_base::_S_local_word_size;
  
!   _Atomic_word ios_base::Init::_S_references;
  
    bool ios_base::Init::_S_synced_with_stdio = true;
  
Index: src/ios_init.cc
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/src/ios_init.cc,v
retrieving revision 1.1
diff -c -p -r1.1 ios_init.cc
*** src/ios_init.cc	17 Oct 2003 14:47:30 -0000	1.1
--- src/ios_init.cc	10 Dec 2003 00:14:44 -0000
*************** namespace std 
*** 80,86 ****
  
    ios_base::Init::Init()
    {
!     if (_S_ios_base_init == 0)
        {
  	// Standard streams default to synced with "C" operations.
  	_S_synced_with_stdio = true;
--- 80,86 ----
  
    ios_base::Init::Init()
    {
!     if (__exchange_and_add(&_S_references, 1) == 0)
        {
  	// Standard streams default to synced with "C" operations.
  	_S_synced_with_stdio = true;
*************** namespace std 
*** 110,124 ****
  	wcin.tie(&wcout);
  	wcerr.flags(ios_base::unitbuf);
  #endif
- 
- 	_S_ios_base_init = 1;
        }
-     ++_S_ios_base_init;
    }
  
    ios_base::Init::~Init()
    {
!     if (--_S_ios_base_init == 1)
        {
  	// Catch any exceptions thrown by basic_ostream::flush()
  	try
--- 110,121 ----
  	wcin.tie(&wcout);
  	wcerr.flags(ios_base::unitbuf);
  #endif
        }
    }
  
    ios_base::Init::~Init()
    {
!     if (__exchange_and_add(&_S_references, -1) == 1)
        {
  	// Catch any exceptions thrown by basic_ostream::flush()
  	try
Index: testsuite/27_io/ios_base/cons/assign_neg.cc
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/testsuite/27_io/ios_base/cons/assign_neg.cc,v
retrieving revision 1.6
diff -c -p -r1.6 assign_neg.cc
*** testsuite/27_io/ios_base/cons/assign_neg.cc	12 Oct 2003 10:12:09 -0000	1.6
--- testsuite/27_io/ios_base/cons/assign_neg.cc	10 Dec 2003 00:14:45 -0000
*************** void test01()
*** 41,45 ****
    io1 = io2;
  }
  // { dg-error "within this context" "" { target *-*-* } 41 } 
! // { dg-error "is private" "" { target *-*-* } 746 } 
  // { dg-error "operator=" "" { target *-*-* } 0 } 
--- 41,45 ----
    io1 = io2;
  }
  // { dg-error "within this context" "" { target *-*-* } 41 } 
! // { dg-error "is private" "" { target *-*-* } 745 } 
  // { dg-error "operator=" "" { target *-*-* } 0 } 
Index: testsuite/27_io/ios_base/cons/copy_neg.cc
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/testsuite/27_io/ios_base/cons/copy_neg.cc,v
retrieving revision 1.6
diff -c -p -r1.6 copy_neg.cc
*** testsuite/27_io/ios_base/cons/copy_neg.cc	12 Oct 2003 10:12:09 -0000	1.6
--- testsuite/27_io/ios_base/cons/copy_neg.cc	10 Dec 2003 00:14:45 -0000
*************** void test02()
*** 41,45 ****
    test_base io2 = io1; 
  }
  // { dg-error "within this context" "" { target *-*-* } 41 } 
! // { dg-error "is private" "" { target *-*-* } 743 } 
  // { dg-error "copy constructor" "" { target *-*-* } 0 } 
--- 41,45 ----
    test_base io2 = io1; 
  }
  // { dg-error "within this context" "" { target *-*-* } 41 } 
! // { dg-error "is private" "" { target *-*-* } 742 } 
  // { dg-error "copy constructor" "" { target *-*-* } 0 } 


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[peturr02 at ru dot is]

------- Additional Comments From peturr02 at ru dot is  2003-12-11 08:18 -------
I think the patch is fine in general, but this bit seems wrong:

*************** namespace std 
*** 80,86 ****
  
    ios_base::Init::Init()
    {
!     if (_S_ios_base_init == 0)
        {
  	// Standard streams default to synced with "C" operations.
  	_S_synced_with_stdio = true;
--- 80,86 ----
  
    ios_base::Init::Init()
    {
!     if (__exchange_and_add(&_S_references, 1) == 0)
        {
  	// Standard streams default to synced with "C" operations.
  	_S_synced_with_stdio = true;
*************** namespace std 
*** 110,124 ****
  	wcin.tie(&wcout);
  	wcerr.flags(ios_base::unitbuf);
  #endif
- 
- 	_S_ios_base_init = 1;
        }
-     ++_S_ios_base_init;
    }
  
    ios_base::Init::~Init()
    {
!     if (--_S_ios_base_init == 1)
        {
  	// Catch any exceptions thrown by basic_ostream::flush()
  	try
--- 110,121 ----
  	wcin.tie(&wcout);
  	wcerr.flags(ios_base::unitbuf);
  #endif
        }
    }
  
    ios_base::Init::~Init()
    {
!     if (__exchange_and_add(&_S_references, -1) == 1)
        {
  	
Currently, the constructor sets _S_ios_base_init to 2 on the first run. Each
call after that adds 1, and each call of the destructor subtracts 1. This is
done so that the streams are only constructed once.

It seems to me that with this patch, the first run of the constructor will
set _S_references to 1, but everything else is as before, so that:

#include <ios> // Note: not <iostream>
int main {
  { ios_base::Init i; } // Construct streams
  { ios_base::Init j; } // Constructs streams again
}

This can cause problems for programs that use static constructors to change
settings of the streams, or that perform IO in destructors or atexit()
functions.

There is a testcase that was supposed to check for this condition:
testsuite/27_io/objects/char/5.cc
testsuite/27_io/objects/wchar_t/5.cc
but it probably fails to catch it because <iostream> is included in the
precompiled header, so an Init object gets constructed before static_ob.

(IMHO neither <iostream> nor <cassert> should be included in the precompiled
headers since both can break obscure test cases, such as this one)


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[bkoz at gcc dot gnu dot org]

------- Additional Comments From bkoz at gcc dot gnu dot org  2003-12-13 06:42 -------

Ouch. Of course, I only tested this with <iostream> and a modified version of
the kung-Foo class as it appeared in this bugzilla log.

With just <ios> I'm sure I'll see this. 

So, just adding an __exchange_and_add(1) inside the ctor loop will work, I think
(as that was what was done before.)

Regarding the PCH, well, I don't think it makes sense to ship a PCH that doesn't
have all the standard includes. However, that doesn't mean that build directory
testing has to use the installed PCH files: it might make sense to either

1) test without PCH
2) test with a modified PCH
3) test some test files without PCH. 

Of the three options, the third is the most appealing (since testing all without
PCH just takes too much time). I don't know how to do this however. (special
flags to pass for just these files that negates -include bits/stdc++.h? Hmm. )

-benjamin


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[carlo at gcc dot gnu dot org]

------- Additional Comments From carlo at gcc dot gnu dot org  2003-12-14 03:45 -------
There is another problem with the patch.
As the old comments state:

        // NB: Allows debugger applications use of the standard streams
!       // from operator new. _S_ios_base_init must be incremented in
!       // _S_ios_create _after_ initialization is completed.
        static bool
!       _S_initialized() { return _S_ios_base_init; }

the flag that things are initialized must be set
AFTER things are initialized.  You'd break this function
with your patch:

*************** namespace std 
*** 80,86 ****
  
    ios_base::Init::Init()
    {
!     if (_S_ios_base_init == 0)
        {
  	// Standard streams default to synced with "C" operations.
  	_S_synced_with_stdio = true;
--- 80,86 ----
  
    ios_base::Init::Init()
    {
!     if (__exchange_and_add(&_S_references, 1) == 0)
        {
  	// Standard streams default to synced with "C" operations.
  	_S_synced_with_stdio = true;
*************** namespace std 
*** 110,124 ****
  	wcin.tie(&wcout);
  	wcerr.flags(ios_base::unitbuf);
  #endif
- 
- 	_S_ios_base_init = 1;
        }
-     ++_S_ios_base_init;
    }
  

As this increments _S_references at the beginning
and not at the end.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[bkoz at gcc dot gnu dot org]

------- Additional Comments From bkoz at gcc dot gnu dot org  2003-12-15 18:05 -------

Petur, your bits are taken care of, thanks.

Carlo, I think that the only valid definition of _S_initialized now is if
_S_refcount >= 1. We're not going to be able to narrow it down with further
granularity.

Because of this, I'm not quite sure if the member funtion _S_initialized makes
any sense at the moment. Why not just use an instance of ios_base::Init? If the
streams are initialized, then it does nothing. If they aren't inialized, the
instance of ios_base::Init will initialized them.

Plus, it's portable.

?

-benjamin


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[cvs-commit at gcc dot gnu dot org]

------- Additional Comments From cvs-commit at gcc dot gnu dot org  2003-12-15 19:03 -------
Subject: Bug 12855

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	bkoz@gcc.gnu.org	2003-12-15 19:03:13

Modified files:
	libstdc++-v3   : ChangeLog 
	libstdc++-v3/include/bits: ios_base.h 
	libstdc++-v3/src: ios.cc ios_init.cc 
	libstdc++-v3/testsuite/27_io/ios_base/cons: assign_neg.cc 
	                                            copy_neg.cc 

Log message:
	2003-12-15  Benjamin Kosnik  <bkoz@redhat.com>
	
	PR libstdc++/12855
	* include/bits/ios_base.h (Init::_S_ios_base_init): Change to
	_S_refcount, make atomic.
	* src/ios.cc: Adjust definition.
	* src/ios_init.cc (ios_base::Init::Init): Use __exchange_and_add,
	and __atomic_add.
	(ios_base::Init::~Init): Same.
	* testsuite/27_io/ios_base/cons/assign_neg.cc: Adjust line numbers.
	* testsuite/27_io/ios_base/cons/copy_neg.cc: Same.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/ChangeLog.diff?cvsroot=gcc&r1=1.2156&r2=1.2157
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/include/bits/ios_base.h.diff?cvsroot=gcc&r1=1.34&r2=1.35
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/src/ios.cc.diff?cvsroot=gcc&r1=1.51&r2=1.52
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/src/ios_init.cc.diff?cvsroot=gcc&r1=1.1&r2=1.2
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/testsuite/27_io/ios_base/cons/assign_neg.cc.diff?cvsroot=gcc&r1=1.6&r2=1.7
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/libstdc++-v3/testsuite/27_io/ios_base/cons/copy_neg.cc.diff?cvsroot=gcc&r1=1.6&r2=1.7



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[carlo at alinoe dot com]

------- Additional Comments From carlo at alinoe dot com  2003-12-15 19:49 -------
Subject: Re:  Thread safety problems in ios_base::Init

On Mon, Dec 15, 2003 at 06:05:37PM -0000, bkoz at gcc dot gnu dot org wrote:
> 
> ------- Additional Comments From bkoz at gcc dot gnu dot org  2003-12-15 18:05 -------
> 
> Petur, your bits are taken care of, thanks.
> 
> Carlo, I think that the only valid definition of _S_initialized now is if
> _S_refcount >= 1. We're not going to be able to narrow it down with further
> granularity.
> 
> Because of this, I'm not quite sure if the member funtion _S_initialized makes
> any sense at the moment. Why not just use an instance of ios_base::Init? If the
> streams are initialized, then it does nothing. If they aren't inialized, the
> instance of ios_base::Init will initialized them.
> 
> Plus, it's portable.

No, that is why _S_initialized exists in the first place, because you CAN'T
use ios_base::Init.

I've gone through a lot of trouble to 1) get the behaviour fixed that
the flag was set AFTER the initialization is done (which is demanded by
the standard) and 2) to get _S_initialized added at extention.
And now you break it because you don't understand it - and even commit
the patch while this issue is still open.

I had written testsuite entry too - but that was never added... Now you
see that it was needed! (You just broke that test).



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855

Re: [Bug libstdc++/12855] Thread safety problems in ios_base::Init

[Carlo Wood]

On Mon, Dec 15, 2003 at 06:05:37PM -0000, bkoz at gcc dot gnu dot org wrote:
> 
> ------- Additional Comments From bkoz at gcc dot gnu dot org  2003-12-15 18:05 -------
> 
> Petur, your bits are taken care of, thanks.
> 
> Carlo, I think that the only valid definition of _S_initialized now is if
> _S_refcount >= 1. We're not going to be able to narrow it down with further
> granularity.
> 
> Because of this, I'm not quite sure if the member funtion _S_initialized makes
> any sense at the moment. Why not just use an instance of ios_base::Init? If the
> streams are initialized, then it does nothing. If they aren't inialized, the
> instance of ios_base::Init will initialized them.
> 
> Plus, it's portable.

No, that is why _S_initialized exists in the first place, because you CAN'T
use ios_base::Init.

I've gone through a lot of trouble to 1) get the behaviour fixed that
the flag was set AFTER the initialization is done (which is demanded by
the standard) and 2) to get _S_initialized added at extention.
And now you break it because you don't understand it - and even commit
the patch while this issue is still open.

I had written testsuite entry too - but that was never added... Now you
see that it was needed! (You just broke that test).

-- 
Carlo Wood <carlo@alinoe.com>

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[bkoz at gcc dot gnu dot org]

------- Additional Comments From bkoz at gcc dot gnu dot org  2003-12-15 20:07 -------
> I had written testsuite entry too - but that was never added... Now you
> see that it was needed! (You just broke that test).

Well, add it, or at least propose it as a patch. Sorry I broke this behavior,
but if testcases aren't in CVS, I don't run them (nor do I think this
unreasonable). Furthermore, getting standard-specified behavior correct is of
higher priority than getting extensions working.

That being said, if there's a way to fix the extension, by all means propose it,
and I'll listen to your arguments.

I'm interested in seeing your testcase. The way I see it, ios_base::Init is now
picture-perfect RAII, which was the intent of the design in the first place, I
think. I'm very curious to see your usage.

best,
benjamin

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[bkoz at gcc dot gnu dot org]

------- Additional Comments From bkoz at gcc dot gnu dot org  2003-12-16 17:53 -------
> I have to note thought that ever since std::ios_base::Init::_S_initialized()
> was added (3.3) std::ios_base::Init::Init() didn't call new anymore!
> So, basically there is no need for _S_initialized() as it should always
> return 'true' with the current implementation.

Right. This is what I was trying to express.

> There is the danger (is there?) that operator new will be called from
> elsewhere before the standard streams are initialized - but in that
> case I could indeed create a Init::Init object.

We've moved to zero-alloc initialization: no allocator, no malloc, no new. (Only
placement new). There should be no allocation in initialization anymore
(valgrind tells me this at least). This is by design, and is not likely to
change, ever.

Does this make you feel more at ease?

I'd prefer to not add a new member function or data member to ios_base.

> I'll check our your attachment, thanks.
> The basic problem however was that it is possible for Init() to
> call itself reentrant (by the same thread) and if it does that,
> then the second time it will return WITHOUT that the standard
> streams are initialized; and that is not according to the standard.

I don't see this: the whole point of fixing this bug was to remove this issue.
Init is atomic now.

-benjamin

-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[carlo at alinoe dot com]

------- Additional Comments From carlo at alinoe dot com  2003-12-17 04:38 -------
Subject: Re:  Thread safety problems in ios_base::Init

On Tue, Dec 16, 2003 at 05:53:47PM -0000, bkoz at gcc dot gnu dot org wrote:
> We've moved to zero-alloc initialization: no allocator, no malloc, no new. (Only
> placement new). There should be no allocation in initialization anymore
> (valgrind tells me this at least). This is by design, and is not likely to
> change, ever.
> 
> Does this make you feel more at ease?

Ok, then this extension can be removed - because it is redundant, by design.
If I ever run into problems again because of this, then we can always
add it back }:-)... or rather remove the call to malloc that then was
added by accident.

Thanks



-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855

[Bug libstdc++/12855] Thread safety problems in ios_base::Init

[bkoz at gcc dot gnu dot org]

------- Additional Comments From bkoz at gcc dot gnu dot org  2003-12-23 19:35 -------

Fixed.

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


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12855