[Bug libstdc++/54172] New: [4.7 Regression] __cxa_guard_acquire thread-safety issue

[Bug libstdc++/54172] New: [4.7 Regression] __cxa_guard_acquire thread-safety issue

[thiago at kde dot org]

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

             Bug #: 54172
           Summary: [4.7 Regression] __cxa_guard_acquire thread-safety
                    issue
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: major
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: thiago@kde.org


Created attachment 27936
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27936
Proposed fix.

In commit 184110, the __cxa_guard_acquire implementation in libsupc++/guard.cc
has been updated to use the new __atomic_* intrinsincs instead of the old
__sync_* ones. I believe this has introduced a regression due to a race
condition.

== Proof ==
While debugging a program, I set a hardware watchpoint on a guard variable and
set gdb to continue execution upon stop. The output was:

Hardware watchpoint 1:
_ZGVZN12_GLOBAL__N_121Q_QGS_textCodecsMutex13innerFunctionEvE6holder

Old value = 0
New value = 256
0x000000381205f101 in __cxxabiv1::__cxa_guard_acquire (g=0x7ffff7dc9a60)
    at ../../../../libstdc++-v3/libsupc++/guard.cc:254
254                 if (__atomic_compare_exchange_n(gi, &expected, pending_bit,
false,
Hardware watchpoint 1:
_ZGVZN12_GLOBAL__N_121Q_QGS_textCodecsMutex13innerFunctionEvE6holder

Old value = 256
New value = 1
__cxxabiv1::__cxa_guard_release (g=0x7ffff7dc9a60) at
../../../../libstdc++-v3/libsupc++/guard.cc:376
376             if ((old & waiting_bit) != 0)
[Switching to Thread 0x7fffebfff700 (LWP 113412)]
Hardware watchpoint 1:
_ZGVZN12_GLOBAL__N_121Q_QGS_textCodecsMutex13innerFunctionEvE6holder

Old value = 1
New value = 256
0x000000381205f101 in __cxxabiv1::__cxa_guard_acquire (g=0x7ffff7dc9a60)
    at ../../../../libstdc++-v3/libsupc++/guard.cc:254
254                 if (__atomic_compare_exchange_n(gi, &expected, pending_bit,
false,
[New Thread 0x7ffff0a2d700 (LWP 113413)]
Hardware watchpoint 1:
_ZGVZN12_GLOBAL__N_121Q_QGS_textCodecsMutex13innerFunctionEvE6holder

Old value = 256
New value = 1
__cxxabiv1::__cxa_guard_release (g=0x7ffff7dc9a60) at
../../../../libstdc++-v3/libsupc++/guard.cc:376
376             if ((old & waiting_bit) != 0)

As can be seen by the output, the guard variable transitioned from 0 -> 256 ->
1 -> 256 -> 1.

== Analysis ==

The code in guard.cc is:

        int expected(0);
        const int guard_bit = _GLIBCXX_GUARD_BIT;
        const int pending_bit = _GLIBCXX_GUARD_PENDING_BIT;
        const int waiting_bit = _GLIBCXX_GUARD_WAITING_BIT;

        while (1)
          {
            if (__atomic_compare_exchange_n(gi, &expected, pending_bit, false,
                                            __ATOMIC_ACQ_REL,
                                            __ATOMIC_RELAXED))
              {
                // This thread should do the initialization.
                return 1;
              }

            if (expected == guard_bit)
              {
                // Already initialized.
                return 0;       
              }
             if (expected == pending_bit)
               {
                 int newv = expected | waiting_bit;
                 if (!__atomic_compare_exchange_n(gi, &expected, newv, false,
                                                  __ATOMIC_ACQ_REL, 
                                                  __ATOMIC_RELAXED))
                   continue;

                 expected = newv;
               }

            syscall (SYS_futex, gi, _GLIBCXX_FUTEX_WAIT, expected, 0);
          }

We have two threads running and they both reach __cxa_guard_acquire more or
less at the same time. On one thread, the execution is the expected path: the
first CAS succeeds and that transitions the guard variable from 0 to 256. That
thread will initialise the static.

In the second thread, the CAS fails, so it will proceed to the second CAS,
trying to replace 256 with 768 (to indicate it's going to sleep).

In the mean time, the first thread calls __cxa_guard_release, which exchanges
the 256 with a 1.

Therefore, on the second thread, the second CAS fails and now expected == 1 (it
got updated). The continue makes it return to the first CAS with expected == 1
and that one succeeds, by replacing it from 1 to 256, which is wrong.

== Solution ==

This issue appears to be caused by the new atomic intrinsics updating the
expected variable and the looping. If the second CAS fails, the code needs to
inspect the value set there to determine what to do next. The possible values
are:

 0: the other thread aborted, we should try again -> continue
 1: initialisation completed, we should return 0
 (256: can't happen)
 768: yet another thread succeeded in setting the waiting bit, we should sleep

The attached patch is a proposed solution to the problem, but I have not been
able to test it yet.

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.7.2
            Summary|[4.7 Regression]            |[4.7/4.8 Regression]
                   |__cxa_guard_acquire         |__cxa_guard_acquire
                   |thread-safety issue         |thread-safety issue

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-08-05
     Ever Confirmed|0                           |1

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-05 19:36:24 UTC ---
The analysis looks correct, and I think the patch is actually an improvement on
the old behaviour, correctly setting 'expected' to 0 for the first CAS but also
avoiding unnecessarily looping when the second CAS fails: we don't need to loop
and do the first CAS again to decide what to do, examining the result of that
second CAS tells you what needs to be done.

I don't particularly like the variable name 'expected' because it serves two
purposes: it holds the expected value and after the CAS it holds the actual
value, the name 'expected' only suits the first usage and makes comparisons
such as (expected == guard_bit) look odd. It's not the expected value that's
being compared, it's the actual value which was *not* what was expected.  I
preferred the original name "old", or maybe better yet "current".

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-08-10 15:07:33 UTC ---
The patch looks good to me, though IMHO only the first hunk is strictly
necessary (and pretty much obvious and matches what the code was doing before
r184110).
The second hunk is just an optimisation, so supposedly it should go only to
4.8?

Can you please post it to gcc-patches and libstdc++ mailing lists where review
should happen?  Thanks.

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[foom at fuhm dot net]

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

James Y Knight <foom at fuhm dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |foom at fuhm dot net

--- Comment #3 from James Y Knight <foom at fuhm dot net> 2012-08-30 02:39:31 UTC ---
Ping; this seems like a serious regression in 4.7 with a patch.

I guess fixing it is blocked on Thiago posting the patch to gcc-patches?

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[thiago at kde dot org]

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

--- Comment #4 from Thiago Macieira <thiago at kde dot org> 2012-08-30 07:52:31 UTC ---
I'll post today.

I haven't yet looked up which mailing list you're even talking about.

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-08-30 08:19:56 UTC ---
Send to both gcc-patches at gcc.gnu.org and libstdc++ at gcc.gnu.org.
I think it would be better to send both hunks separately, as the first hunk is
shorter for backporting and the only necessary thing to fix, and you should
include ChangeLog entries alongside with the patches, something like:
2012-08-30  Thiago Macieira  <whatever@whatever.whatever>

        PR libstdc++/54172
        * libsupc++/guard.cc (__cxa_guard_acquire): Describe your change here.

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[redi at gcc dot gnu.org]

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-08-30 09:56:44 UTC ---
This is just the usual submission process, documented at
http://gcc.gnu.org/contribute.html#patches

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[thiago at kde dot org]

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

--- Comment #7 from Thiago Macieira <thiago at kde dot org> 2012-09-01 08:26:05 UTC ---
I posted the patches on Thursday, three patches because I found one more issue,
to both lists.

Will they be picked up from there and applied to the source tree?

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[rth at gcc dot gnu.org]

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

Richard Henderson <rth at gcc dot gnu.org> changed:

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

--- Comment #8 from Richard Henderson <rth at gcc dot gnu.org> 2012-09-04 17:19:41 UTC ---
(In reply to comment #7)
> I posted the patches on Thursday, three patches because I found one more issue,
> to both lists.

I havn't seen anything from you arrive on gcc-patches.
But I will say that the patch attached here looks good.

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[thiago at kde dot org]

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

--- Comment #9 from Thiago Macieira <thiago at kde dot org> 2012-09-04 17:47:08 UTC ---
(In reply to comment #8)
> (In reply to comment #7)
> > I posted the patches on Thursday, three patches because I found one more issue,
> > to both lists.
> 
> I havn't seen anything from you arrive on gcc-patches.
> But I will say that the patch attached here looks good.

http://gcc.gnu.org/ml/gcc-patches/2012-08/msg02026.html
http://gcc.gnu.org/ml/gcc-patches/2012-08/msg02027.html
http://gcc.gnu.org/ml/gcc-patches/2012-08/msg02028.html

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[paolo.carlini at oracle dot com]

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

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |paolo.carlini at oracle dot
                   |                            |com

--- Comment #10 from Paolo Carlini <paolo.carlini at oracle dot com> 2012-09-05 16:22:14 UTC ---
Richard, could you please review the 3 patches as sent to the mailing list?

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[rth at gcc dot gnu.org]

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

--- Comment #11 from Richard Henderson <rth at gcc dot gnu.org> 2012-09-06 18:53:25 UTC ---
The combined patch set also looks ok.

Talking with bkoz, he would like to see more comments added,
since a reasonable testcase seems unlikely.

[Bug libstdc++/54172] [4.7/4.8 Regression] __cxa_guard_acquire thread-safety issue

[bkoz at gcc dot gnu.org]

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

--- Comment #12 from Benjamin Kosnik <bkoz at gcc dot gnu.org> 2012-09-06 20:31:13 UTC ---
Author: bkoz
Date: Thu Sep  6 20:31:08 2012
New Revision: 191042

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191042
Log:
2012-09-06  Thiago Macieira  <thiago.macieira@intel.com>

    PR libstdc++/54172
        * libsupc++/guard.cc (__cxa_guard_acquire): Exit the loop earlier if
        we detect that another thread has had success. Don't compare_exchange
        from a finished state back to a waiting state. Comment.


Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/libsupc++/guard.cc

[Bug libstdc++/54172] [4.7 Regression] __cxa_guard_acquire thread-safety issue

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
           Priority|P3                          |P2
            Summary|[4.7/4.8 Regression]        |[4.7 Regression]
                   |__cxa_guard_acquire         |__cxa_guard_acquire
                   |thread-safety issue         |thread-safety issue

--- Comment #13 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-09-07 10:23:00 UTC ---
Fixed on trunk sofar.

[Bug libstdc++/54172] [4.7 Regression] __cxa_guard_acquire thread-safety issue

[bkoz at gcc dot gnu.org]

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

--- Comment #14 from Benjamin Kosnik <bkoz at gcc dot gnu.org> 2012-09-10 05:08:15 UTC ---
Author: bkoz
Date: Mon Sep 10 05:08:07 2012
New Revision: 191125

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191125
Log:
2012-09-09  Thiago Macieira  <thiago.macieira@intel.com>

    PR libstdc++/54172
        * libsupc++/guard.cc (__cxa_guard_acquire): Exit the loop earlier if
        we detect that another thread has had success. Don't compare_exchange
        from a finished state back to a waiting state. Comment.


Modified:
    branches/gcc-4_7-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_7-branch/libstdc++-v3/libsupc++/guard.cc

[Bug libstdc++/54172] [4.7 Regression] __cxa_guard_acquire thread-safety issue

[bkoz at gcc dot gnu.org]

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

--- Comment #14 from Benjamin Kosnik <bkoz at gcc dot gnu.org> 2012-09-10 05:08:15 UTC ---
Author: bkoz
Date: Mon Sep 10 05:08:07 2012
New Revision: 191125

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191125
Log:
2012-09-09  Thiago Macieira  <thiago.macieira@intel.com>

    PR libstdc++/54172
        * libsupc++/guard.cc (__cxa_guard_acquire): Exit the loop earlier if
        we detect that another thread has had success. Don't compare_exchange
        from a finished state back to a waiting state. Comment.


Modified:
    branches/gcc-4_7-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_7-branch/libstdc++-v3/libsupc++/guard.cc

--- Comment #15 from Benjamin Kosnik <bkoz at gcc dot gnu.org> 2012-09-10 05:08:51 UTC ---
In 4.7-branch

[Bug libstdc++/54172] [4.7 Regression] __cxa_guard_acquire thread-safety issue

[bkoz at gcc dot gnu.org]

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

--- Comment #15 from Benjamin Kosnik <bkoz at gcc dot gnu.org> 2012-09-10 05:08:51 UTC ---
In 4.7-branch

[Bug libstdc++/54172] [4.7 Regression] __cxa_guard_acquire thread-safety issue

[jakub at gcc dot gnu.org]

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

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-09-11 15:23:01 UTC ---
Author: jakub
Date: Tue Sep 11 15:22:54 2012
New Revision: 191190

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191190
Log:
    PR libstdc++/54172
    * libsupc++/guard.cc (__cxa_guard_acquire): Fix up the last
    argument of the first __atomic_compare_exchange_n.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/libsupc++/guard.cc

[Bug libstdc++/54172] [4.7 Regression] __cxa_guard_acquire thread-safety issue

[jakub at gcc dot gnu.org]

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

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-09-11 15:23:01 UTC ---
Author: jakub
Date: Tue Sep 11 15:22:54 2012
New Revision: 191190

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191190
Log:
    PR libstdc++/54172
    * libsupc++/guard.cc (__cxa_guard_acquire): Fix up the last
    argument of the first __atomic_compare_exchange_n.

Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/libsupc++/guard.cc

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-09-11 15:24:12 UTC ---
Author: jakub
Date: Tue Sep 11 15:24:06 2012
New Revision: 191191

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191191
Log:
    PR libstdc++/54172
    * libsupc++/guard.cc (__cxa_guard_acquire): Fix up the last
    argument of the first __atomic_compare_exchange_n.

Modified:
    branches/gcc-4_7-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_7-branch/libstdc++-v3/libsupc++/guard.cc

[Bug libstdc++/54172] [4.7 Regression] __cxa_guard_acquire thread-safety issue

[jakub at gcc dot gnu.org]

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

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-09-11 15:24:12 UTC ---
Author: jakub
Date: Tue Sep 11 15:24:06 2012
New Revision: 191191

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=191191
Log:
    PR libstdc++/54172
    * libsupc++/guard.cc (__cxa_guard_acquire): Fix up the last
    argument of the first __atomic_compare_exchange_n.

Modified:
    branches/gcc-4_7-branch/libstdc++-v3/ChangeLog
    branches/gcc-4_7-branch/libstdc++-v3/libsupc++/guard.cc

[Bug libstdc++/54172] [4.7 Regression] __cxa_guard_acquire thread-safety issue

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.7.2                       |4.7.3

--- Comment #18 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-09-20 10:17:37 UTC ---
GCC 4.7.2 has been released.

[Bug libstdc++/54172] [4.7 Regression] __cxa_guard_acquire thread-safety issue

[bkoz at gcc dot gnu.org]

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

Benjamin Kosnik <bkoz at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |bkoz at gcc dot gnu.org
         Resolution|                            |FIXED
   Target Milestone|4.7.3                       |4.7.2

--- Comment #19 from Benjamin Kosnik <bkoz at gcc dot gnu.org> 2012-09-26 18:40:33 UTC ---
Fixed in trunk and 4.7.2.