pthread_once hangs when init routine throws an exception [BZ #18435]

The C++ 2011 std::call_once function is specified to allow
the initialization routine to exit by throwing an exception.
Such an execution, termed exceptional, requires call_once to
propagate the exception to its caller. A program may contain
any number of exceptional executions but only one returning
execution (which, if it exists, must be the last execution
with the same once flag).

On POSIX systems such as Linux std::call_once is implemented
in terms of pthread_once. However, as discussed in libstdc++
bug 66146 - "call_once not C++11-compliant on ppc64le," GLIBC's
pthread_once hangs when the initialization function exits by
throwing an exception on at least arm and ppc64 (though
apparently not on x86_64). This effectively prevents call_once
from conforming to the C++ requirements since there doesn't
appear to be a thread-safe way to work around this problem in
libstdc++.

The attached patch changes pthread_once to handle gracefully
init functions that exit by throwing exceptions. It has been
tested on ppc64, ppc64le, and x86_64 with no regressions.

During the discussion of the bug concerns were raised about
whether the use case of throwing exceptions from the
pthread_once init routine is intended to be supported either
by POSIX, or by GLIBC. After some research I believe that both
POSIX and GLIBC have, in fact, intended to support it, for at
least two reasons:

First, the POSIX Rationale states in section Thread Cancellation
Overview, under Thread Cancellation Cleanup Handlers, that:

   it is an explicit goal of POSIX.1-2008 to be compatible with
   existing exception facilities and languages having exceptions.

Second, as is evident from the comment above the pthread_once
declaration in GLIBC (quoted below), GLIBC too has intended
to support this use case since 2004 when the comment was
added (and the __THROW specification removed from the API):

    ...
    The initialization functions might throw exception which
    is why this function is not marked with __THROW.  */

Martin

On 05/31/2015 11:37 PM, Martin Sebor wrote:
> The C++ 2011 std::call_once function is specified to allow
> the initialization routine to exit by throwing an exception.
> Such an execution, termed exceptional, requires call_once to
> propagate the exception to its caller. A program may contain
> any number of exceptional executions but only one returning
> execution (which, if it exists, must be the last execution
> with the same once flag).

What do you propose as the commit message?

It's not immediately obvious to me why this change works. :)

On 31/05/15 22:37, Martin Sebor wrote:
> The C++ 2011 std::call_once function is specified to allow
> the initialization routine to exit by throwing an exception.
> Such an execution, termed exceptional, requires call_once to
> propagate the exception to its caller. A program may contain
> any number of exceptional executions but only one returning
> execution (which, if it exists, must be the last execution
> with the same once flag).
> 
> On POSIX systems such as Linux std::call_once is implemented
> in terms of pthread_once. However, as discussed in libstdc++
> bug 66146 - "call_once not C++11-compliant on ppc64le," GLIBC's
> pthread_once hangs when the initialization function exits by
> throwing an exception on at least arm and ppc64 (though
> apparently not on x86_64). This effectively prevents call_once
> from conforming to the C++ requirements since there doesn't
> appear to be a thread-safe way to work around this problem in
> libstdc++.
> 

i think this should be fixed in libstdc++ even if glibc guarantees
that exceptions work from the pthread_once init function.

posix cannot give c++ exception safety guarantees.
(even if the rationale has vague statements along those lines).

(libstdc++ lock semantics does not match posix mutexes either:
for c++ weaker acquire-release ordering is enough).

so maybe libstdc++ should consider implementing locks and call_once
with the correct semantics independent of libc (and then maybe it
can get rid of the horrible gthr-posix.h weak reference hacks too).

Martin Sebor <msebor@redhat.com> writes:

> diff --git a/nptl/Makefile b/nptl/Makefile
> index d784c8d..1bf35cb 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -203,6 +203,7 @@ CFLAGS-send.c = -fexceptions -fasynchronous-unwind-tables
>  
>  CFLAGS-pt-system.c = -fexceptions
>  
> +LDFLAGS-tst-once5 = -lstdc++

That should be LDLIBS-tst-once5.

Andreas.

On 06/01/2015 02:38 AM, Florian Weimer wrote:
> On 05/31/2015 11:37 PM, Martin Sebor wrote:
>> The C++ 2011 std::call_once function is specified to allow
>> the initialization routine to exit by throwing an exception.
>> Such an execution, termed exceptional, requires call_once to
>> propagate the exception to its caller. A program may contain
>> any number of exceptional executions but only one returning
>> execution (which, if it exists, must be the last execution
>> with the same once flag).
>
> What do you propose as the commit message?

I usually repeat the ChangeLog entry in my commit messages
(included in the patch) but I'd be happy to add more detail
or change the format if there's a preference one way or the
other.

>
> It's not immediately obvious to me why this change works. :)

The pthread_cleanup_push and pthread_cleanup_pop macros are
defined in sysdeps/nptl/pthread.h. In C code outside of
glibc compiled with -fexceptions (with __EXCEPTIONS defined),
they make use of GCC attribute cleanup to invoke the cleanup
handler during stack unwinding. (In C++ they make use of
a destructor of a class object.) These macros do the right
thing when an exception is thrown in the code they surround.

The problem is that the nptl/pthreadP.h header redefines
the macros for internal use by glibc without the use of
the cleanup attribute. As a result, when an exception is
thrown, the cleanup handler is not invoked. This is what
happens in pthread_once.c.

By removing the macro redefinitions from nptl/pthreadP.h
the change causes pthread_once.c to be compiled with the
more robust macros defined in pthread.h and allows cleanup
to take place even after an exception has been thrown so
long as glibc has been compiled with -fexceptions.

Martin

> i think this should be fixed in libstdc++ even if glibc guarantees
> that exceptions work from the pthread_once init function.

I'm not sure this is fixable in libstdc++ without giving up
interoperability with the rest of glibc (handling forks)
or without relying on glibc's implementation details, and
without breaking compatibility.

But this patch is a useful improvement to glibc irrespective
of libstdc++ since its pthread_once is already exception safe
on x86. Making it exception-safe on other targets allows more
mixed-language code to be portable.

Beyond portability, avoiding a deadlock also renders moot any
security concerns about using the API as a vector for denial
of service attacks.

>
> posix cannot give c++ exception safety guarantees.
> (even if the rationale has vague statements along those lines).

You're right that POSIX cannot really speak about exception
safety. But POSIX shouldn't (and doesn't) prevent programs
written in languages with exceptions from making use of
its APIs. pthread_once also isn't the only POSIX API that
interacts with exceptions. Atexit, bsearch, lsearch and
qsort are examples of others. In glibc, as far as I can
tell, they all work correctly in the presence of exceptions.

