private static object construction bug?

private static object construction bug?

[Joe Buehler]

I entered a bug report on this (7458) that was promptly closed by
a Cygwin developer, but I believe incorrectly so, so I am posting here.

I think gcc is generating code that is not thread-safe for
the attached C++ code.  This is regardless of whether gcc is
compiled for thread support, and apparently regardless of platform,
or even gcc version.

The static variable inside the function is constructed on function
entry, but the construction is not thread-safe -- two threads calling
this function at about the same time can cause double construction of
the object etc.

This was found originally in gcc 2.95 under Cygwin (not compiled with
thread support), but I believe is not specific to cygwin or whether
thread support is on, or the particular version of gcc.

- I built a version of gcc 3.1.1 under AIX 4.3.3 with
   --enable-threads=posix, and it appears to have the bug.

- I checked the compiler on a RedHat 7.1 i386 box running 2.96,
   which looks to have been compiled with --enable-threads=posix,
   and it definitely has the bug.

I would appreciate it if someone familiar with the internals of
gcc could verify whether this is a real problem.

Joe Buehler

struct the_struct {
	int i;
	the_struct() {
		i = 0;
	};
	~the_struct() {
		i = 1;
	};
};

void
function()
{
	static the_struct temporary;
}

Re: private static object construction bug?

[Joe Buck]

> I entered a bug report on this (7458) that was promptly closed by
> a Cygwin developer, but I believe incorrectly so, so I am posting here.

I've squashed the example a bit, but here it is:

struct the_struct {
	int i;
	the_struct() { i = 0;}
	~the_struct() {	i = 1;}
};

void function() {
	static the_struct temporary;
}

The decision to close the bug report was correct.

> I think gcc is generating code that is not thread-safe for
> the attached C++ code.

Sorry, but the version of "thread-safe" that you are asking for would
require substantially slower code: you appear to want thread-safe locking
for the creation of all objects (in your case, for the construction of
a static variable that is local to a function).  GCC is never going to do
that.  The speed penalty would be enormous.

When we talk about thread-safety, what is meant is that any allocation
mechanism that exists "behind the scenes", like reference-counted string
objects or allocation pools, can't introduce any thread conflict issues.
However, no locking code is introduced for objects that can be potentially
accessed in different threads, such locking is up to the application
programmers.

To understand the kind of thread safety the compiler and the library
provide, see

http://www.sgi.com/tech/stl/thread_safety.html

(this is an SGI document, but the philosophy used by GCC is essentially
the same).

Here is the example:

Re: private static object construction bug?

[Joe Buehler]

Joe Buck wrote:

> Sorry, but the version of "thread-safe" that you are asking for would
> require substantially slower code: you appear to want thread-safe locking
> for the creation of all objects (in your case, for the construction of
> a static variable that is local to a function).  GCC is never going to do
> that.  The speed penalty would be enormous.

OK, thanks, that's what I wanted to know.  I had assumed that all globals
were constructed before main() enters, but apparently C++ is not defined
that way for all types of globals.

Joe Buehler

Re: private static object construction bug?

[Joe Buck]

> Joe Buck wrote:
> 
> > Sorry, but the version of "thread-safe" that you are asking for would
> > require substantially slower code: you appear to want thread-safe locking
> > for the creation of all objects (in your case, for the construction of
> > a static variable that is local to a function).  GCC is never going to do
> > that.  The speed penalty would be enormous.
> 
> OK, thanks, that's what I wanted to know.  I had assumed that all globals
> were constructed before main() enters, but apparently C++ is not defined
> that way for all types of globals.

But you didn't have a global, you had a static variable that is local to a
function.  The rule for such objects is different.

Re: private static object construction bug?

[Mark Mitchell]

--On Monday, August 05, 2002 09:48:49 AM -0700 Joe Buck 
<Joe.Buck@synopsys.com> wrote:

