[Bug libstdc++/36801] New: config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[Bug libstdc++/36801] New: config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[jifl-bugzilla at jifvik dot org]

In 4.3.1 (and 4.2.x and the trunk), ARM targets and no doubt plenty others use
libstdc++/config/cpu/generic/atomicity_mutex/atomicity.h.

With some build magic in libstdc++/src/Makefile.am, this file is also
atomicity.cc when building the library. Thus this object gets instantiated:
{
  __gnu_cxx::__mutex atomic_mutex;
} // anonymous namespace

The constructor for __mutex in include/ext/concurrence.h has a private member
of the underlying __gthread_mutex_t, and its constructor uses
GTHREAD_MUTEX_INIT or calls GTHREAD_MUTEX_INIT_FUNCTION as appropriate, as of
course it should.

The problem is that this becomes a global constructor and there is no control
over where in the running of global constructors it gets run. A consequence of
this is that any other initialisation function from another constructor using
atomic operations can end up locking a mutex which has never been initialised,
with unsurprising results. An example of such a case is ios_base::Init::Init()
which calls __gnu_cxx::__exchange_and_add_dispatch() which eventually reaches
__gnu_cxx::__scoped_lock sentry(atomic_mutex); which attempts to lock
atomic_mutex.

Code prior to this implementation used __gthread_mutex_t in atomicity.h
directly  using a macro defined by concurrence.h:
  __glibcxx_mutex_define_initialized(atomic_mutex);
and as such this used to work, so this is a regression.

It seems this is an unintended consequence of this change:
http://gcc.gnu.org/ml/libstdc++/2006-09/msg00084.html

I think this needs returning to something similar to the way it was before, so
that the __gthread_mutex_t was created in the atomic_mutex's own constructor.


-- 
           Summary: config/cpu/generic/atomicity_mutex/atomicity.h
                    incorrectly relies on global constructor ordering
           Product: gcc
           Version: 4.3.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: jifl-bugzilla at jifvik dot org
 GCC build triplet: i686-pc-linux-gnu
  GCC host triplet: i686-pc-linux-gnu
GCC target triplet: arm-eabi


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[jifl-bugzilla at jifvik dot org]

------- Comment #1 from jifl-bugzilla at jifvik dot org  2008-07-11 13:03 -------
On thinking about this a bit more, I think this would be a regression for the
case where __GTHREAD_MUTEX_INIT is used. In the case of
__GTHREAD_MUTEX_INIT_FUNCTION, I believe the previous implementation was also
susceptible to this issue, although in practice, I don't recall ever seeing it
- maybe just luck of link order.

So the __GTHREAD_MUTEX_INIT case can probably be addressed by bringing the
implementation back to the way it was before. I haven't so far thought of a way
to address the __GTHREAD_MUTEX_INIT_FUNCTION case other than the use of
__gthread_once().

I would be tempted to provide a patch, but I don't have a copyright assignment
for GCC (and of course it takes weeks to sort that out).


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[paolo dot carlini at oracle dot com]

------- Comment #2 from paolo dot carlini at oracle dot com  2008-07-11 13:36 -------
CCing Benjamin...

By the way, if the patch would be very short, the full Assignment would not be
needed...


-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bkoz at redhat dot com


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[bkoz at gcc dot gnu dot org]

------- Comment #3 from bkoz at gcc dot gnu dot org  2008-07-11 20:53 -------

Hey Jonathan. 

It would be most helpful if you could come up with a test case. I know, it will
be a pain and difficult, etc etc etc yadda yadda yadda, but really this would
be enormously helpful.

Instead of putting this back to the previous approach, I'd be more inclined to
use some kind of gthread_once approach. That's the usual approach that seems to
work well when used in locales where order-of-initialization has to be defined
with certainty, and in io, etc.

As this is present in the 4.2.x toolchain, we should probably adjust the bug
report to say this. I suspect this will only be fixed in the 4.3.x toolchain
though.


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[jifl-bugzilla at jifvik dot org]

------- Comment #4 from jifl-bugzilla at jifvik dot org  2008-07-12 01:53 -------
I can't really work out how to provide a testcase as such. To reproduce it all
I'm doing is:
#include <cstdio>
#include <iostream>
int
main(int argc, char *argv[])
{
    std::cout << "Hello world" << std::endl;
    return 0;
}

The actual error comes as a result of libstdc++'s interaction with the
surrounding environment when GCC is built with --enable-threads with my OS, and
what that means for the final linked executable running on an ARM board. So
short of giving you my OS and an ARM board I can't think of any way to
construct a piece of code that you can use in your environment to reproduce it.