Looking ahead, as glibc implements C11 threads and as C++
adopts C11, making C11 call_once deal with exceptions
the same way C++ 11 std::call_once does will likely become
a (C++) requirement.

> so maybe libstdc++ should consider implementing locks and call_once
> with the correct semantics independent of libc (and then maybe it
> can get rid of the horrible gthr-posix.h weak reference hacks too).

I suspect implementing std::call_once independently of
glibc isn't possible without breaking compatibility (but
I'd have to study the spec and libstdc++ code to say for
certain).

Beyond call_once, C++ 11 encourages implementations to
provide a member function called native_handle() that
returns a reference to the underlying native object.
libstdc++ mutex (and other classes) expose this member
function to provide interoperability with pthreads. As
a result, reimplementing libstdc++ independently of libc
is impossible without losing such interoperability and
without breaking compatibility with existing code.

Martin

On 01 Jun 2015 09:55, Martin Sebor wrote:
> On 06/01/2015 02:38 AM, Florian Weimer wrote:
> > On 05/31/2015 11:37 PM, Martin Sebor wrote:
> >> The C++ 2011 std::call_once function is specified to allow
> >> the initialization routine to exit by throwing an exception.
> >> Such an execution, termed exceptional, requires call_once to
> >> propagate the exception to its caller. A program may contain
> >> any number of exceptional executions but only one returning
> >> execution (which, if it exists, must be the last execution
> >> with the same once flag).
> >
> > What do you propose as the commit message?
> 
> I usually repeat the ChangeLog entry in my commit messages
> (included in the patch) but I'd be happy to add more detail
> or change the format if there's a preference one way or the
> other.

it would be best if your commit message was useful.  usually that means your 
e-mail that was sent to the list w/the patch explaining what it was all about.
if you're familiar with the linux kernel git message style, then that ;).

> > It's not immediately obvious to me why this change works. :)
> 
> The pthread_cleanup_push and pthread_cleanup_pop macros are
> defined in sysdeps/nptl/pthread.h. In C code outside of
> glibc compiled with -fexceptions (with __EXCEPTIONS defined),
> they make use of GCC attribute cleanup to invoke the cleanup
> handler during stack unwinding. (In C++ they make use of
> a destructor of a class object.) These macros do the right
> thing when an exception is thrown in the code they surround.
> 
> The problem is that the nptl/pthreadP.h header redefines
> the macros for internal use by glibc without the use of
> the cleanup attribute. As a result, when an exception is
> thrown, the cleanup handler is not invoked. This is what
> happens in pthread_once.c.
> 
> By removing the macro redefinitions from nptl/pthreadP.h
> the change causes pthread_once.c to be compiled with the
> more robust macros defined in pthread.h and allows cleanup
> to take place even after an exception has been thrown so
> long as glibc has been compiled with -fexceptions.

they're in there to avoid the PLT right ?  so deleting them means the relocs 
come back.  can't we get both here ?
-mike

On Mon, 2015-06-01 at 11:20 +0100, Szabolcs Nagy wrote:
> (libstdc++ lock semantics does not match posix mutexes either:
> for c++ weaker acquire-release ordering is enough).

I agree regarding what's specified.  However, I don't see us use
anything stronger than mo_acquire for lock acquisitions in glibc because
it would slow down the vast majority of uses in order to make arguably
arcane use cases work that never worked with glibc so far (e.g., abusing
locks or other sync ops that return success as full barriers and abusing
semaphores as mo_relaxed atomics).

> so maybe libstdc++ should consider implementing locks and call_once
> with the correct semantics independent of libc

There's pros and cons to that.  I agree that libstdc++ could implement
exactly the semantics it requires and benefit in some cases (e.g., don't
have to abort HTM txns on trylock in the Intel-RTM-based HLE
implementation).  However, we'd duplicate code that we'd eventually like
to tune.  libstdc++ couldn't use tuning of the PThreads implementation
on non-glibc platforms.