>
>> I entered a bug report on this (7458) that was promptly closed by
>> a Cygwin developer, but I believe incorrectly so, so I am posting here.
>
> I've squashed the example a bit, but here it is:
>
> struct the_struct {
> 	int i;
> 	the_struct() { i = 0;}
> 	~the_struct() {	i = 1;}
> };
>
> void function() {
> 	static the_struct temporary;
> }
>
> The decision to close the bug report was correct.

Actually, I think it was only sort-of correct.

The ABI specifies functions a compiler can use to get thread safe
construction of local temporaries.  The compiler can choose not to
use them.  G++ makes that choice, but partly due to not getting
around to calling the functions.  The intended code sequence is
not really any slower after the object has been constructed.

Right now, we do this approximately (there are exceptions to worry
about!):

  if (!guard) {
    guard = 1;
    // Construct variable.
  }

The new sequence would be:

  if (!guard) {
    __cxa_guard_acquire (&guard); // I forget the exact name.
    // Construct variable.
    __cxa_guard_release (&guard):
  }

So, you pay two extra function calls the first time into the function,
and nothing thereafter, modulo space.

So, the compiler is ABI compliant, but as QOI issue we should probably
use the functions.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: private static object construction bug?

[Zack Weinberg]

On Mon, Aug 05, 2002 at 01:58:18PM -0700, Mark Mitchell wrote:

>  if (!guard) {
>    __cxa_guard_acquire (&guard); // I forget the exact name.
>    // Construct variable.
>    __cxa_guard_release (&guard):
>  }

This sequence has two different races.

1: Thread 1 tests guard, finds it zero, and calls __cxa_guard_acquire.
Before __cxa_guard_acquire can modify guard, Thread 2 tests guard,
finds it still zero, and enters the block.  The object will be
constructed twice.

2: Thread 1 tests guard, finds it zero, calls __cxa_guard_acquire
which sets guard to a nonzero value.  Before the object can be
constructed, Thread 2 tests guard, finds it nonzero, and proceeds to
use the uninitialized object.

To cure these races, we need to generate code like this instead:

  if (guard) {
    if (__cxa_guard_acquire (&guard)) {
      // construct variable.
      __cxa_guard_release (&guard)
    }
  }