I guess you're reluctant to try fixing a problem you can't see yourself, so in
that case I'll produce and test a patch, now that I know you're okay with the
__gthread_once approach. And I'll try to keep it as short as possible to avoid
needing to sort out a copyright assignment. In fact, I'll apply for a copyright
assignment now just in case anyway.

Hmm, seems I can't assign this bug to myself :-).


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[jifl-bugzilla at jifvik dot org]

------- Comment #5 from jifl-bugzilla at jifvik dot org  2008-07-15 01:19 -------
Created an attachment (id=15909)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=15909&action=view)
Patch against 4.3.1 using a once variable to ensure safe initialisation

Here's a patch, let me know what you think. As a bonus, I've added a fix for a
thinko in concurrence.h with initialising a condition variable.

2008-07-14  Jonathan Larmour  <jifl@eCosCentric.com>

        PR libstdc++/36801
        * include/ext/concurrence.h: Separate out __mutex construction into
        a separate public method.
        Fix __gthread_cond_t initialisation function macro name.
        * config/cpu/generic/atomicity_mutex/atomicity.h: In a threaded
        environment, use a once variable to guarantee atomic_mutex is
        initialised before use.


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[paolo dot carlini at oracle dot com]

------- Comment #6 from paolo dot carlini at oracle dot com  2008-10-03 22:42 -------
Benjamin, any feedback on this? Thanks!


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[jifl-bugzilla at jifvik dot org]

------- Comment #7 from jifl-bugzilla at jifvik dot org  2008-10-04 02:54 -------
To avoid any uncertainty, I arranged a copyright assignment. Unfortunately the
FSF's copyright clerk left and there was a gap before the replacement started,
but it just so happens that today he confirmed the assignment has been
completed. I note that /home/g/gnuorg/copyright.list on fencepost.gnu.org has
not yet been updated, but I can mail the PDF (privately) if you wish, or you
can contact copyright-clerk@fsf for confirmation.

The upshot is that the copyright of this patch is not something to worry about
anyway.


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[jifl-bugzilla at jifvik dot org]

------- Comment #8 from jifl-bugzilla at jifvik dot org  2008-10-30 13:10 -------
Hi Benjamin, the copyright assignment is definitely sorted, and can be seen on
copyright.list on fencepost.gnu.org now.

Any reason for this not to go in (although I don't have commit perms)? Would it
better for me to submit it to gcc-patches?

Thanks.

Jifl


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[paolo dot carlini at oracle dot com]

------- Comment #9 from paolo dot carlini at oracle dot com  2008-12-17 17:21 -------
Benjamin, any feedback on this? Thanks!


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[bkoz at gcc dot gnu dot org]

------- Comment #10 from bkoz at gcc dot gnu dot org  2009-01-07 03:40 -------
Mine.


-- 

bkoz at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |bkoz at gcc dot gnu dot org
                   |dot org                     |
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2009-01-07 03:40:05
               date|                            |


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[bkoz at gcc dot gnu dot org]

------- Comment #11 from bkoz at gcc dot gnu dot org  2009-01-07 03:56 -------
Created an attachment (id=17042)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=17042&action=view)
patch version 2


Hey! Sorry about the delay on this. Please try the attached patch, and let me
know if it works for you.

This appears to be another manifestation of 29118. 

I've audited the mutex usage in libstc++ sources and it looks like similar
patches may be necessary for:

mt_allocator.cc: freelist_mutex
pool_allocator.cc: palloc_init_mutex
mutex.cc: __once_mutex
locale.cc: locale_cache_mutex
debug.cc: safe_base_mutex
atomic.cc: atomic_mutex

That will be a follow-up patch.


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[bkoz at gcc dot gnu dot org]

------- Comment #12 from bkoz at gcc dot gnu dot org  2009-01-07 09:18 -------
Created an attachment (id=17043)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=17043&action=view)
plus mt_allocator fixes


This fixes these longstanding fails on darwin:

FAIL: ext/mt_allocator/deallocate_local_thread-5.cc execution test
FAIL: ext/mt_allocator/deallocate_local_thread-7.cc execution test


-- 

bkoz at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #17042|0                           |1
        is obsolete|                            |


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[jifl-bugzilla at jifvik dot org]

------- Comment #13 from jifl-bugzilla at jifvik dot org  2009-01-07 18:03 -------
The patch seems to be ok from my cursory checking, thanks!

Note that my original patch also included a trivial fix for concurrence.h where
__GTHREAD_MUTEX_INIT_FUNCTION was called when it should have been
__GTHREAD_COND_INIT_FUNCTION.


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[paolo dot carlini at oracle dot com]