Most importantly perhaps, in the long run, the way you block or spin can
depend on how threads are scheduled and distributed across the machine.
This is information that's most naturally contained in glibc, so if you
want to make use of that synergy, implementing base synchronization
together with threads in glibc is the easiest way.  Likewise, libstdc++
can share compute resources with non-C++ code (e.g., OpenMP stuff), and
glibc is naturally below both libgomp and libstdc++.

On Mon, 2015-06-01 at 12:41 -0600, Martin Sebor wrote:
> Beyond call_once, C++ 11 encourages implementations to
> provide a member function called native_handle() that
> returns a reference to the underlying native object.
> libstdc++ mutex (and other classes) expose this member
> function to provide interoperability with pthreads. As
> a result, reimplementing libstdc++ independently of libc
> is impossible without losing such interoperability and
> without breaking compatibility with existing code.

Note that the interoperability is also a dependency.  I think it's
actually better for libstdc++ to not make an ABI guarantee that
native_handle() returns a pointer to a PThreads entity;  this way, we do
have the freedom to improve libstdc++ independently of glibc and more
importantly of POSIX.
I don't know for sure whether Jonathan has applied the change already,
but IIRC, we agreed that native_handle() should return void*.

On 03/06/15 13:03 +0200, Torvald Riegel wrote:
>I don't know for sure whether Jonathan has applied the change already,
>but IIRC, we agreed that native_handle() should return void*.

No, I forgot to do it for gcc-5.

On Mon, 2015-06-01 at 09:55 -0600, Martin Sebor wrote:
> On 06/01/2015 02:38 AM, Florian Weimer wrote:
> > It's not immediately obvious to me why this change works. :)
> 
> The pthread_cleanup_push and pthread_cleanup_pop macros are
> defined in sysdeps/nptl/pthread.h. In C code outside of
> glibc compiled with -fexceptions (with __EXCEPTIONS defined),
> they make use of GCC attribute cleanup to invoke the cleanup
> handler during stack unwinding. (In C++ they make use of
> a destructor of a class object.) These macros do the right
> thing when an exception is thrown in the code they surround.
> 
> The problem is that the nptl/pthreadP.h header redefines
> the macros for internal use by glibc without the use of
> the cleanup attribute. As a result, when an exception is
> thrown, the cleanup handler is not invoked. This is what
> happens in pthread_once.c.
> 
> By removing the macro redefinitions from nptl/pthreadP.h
> the change causes pthread_once.c to be compiled with the
> more robust macros defined in pthread.h and allows cleanup
> to take place even after an exception has been thrown so
> long as glibc has been compiled with -fexceptions.

This sounds reasonable for me, but I don't know enough about this to
know for sure :)

I agree with the motivation of the change (ie, making it possible for
libstdc++ to use pthread_once in the implementation);  I haven't looked
at this in detail, but your arguments regarding C/C++ compatibility that
you made elsewhere in the thread seemed fine for me.

On Wed, Jun 03, 2015 at 01:03:47PM +0200, Torvald Riegel wrote:
> On Mon, 2015-06-01 at 12:41 -0600, Martin Sebor wrote:
> > Beyond call_once, C++ 11 encourages implementations to
> > provide a member function called native_handle() that
> > returns a reference to the underlying native object.
> > libstdc++ mutex (and other classes) expose this member
> > function to provide interoperability with pthreads. As
> > a result, reimplementing libstdc++ independently of libc
> > is impossible without losing such interoperability and
> > without breaking compatibility with existing code.
> 
> Note that the interoperability is also a dependency.  I think it's
> actually better for libstdc++ to not make an ABI guarantee that
> native_handle() returns a pointer to a PThreads entity;  this way, we do
> have the freedom to improve libstdc++ independently of glibc and more
> importantly of POSIX.
> I don't know for sure whether Jonathan has applied the change already,
> but IIRC, we agreed that native_handle() should return void*.

I'm strongly in favor of having the underlying implementations be
opaque and not guaranteeing pthread.

Rich

On 06/03/2015 01:14 PM, Rich Felker wrote:
> On Wed, Jun 03, 2015 at 01:03:47PM +0200, Torvald Riegel wrote:
>> On Mon, 2015-06-01 at 12:41 -0600, Martin Sebor wrote:
>>> Beyond call_once, C++ 11 encourages implementations to
>>> provide a member function called native_handle() that
>>> returns a reference to the underlying native object.
>>> libstdc++ mutex (and other classes) expose this member
>>> function to provide interoperability with pthreads. As
>>> a result, reimplementing libstdc++ independently of libc
>>> is impossible without losing such interoperability and
>>> without breaking compatibility with existing code.
>>
>> Note that the interoperability is also a dependency.  I think it's
>> actually better for libstdc++ to not make an ABI guarantee that
>> native_handle() returns a pointer to a PThreads entity;  this way, we do
>> have the freedom to improve libstdc++ independently of glibc and more
>> importantly of POSIX.
>> I don't know for sure whether Jonathan has applied the change already,
>> but IIRC, we agreed that native_handle() should return void*.
>
> I'm strongly in favor of having the underlying implementations be
> opaque and not guaranteeing pthread.

As the risk of continuing to diverge from the main topic of
this thread I think it might be worth clarifying this point.