I inverted the outer test because 'guard' is now a tristate variable;
for speed, we still want the outer test to be against zero, so zero
needs to be the 'initialized' state (which unfortunately means the
guard can't go in BSS).  The races now look like this:

Thread 1 tests guard, finds it nonzero, and calls __cxa_guard_acquire.
Thread 2 tests guard, finds it nonzero, and calls __cxa_guard_acquire.
__cxa_guard_acquire changes guard from (say) -1 to 1 with an atomic
operation.  One of the two threads will win the race to do this.  That
thread returns a nonzero value from __cxa_guard_acquire, and proceeds
to construct the object.  The other thread blocks until
__cxa_guard_release is called, at which point it returns zero from
__cxa_guard_acquire and skips the constructor.  __cxa_guard_release
also changes guard from (say) 1 to 0 with an atomic operation; future
threads will skip the entire block.

It is likely that the ABI standard does specify something like this,
but I wanted to make sure people understood that thread-safe object
construction is not as simple as you made it look.  (The guard
variable can't be a simple integer, either; it needs to be a structure
containing an integer and a mutex.)

zw

Re: private static object construction bug?

[Fergus Henderson]

On 05-Aug-2002, Mark Mitchell <mark@codesourcery.com> wrote:
> 
> 
> --On Monday, August 05, 2002 09:48:49 AM -0700 Joe Buck 
> <Joe.Buck@synopsys.com> wrote:
> 
> >
> >> I entered a bug report on this (7458) that was promptly closed by
> >> a Cygwin developer, but I believe incorrectly so, so I am posting here.
> >
> > I've squashed the example a bit, but here it is:
> >
> > struct the_struct {
> > 	int i;
> > 	the_struct() { i = 0;}
> > 	~the_struct() {	i = 1;}
> > };
> >
> > void function() {
> > 	static the_struct temporary;
> > }
> >
> > The decision to close the bug report was correct.
> 
> Actually, I think it was only sort-of correct.
> 
> The ABI specifies functions a compiler can use to get thread safe
> construction of local temporaries.

Did you mean to say "local statics"?

I don't see what would be non-thread-safe about constructing
local temporaries, since temporaries have automatic storage
duration and get constructed on the thread's stack.

> The compiler can choose not to
> use them.  G++ makes that choice, but partly due to not getting
> around to calling the functions.  The intended code sequence is
> not really any slower after the object has been constructed.
> 
> Right now, we do this approximately (there are exceptions to worry
> about!):
> 
>   if (!guard) {
>     guard = 1;
>     // Construct variable.
>   }
> 
> The new sequence would be:
> 
>   if (!guard) {
>     __cxa_guard_acquire (&guard); // I forget the exact name.
>     // Construct variable.
>     __cxa_guard_release (&guard):
>   }

Hmm. I don't understand how this would solve the problem.
It would prevent simultaneous construction, but the
variable could still get constructed twice, couldn't it?
That could still lead to problems.

A slightly different code sequence could solve the double-construction
problem.

-- 
Fergus Henderson <fjh@cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.

Re: private static object construction bug?

[Mark Mitchell]

--On Monday, August 05, 2002 03:50:12 PM -0700 Zack Weinberg 
<zack@codesourcery.com> wrote:

> On Mon, Aug 05, 2002 at 01:58:18PM -0700, Mark Mitchell wrote:
>
>>  if (!guard) {
>>    __cxa_guard_acquire (&guard); // I forget the exact name.
>>    // Construct variable.
>>    __cxa_guard_release (&guard):
>>  }
>
> This sequence has two different races.
>

Yes; I was overly simplistic.  You are of course correct.

> It is likely that the ABI standard does specify something like this,
> but I wanted to make sure people understood that thread-safe object
> construction is not as simple as you made it look.  (The guard
> variable can't be a simple integer, either; it needs to be a structure
> containing an integer and a mutex.)

Yes.  The ABI says that guard is 64 bits long, and that one of the
bytes is a boolean.  (Our current implementation just twiddles that
byte.)  The idea is that __cxa_guard_acquire twiddles the byte in
a thread-safe way.

Thanks for clarifying.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: private static object construction bug?

[Mark Mitchell]

--On Tuesday, August 06, 2002 08:51:28 AM +1000 Fergus Henderson 
<fjh@cs.mu.OZ.AU> wrote:

> On 05-Aug-2002, Mark Mitchell <mark@codesourcery.com> wrote:
>>
>>
>> --On Monday, August 05, 2002 09:48:49 AM -0700 Joe Buck
>> <Joe.Buck@synopsys.com> wrote:
>>
>> >
>> >> I entered a bug report on this (7458) that was promptly closed by
>> >> a Cygwin developer, but I believe incorrectly so, so I am posting
>> >> here.
>> >
>> > I've squashed the example a bit, but here it is:
>> >
>> > struct the_struct {
>> > 	int i;
>> > 	the_struct() { i = 0;}
>> > 	~the_struct() {	i = 1;}
>> > };
>> >
>> > void function() {
>> > 	static the_struct temporary;
>> > }
>> >
>> > The decision to close the bug report was correct.
>>
>> Actually, I think it was only sort-of correct.
>>
>> The ABI specifies functions a compiler can use to get thread safe
>> construction of local temporaries.
>
> Did you mean to say "local statics"?

Yes.  My mailer crashed as I hit send -- I could see that typo but
I couldn't fix it. :-)

> A slightly different code sequence could solve the double-construction
> problem.

You guys are too smart.  I try to go up to ten thousand feet to get
the idea across and y'all want to pick apart the details. :-)

Ignore my pseudo-code; the point is that (in contrast to what Joe said),
the ABI does consider it a good idea for compilers to provide for
thread-safe constuction of local statics and gives routines to do that;
however, GCC doesn't currently use those routines.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com