------- Comment #14 from paolo dot carlini at oracle dot com  2009-01-07 18:17 -------
(In reply to comment #13)
> Note that my original patch also included a trivial fix for concurrence.h where
> __GTHREAD_MUTEX_INIT_FUNCTION was called when it should have been
> __GTHREAD_COND_INIT_FUNCTION.

This is already in, thanks. 


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[bkoz at gcc dot gnu dot org]

------- Comment #15 from bkoz at gcc dot gnu dot org  2009-01-08 03:15 -------
Subject: Bug 36801

Author: bkoz
Date: Thu Jan  8 03:14:24 2009
New Revision: 143182

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=143182
Log:
2009-01-07  Benjamin Kosnik  <bkoz@redhat.com>
            Jonathan Larmour  <jifl@eCosCentric.com>

        PR libstdc++/36801
        * config/cpu/generic/atomicity_mutex/atomicity.h (get_atomic_mutex):
        New.
        (__gnu_cxx::__exchange_and_add): Use it.
        * src/atomic.cc (get_atomic_mutex): New.
        * src/debug.cc (get_safe_base_mutex): New.
        * src/locale.cc (get_locale_cache_mutex): New.
        * src/mt_allocator.cc (get_freelist): New.
        (get_freelist_mutex): New.
        * src/pool_allocator.cc (get_palloc_mutex): New.
        * include/std/mutex (__once_functor_lock): To
        (__get_once_functor_lock): ...this.
        * src/mutex.cc (__once_mutex): Don't export, use
        (get_once_mutex): ...this.
        * config/abi/pre/gnu.ver: Adjust exports.


Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/config/abi/pre/gnu.ver
    trunk/libstdc++-v3/config/cpu/generic/atomicity_mutex/atomicity.h
    trunk/libstdc++-v3/include/std/mutex
    trunk/libstdc++-v3/src/atomic.cc
    trunk/libstdc++-v3/src/debug.cc
    trunk/libstdc++-v3/src/locale.cc
    trunk/libstdc++-v3/src/mt_allocator.cc
    trunk/libstdc++-v3/src/mutex.cc
    trunk/libstdc++-v3/src/pool_allocator.cc


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[bkoz at gcc dot gnu dot org]

------- Comment #16 from bkoz at gcc dot gnu dot org  2009-01-12 20:57 -------
Created an attachment (id=17080)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=17080&action=view)
for gcc-4_3-branch


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[bkoz at gcc dot gnu dot org]

------- Comment #17 from bkoz at gcc dot gnu dot org  2009-01-12 21:32 -------
Subject: Bug 36801

Author: bkoz
Date: Mon Jan 12 21:32:19 2009
New Revision: 143310

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=143310
Log:
2009-01-12  Benjamin Kosnik  <bkoz@redhat.com>
            Jonathan Larmour  <jifl@eCosCentric.com>

        PR libstdc++/36801
        * config/cpu/generic/atomicity_mutex/atomicity.h (get_atomic_mutex):
        New.
        (__gnu_cxx::__exchange_and_add): Use it.
        * src/debug.cc (get_safe_base_mutex): New.
        * src/locale.cc (get_locale_cache_mutex): New.
        * src/mt_allocator.cc (get_freelist): New.
        (get_freelist_mutex): New.
        * src/pool_allocator.cc (get_palloc_mutex): New.

2009-01-12  Jonathan Larmour  <jifl@eCosCentric.com>

        * include/ext/concurrence.h: Fix __gthread_cond_t initialisation
        function macro name.


Modified:
    branches/gcc-4_3-branch/libstdc++-v3/ChangeLog
   
branches/gcc-4_3-branch/libstdc++-v3/config/cpu/generic/atomicity_mutex/atomicity.h
    branches/gcc-4_3-branch/libstdc++-v3/include/ext/concurrence.h
    branches/gcc-4_3-branch/libstdc++-v3/src/debug.cc
    branches/gcc-4_3-branch/libstdc++-v3/src/locale.cc
    branches/gcc-4_3-branch/libstdc++-v3/src/mt_allocator.cc
    branches/gcc-4_3-branch/libstdc++-v3/src/pool_allocator.cc


-- 


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

[Bug libstdc++/36801] config/cpu/generic/atomicity_mutex/atomicity.h incorrectly relies on global constructor ordering

[bkoz at gcc dot gnu dot org]

------- Comment #18 from bkoz at gcc dot gnu dot org  2009-01-12 21:36 -------

fixed for 4.4.x/4.3.x


-- 

bkoz at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|---                         |4.3.3


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