The sole purpose of the native_handle() function (whatever
its return type may be) is to make it possible for components
such as libraries to operate on the same mutex or thread object
using the pthreads (or other "native") API as C++ programs that
create and manipulate the objects using the C++ APIs and use
the libraries. (I.e., the purpose is not just to expose some
sort of a unique id for the object).

Since libstdc++ is already implemented in terms of pthreads
and exposes the underlying pthreads types via native_handle
(with all the ABI/API ramifications), I suspect there's
little chance that the underlying implementation can change.

Clang's libc++ does the same thing, as does the Visual C++
standard library (which exposes the OS HANDLE (void*) as its
native_handle_type).

Martin

On Wed, Jun 03, 2015 at 02:14:10PM -0600, Martin Sebor wrote:
> On 06/03/2015 01:14 PM, Rich Felker wrote:
> >On Wed, Jun 03, 2015 at 01:03:47PM +0200, Torvald Riegel wrote:
> >>On Mon, 2015-06-01 at 12:41 -0600, Martin Sebor wrote:
> >>>Beyond call_once, C++ 11 encourages implementations to
> >>>provide a member function called native_handle() that
> >>>returns a reference to the underlying native object.
> >>>libstdc++ mutex (and other classes) expose this member
> >>>function to provide interoperability with pthreads. As
> >>>a result, reimplementing libstdc++ independently of libc
> >>>is impossible without losing such interoperability and
> >>>without breaking compatibility with existing code.
> >>
> >>Note that the interoperability is also a dependency.  I think it's
> >>actually better for libstdc++ to not make an ABI guarantee that
> >>native_handle() returns a pointer to a PThreads entity;  this way, we do
> >>have the freedom to improve libstdc++ independently of glibc and more
> >>importantly of POSIX.
> >>I don't know for sure whether Jonathan has applied the change already,
> >>but IIRC, we agreed that native_handle() should return void*.
> >
> >I'm strongly in favor of having the underlying implementations be
> >opaque and not guaranteeing pthread.
> 
> As the risk of continuing to diverge from the main topic of
> this thread I think it might be worth clarifying this point.
> 
> The sole purpose of the native_handle() function (whatever
> its return type may be) is to make it possible for components
> such as libraries to operate on the same mutex or thread object
> using the pthreads (or other "native") API as C++ programs that
> create and manipulate the objects using the C++ APIs and use
> the libraries. (I.e., the purpose is not just to expose some
> sort of a unique id for the object).
> 
> Since libstdc++ is already implemented in terms of pthreads
> and exposes the underlying pthreads types via native_handle
> (with all the ABI/API ramifications), I suspect there's
> little chance that the underlying implementation can change.
> 
> Clang's libc++ does the same thing, as does the Visual C++
> standard library (which exposes the OS HANDLE (void*) as its
> native_handle_type).

I understand the purpose here, but I also think it's a bad design and
not useful in portable code, which cannot make any assumptions about
the underlying system objects used by the C++ standard library of even
that "underlying" objects exist. I think the whole native_handle() API
just encourages non-portable constructs and implementation lock-in
that is harmful both to implementors (less freedom to improve) and
applications (trapped by implementation-specific assumptions).

Rich

> I understand the purpose here, but I also think it's a bad design and
> not useful in portable code, which cannot make any assumptions about
> the underlying system objects used by the C++ standard library of even
> that "underlying" objects exist. I think the whole native_handle() API
> just encourages non-portable constructs and implementation lock-in
> that is harmful both to implementors (less freedom to improve) and
> applications (trapped by implementation-specific assumptions).

You're right, it does have that effect. It was a conscious
design choice made by the authors of the C++ threads library.
IIRC, some of the early design alternative included:

1) providing a rich set of functions that exposes the union of
    functionality of the underlying implementations; functions
    that don't have a native equivalent simply fail (the Rogue
    Wave Threads library does that)

2) providing a narrow interface (the C11 threads library takes
    this approach, though I don't think it precludes (3))

3) providing a narrow interface that exposes the common subset
    of functionality and give users a means to access the wrapped
    native objects

The challenge with (1) is that it can be difficult to both
specify and use portably because sometimes there are similar
underlying APIs but with subtle differences.

The problem with (2) is that it tends to be overly constraining
for non-trivial uses.

So C++ chose (3).

Martin

On Wed, Jun 03, 2015 at 05:49:06PM -0600, Martin Sebor wrote:
> >I understand the purpose here, but I also think it's a bad design and
> >not useful in portable code, which cannot make any assumptions about
> >the underlying system objects used by the C++ standard library of even
> >that "underlying" objects exist. I think the whole native_handle() API
> >just encourages non-portable constructs and implementation lock-in
> >that is harmful both to implementors (less freedom to improve) and
> >applications (trapped by implementation-specific assumptions).
> 
> You're right, it does have that effect. It was a conscious
> design choice made by the authors of the C++ threads library.
> IIRC, some of the early design alternative included:
> 
> 1) providing a rich set of functions that exposes the union of
>    functionality of the underlying implementations; functions
>    that don't have a native equivalent simply fail (the Rogue
>    Wave Threads library does that)
> 
> 2) providing a narrow interface (the C11 threads library takes
>    this approach, though I don't think it precludes (3))
> 
> 3) providing a narrow interface that exposes the common subset
>    of functionality and give users a means to access the wrapped
>    native objects
> 
> The challenge with (1) is that it can be difficult to both
> specify and use portably because sometimes there are similar
> underlying APIs but with subtle differences.
> 
> The problem with (2) is that it tends to be overly constraining
> for non-trivial uses.
> 
> So C++ chose (3).

Is there anything non-conforming about making native_handle() just
return &this (i.e. considering the C++ object its own native
primitive)? That seems like the right solution.

Choice 3 is the utterly worst choice because once an application goes
down that path, there's no way to extricate the mess and make it
portable. If you need functionality that the C++ synchronization
objects don't provide then you need to make your own objects using
some other primitives, not make assumptions about how the C++ standard
library's primitives are implemented.

Rich

> Is there anything non-conforming about making native_handle() just
> return &this (i.e. considering the C++ object its own native
> primitive)? That seems like the right solution.

It wouldn't be non-conforming (just pointless). Whether or not
the functions even exist is implementation-defined, as is their
return type and their semantics. But for implementations that
have already made the choice to expose the underlying native
object, it's water under the bridge. They can't change what
the functions return (or very well how they're implemented)
without breaking programs that depend on it.

Martin

On Wed, 2015-06-03 at 14:14 -0600, Martin Sebor wrote:
> The sole purpose of the native_handle() function (whatever
> its return type may be) is to make it possible for components
> such as libraries to operate on the same mutex or thread object
> using the pthreads (or other "native") API as C++ programs that
> create and manipulate the objects using the C++ APIs and use
> the libraries. (I.e., the purpose is not just to expose some
> sort of a unique id for the object).

I think everyone of us is aware of that.

That can be useful in some cases, but it is really constraining in
others.  It essentially ties the implementation to two APIs and ABIs.
We are giving out access to implementation details.  It also ties
libstdc++ to two standards bodies.

The problem with mutexes in particular is that POSIX and C++11 mutexes
do *not* have exactly the same semantics.  If POSIX should, wrongly so
IMO, insist on requiring successful mutex operations to have the effect
of a full barrier, then we will have to run with a different C++
implementation because we don't want to have this overhead for C++ too.
We could alternatively add a C++ mode as a new mutex kind (ie, a glibc
extension) and return mutexes of this kind, but that wouldn't be
compatible with old code either.

Therefore, I think giving out a void* with no strings attached, or not
supporting native_handle at all, are the right solutions so we don't end
up in a situation in which we can't fix or improve something without
both standards moving in the same direction.

> Since libstdc++ is already implemented in terms of pthreads
> and exposes the underlying pthreads types via native_handle
> (with all the ABI/API ramifications), I suspect there's
> little chance that the underlying implementation can change.

We wanted to change to void* with GCC-5, but missed that boat
unfortunately.  We can document that we don't give ABI/API guarantees
for it, but this would be much clearer to users if this would return
void* I believe.

>> By removing the macro redefinitions from nptl/pthreadP.h
>> the change causes pthread_once.c to be compiled with the
>> more robust macros defined in pthread.h and allows cleanup
>> to take place even after an exception has been thrown so
>> long as glibc has been compiled with -fexceptions.
>
> they're in there to avoid the PLT right ?  so deleting them means the relocs
> come back.  can't we get both here ?

Looking at the powepc64 assembly for ___pthread_once_slow before
and after the change, there are no PLT calls in either. The only
difference is that with the change, the function calls GCC's
_Unwind_Resume which is a direct branch to the function.

Comparing the rest of libpthread.so before and after the change,
the number of calls via the PLT is the same.

I admit I'm not sure what the intended purpose of the internal
macros was. I had searched git logs before I made the change
but couldn't find anything helpful beyond some ChangeLog entries
that referred to it as an internal optimization. They seem to
have been introduced before 2001 (the year of the first Git
commit, AFAICS).

If I missed something let me know.

Martin

On 03/06/15 20:21 -0400, Rich Felker wrote:
>Is there anything non-conforming about making native_handle() just
>return &this (i.e. considering the C++ object its own native
>primitive)? That seems like the right solution.

As Martin said, it would be completely pointless. If you don't want to
allow access to the underlying primitive then you don't define
native_handle() at all.

>Choice 3 is the utterly worst choice because once an application goes
>down that path, there's no way to extricate the mess and make it
>portable. If you need functionality that the C++ synchronization
>objects don't provide then you need to make your own objects using
>some other primitives, not make assumptions about how the C++ standard
>library's primitives are implemented.

For portable code, yes. But native_handle() is meant for non-portable
uses.

On 04/06/15 09:51 +0200, Torvald Riegel wrote:
>We wanted to change to void* with GCC-5, but missed that boat
>unfortunately.  We can document that we don't give ABI/API guarantees
>for it, but this would be much clearer to users if this would return
>void* I believe.

I think it's still OK to change it to void* now, and document that it
is currently the address of the POSIX primitive, but that the function
could be modified or removed in future.

A disadvantage of returning void* is that if we do change what it
returns then code using it still compiles and links. If we change it
from returning pthread_xxx_t* to something else, or just remove it
entirely, then code using it fails earlier, at compile-time.

So we need to document that there are no guarantees for it, whatever
it returns. Simply changing to void* isn't enough.

On 08/06/15 12:23, Jonathan Wakely wrote:
> On 03/06/15 20:21 -0400, Rich Felker wrote:
>> Is there anything non-conforming about making native_handle() just
>> return &this (i.e. considering the C++ object its own native
>> primitive)? That seems like the right solution.
> 
> As Martin said, it would be completely pointless. If you don't want to
> allow access to the underlying primitive then you don't define
> native_handle() at all.

does libstdc++ want to allow access to the underlying primitives?

ie. is it documented that the types are mapped to pthread types
and how to use them safely?

>> Choice 3 is the utterly worst choice because once an application goes
>> down that path, there's no way to extricate the mess and make it
>> portable. If you need functionality that the C++ synchronization
>> objects don't provide then you need to make your own objects using
>> some other primitives, not make assumptions about how the C++ standard
>> library's primitives are implemented.
> 
> For portable code, yes. But native_handle() is meant for non-portable
> uses.

may be my idea about redoing the synchronization primitives in libstdc++
does not work, but there can be other reasons for libstdc++ to have
its semantics detached from the pthread api.  allowing user code to use
pthread primitives behind the back of libstdc++ sounds problematic even
for 'non-portable' code.

i tried to dig up the rationale for native_handle but it seems early c++0x
documents already had it, presumably copied from boost::thread.  i guess
it's too late to fix this now..

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2184.html
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2285.html

http://www.boost.org/doc/libs/1_58_0/doc/html/thread/thread_management.html#thread.thread_management.tutorial.native_in

On 06/08/2015 01:27 PM, Jonathan Wakely wrote:

> A disadvantage of returning void* is that if we do change what it
> returns then code using it still compiles and links. If we change it
> from returning pthread_xxx_t* to something else, or just remove it
> entirely, then code using it fails earlier, at compile-time.

C++ doesn't have implicit conversion from void * to pthread_mutex_t
etc., so existing code (which presumably doesn't have cast) would fail
to compile.  I think.

On 05/31/2015 05:37 PM, Martin Sebor wrote:
> The C++ 2011 std::call_once function is specified to allow
> the initialization routine to exit by throwing an exception.
> Such an execution, termed exceptional, requires call_once to
> propagate the exception to its caller. A program may contain
> any number of exceptional executions but only one returning
> execution (which, if it exists, must be the last execution
> with the same once flag).
> 
> On POSIX systems such as Linux std::call_once is implemented
> in terms of pthread_once. However, as discussed in libstdc++
> bug 66146 - "call_once not C++11-compliant on ppc64le," GLIBC's
> pthread_once hangs when the initialization function exits by
> throwing an exception on at least arm and ppc64 (though
> apparently not on x86_64). This effectively prevents call_once
> from conforming to the C++ requirements since there doesn't
> appear to be a thread-safe way to work around this problem in
> libstdc++.
> 
> The attached patch changes pthread_once to handle gracefully
> init functions that exit by throwing exceptions. It has been
> tested on ppc64, ppc64le, and x86_64 with no regressions.
> 
> During the discussion of the bug concerns were raised about
> whether the use case of throwing exceptions from the
> pthread_once init routine is intended to be supported either
> by POSIX, or by GLIBC. After some research I believe that both
> POSIX and GLIBC have, in fact, intended to support it, for at
> least two reasons:
> 
> First, the POSIX Rationale states in section Thread Cancellation
> Overview, under Thread Cancellation Cleanup Handlers, that:
> 
>   it is an explicit goal of POSIX.1-2008 to be compatible with
>   existing exception facilities and languages having exceptions.
> 
> Second, as is evident from the comment above the pthread_once
> declaration in GLIBC (quoted below), GLIBC too has intended
> to support this use case since 2004 when the comment was
> added (and the __THROW specification removed from the API):
> 
>    ...
>    The initialization functions might throw exception which
>    is why this function is not marked with __THROW.  */

This patch has serious problems which cause regressions on at
least aarch64 and possibly other arches.

The imposed requirement that all cancellation sites be compiled
with -fasynchronous-unwind-tables is problematic, particularly
for old applications which won't have done this and cancellation
will silently fail to run cancel handlers.

Worse is that as the compiler moves around the asm cancellation
wrapper for the syscall outside of the cleanup region because
the compiler assumes asm can never raise exceptions. This is
the more serious issue that needs addressing.

Please revert this patch. We need to look at another solution
that doesn't regress any tests.

Cheers,
Carlos.

> This patch has serious problems which cause regressions on at
> least aarch64 and possibly other arches.

I finally got an Aarch64 box, managed to reproduce one of the two
reported test regressions (the one in tst-join5; the other test
passes for me) and have been debugging it in between other tasks.

>
> The imposed requirement that all cancellation sites be compiled
> with -fasynchronous-unwind-tables is problematic, particularly
> for old applications which won't have done this and cancellation
> will silently fail to run cancel handlers.

I'm not sure I understand what you mean here. The patch doesn't
introduce any assumptions that didn't exist before. Callers of
the cancellation functions don't depend on -fasynchronous-unwind-
tables: only the functions themselves do (when __EXCEPTIONS is
defined), and they are being compiled that way.

>
> Worse is that as the compiler moves around the asm cancellation
> wrapper for the syscall outside of the cleanup region because
> the compiler assumes asm can never raise exceptions. This is
> the more serious issue that needs addressing.

The asm is declared volatile memory so the compiler shouldn't
reorder it with other statements that perform memory accesses.

But the problem does appear to be sensitive to inlining in
pthread_join.c. When I outline the call to lll_wait_tid the
problem disappears. But when comparing the assembly between
the two versions of the file I don't see the system call being
moved past the cleanup call (the cleanup, when outlined, is
the second to last call in the function, just before the one
to _Unwind_Resume). The call just doesn't take place. I need
to study the assembly in more detail to understand exactly
where the problem is.

>
> Please revert this patch. We need to look at another solution
> that doesn't regress any tests.

I've seen your note about getting ready for the 2.22 release
so I wouldn't want to jeopardize the schedule. But if there's
some slack (say a few days) I would like to get to the bottom
of this and try to resolve the problem without reverting the
patch. But if it's urgent, I will certainly revert the patch
and work on fixing this for 2.23.

Martin

* Martin Sebor <msebor@gmail.com> [2015-07-08 15:28:23 -0600]:
> >This patch has serious problems which cause regressions on at
> >least aarch64 and possibly other arches.
> 
> I finally got an Aarch64 box, managed to reproduce one of the two
> reported test regressions (the one in tst-join5; the other test
> passes for me) and have been debugging it in between other tasks.
> 

we know the root cause already (you were somehow left
off from the cc at some point).

here is my analysis and carlos' reply:
http://sourceware.org/ml/libc-alpha/2015-07/msg00260.html

> I'm not sure I understand what you mean here. The patch doesn't
> introduce any assumptions that didn't exist before. Callers of
> the cancellation functions don't depend on -fasynchronous-unwind-
> tables: only the functions themselves do (when __EXCEPTIONS is
> defined), and they are being compiled that way.

the new requirement is to compile pthread_once's
callback argument with async unwind info.

> >Worse is that as the compiler moves around the asm cancellation
> >wrapper for the syscall outside of the cleanup region because
> >the compiler assumes asm can never raise exceptions. This is
> >the more serious issue that needs addressing.
> 
> The asm is declared volatile memory so the compiler shouldn't
> reorder it with other statements that perform memory accesses.
> 
> But the problem does appear to be sensitive to inlining in
> pthread_join.c. When I outline the call to lll_wait_tid the
> problem disappears. But when comparing the assembly between
> the two versions of the file I don't see the system call being
> moved past the cleanup call (the cleanup, when outlined, is
> the second to last call in the function, just before the one
> to _Unwind_Resume). The call just doesn't take place. I need
> to study the assembly in more detail to understand exactly
> where the problem is.

the problem is that -fexceptions is not 'async unwind safe'

the cleanup handler is only guaranteed to run if the unwind
goes through extern functions (that may throw).

there is a proposed new cancellation design that gets rid
of async cancel + inline asm syscalls + cleanup handlers,
with that your patch would be safe, but without it, it isnt.
(that change is scheduled for 2.23)

> >Please revert this patch. We need to look at another solution
> >that doesn't regress any tests.
> 
> I've seen your note about getting ready for the 2.22 release
> so I wouldn't want to jeopardize the schedule. But if there's
> some slack (say a few days) I would like to get to the bottom
> of this and try to resolve the problem without reverting the
> patch. But if it's urgent, I will certainly revert the patch
> and work on fixing this for 2.23.
> 
> Martin

On 07/08/2015 04:13 PM, Szabolcs Nagy wrote:
> * Martin Sebor <msebor@gmail.com> [2015-07-08 15:28:23 -0600]:
>>> This patch has serious problems which cause regressions on at
>>> least aarch64 and possibly other arches.
>>
>> I finally got an Aarch64 box, managed to reproduce one of the two
>> reported test regressions (the one in tst-join5; the other test
>> passes for me) and have been debugging it in between other tasks.
>>
>
> we know the root cause already (you were somehow left
> off from the cc at some point).
>
> here is my analysis and carlos' reply:
> http://sourceware.org/ml/libc-alpha/2015-07/msg00260.html

Ah, thank you! I've been busy with another project and haven't
had a chance to read the list. Let me digest it and follow up
on the thread. (A couple of comments are below.)

>
>> I'm not sure I understand what you mean here. The patch doesn't
>> introduce any assumptions that didn't exist before. Callers of
>> the cancellation functions don't depend on -fasynchronous-unwind-
>> tables: only the functions themselves do (when __EXCEPTIONS is
>> defined), and they are being compiled that way.
>
> the new requirement is to compile pthread_once's
> callback argument with async unwind info.

That's only required in C++ code that throws exceptions from
the once function. C callers are not affected (and in my
tests on aarch64 and ppc64le, they're compiled with neither
-fexceptions or -fasynchronous-unwind-tables and succeed).
This includes tst-once3 which you reported as failing so
there must be something more subtle going on.

> the problem is that -fexceptions is not 'async unwind safe'
>
> the cleanup handler is only guaranteed to run if the unwind
> goes through extern functions (that may throw).
>
> there is a proposed new cancellation design that gets rid
> of async cancel + inline asm syscalls + cleanup handlers,
> with that your patch would be safe, but without it, it isnt.
> (that change is scheduled for 2.23)

I'm fine deferring the patch until 2.23, though I would like
to understand why we're seeing different results for some
of the tests.

Martin

* Martin Sebor <msebor@gmail.com> [2015-07-08 16:52:31 -0600]:
> On 07/08/2015 04:13 PM, Szabolcs Nagy wrote:
> >* Martin Sebor <msebor@gmail.com> [2015-07-08 15:28:23 -0600]:
> >>I'm not sure I understand what you mean here. The patch doesn't
> >>introduce any assumptions that didn't exist before. Callers of
> >>the cancellation functions don't depend on -fasynchronous-unwind-
> >>tables: only the functions themselves do (when __EXCEPTIONS is
> >>defined), and they are being compiled that way.
> >
> >the new requirement is to compile pthread_once's
> >callback argument with async unwind info.
> 
> That's only required in C++ code that throws exceptions from
> the once function. C callers are not affected (and in my
> tests on aarch64 and ppc64le, they're compiled with neither
> -fexceptions or -fasynchronous-unwind-tables and succeed).
> This includes tst-once3 which you reported as failing so
> there must be something more subtle going on.

i see.

i don't understand why the unwind from the cancel signal
handler would be different from a c++ exception thrown
from the once function.

> >the problem is that -fexceptions is not 'async unwind safe'
> >
> >the cleanup handler is only guaranteed to run if the unwind
> >goes through extern functions (that may throw).
> >
> >there is a proposed new cancellation design that gets rid
> >of async cancel + inline asm syscalls + cleanup handlers,
> >with that your patch would be safe, but without it, it isnt.
> >(that change is scheduled for 2.23)
> 
> I'm fine deferring the patch until 2.23, though I would like
> to understand why we're seeing different results for some
> of the tests.

ok, i can check if different gcc version changes the outcome.


my wording above was not correct, what i meant was

-fexceptions -fasynchronous-unwind-tables

do not guarantee that the cleanup handlers are run
when an exception is thrown from an async signal handler,
which means PTHREAD_CANCEL_ASYNCRHONOUS case is broken.

asm volatile does not help because an instruction that
does not throw according to gcc can be moved outside the
exception handling range (jump outside and jump back)
and then the cleanup handler is not run if SIGCANCEL
happens at that instruction.

by new cancel design i was refering to this patch set:
http://sourceware.org/ml/libc-alpha/2015-06/msg00895.html
but now i see it does not fix the 'CANCEL_ASYNC' usage in
pthread_join, so further work is needed: as long as the
libc has async cancel with cleanups, the exception based
cleanup is not safe.

(it is a separate matter what external code should do
that uses async cancellation with cleanups, gcc can
probably fix -fexceptions to make that safe).

> That's only required in C++ code that throws exceptions from
> the once function. C callers are not affected (and in my
> tests on aarch64 and ppc64le, they're compiled with neither
> -fexceptions or -fasynchronous-unwind-tables and succeed).
> This includes tst-once3 which you reported as failing so
> there must be something more subtle going on.

I'm afraid I need to correct what I said above: tst-once3 does
pass on aarch64 when compiled with the system GCC 4.9.2(*) but
fails on both powerpc64 and powerpc64le, with both GCC 4.8.3 and
5.1.0. The test runs successfully to completion once I compile
it with -fasynchronous-unwind-tables.

The test fails on aarch64 when compiled with GCC 5.1.0 (just
the test alone).

I'm still trying to figure out why I didn't see the failure (or
missed it) it in my testing of the patch on powerpc.

In any case, I'll revert the patch tomorrow and pick it up again
after the 2.22 release.

Sorry for the trouble it has caused!
Martin

PS The system GCC on Fedora 21/aarch64 was configured with
--disable-libunwind-exceptions. I wonder if that's masking
the problem. I'll see if it goes away if I rebuild GCC 5.1
with the same option.

The part of the patch that was causing problems has been reverted
with the commit below. In discussing with Carlos whether or not
to also revert the test we felt that keeping it and marking it
XFAIL would be preferable to removing it. I couldn't come up with
a way to mark the test XFAIL only for a subset of targets (it
should pass on x86_64) so it might be reported as XPASS. If
someone can suggest a way to handle this more cleanly (i.e.,
only marking a test XFAIL where it's known to FAIL or not known
to PASS) that's not overly intrusive (i.e., doesn't require
touching many files) I'd be glad to make that change.

Martin

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=203c1a898dd2e281eae30f3c57ceb8d4a50b00f4

commit 203c1a898dd2e281eae30f3c57ceb8d4a50b00f4
Author: Martin Sebor <msebor@gcc.gnu.org>
Date:   Thu Jul 9 19:25:38 2015 -0400

     The patch committed to fix bug #18435 caused regressions on aarch64
     and also powerpc64 and powerpc64le. See the discussion in the thread
     below for details. This change reverts the problematic bits leaving
     the added test in place and marking XFAIL in anticipation of fixing
     the bug in the near future.
     https://sourceware.org/ml/libc-alpha/2015-07/msg00141.html

         [BZ #18435]
         * nptl/pthreadP.h (pthread_cleanup_push, pthread_cleanup_pop):
         Revert commit ed225df3ad9cbac3c22ec3f0fbbed1f9c61d1c54.
         * nptl/Makefile (test-xfail-tst-once5): Define.

-----------------------------------------------------------------------

Summary of changes:
  ChangeLog       |    7 +++++++
  NEWS            |   12 ++++++------
  nptl/Makefile   |    4 ++++
  nptl/pthreadP.h |   11 +++++++++++
  4 files changed, 28 insertions(+), 6 deletions(-)