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

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

[Florian Weimer]

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. :)

-- 
Florian Weimer / Red Hat Product Security

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

[Martin Sebor]

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

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

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 2269 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Martin Sebor]

>> 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

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

[Torvald Riegel]

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.

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

[Szabolcs Nagy]

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).

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

[Martin Sebor]

> 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

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

[Torvald Riegel]

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*.

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

[Jonathan Wakely]

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.

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

[Rich Felker]

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

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

[Martin Sebor]

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

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

[Rich Felker]

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

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

[Martin Sebor]

> 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

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

[Rich Felker]

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

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

[Martin Sebor]

> 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

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

[Jonathan Wakely]

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.

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

[Szabolcs Nagy]

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

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

[Torvald Riegel]

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.

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

[Jonathan Wakely]

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.

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

[Florian Weimer]

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.

-- 
Florian Weimer / Red Hat Product Security

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

[Torvald Riegel]

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++.

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

[Andreas Schwab]

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 2287 bytes --]

Attached is an updated version of the patch that addresses
the LDFLAGS -> LDLIBS comment. Retested on ppc64.

Is it okay to commit?

Martin

On 05/31/2015 03: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.  */
>
> Martin


[-- Attachment #2: glibc-18435-pthread_once-3.patch --]
[-- Type: text/x-patch, Size: 5361 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 353b383..1cd79e8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2015-06-09  Martin Sebor  <msebor@redhat.com>
+
+	[BZ #18435]
+	* nptl/Makefile: Add tst-once5.cc.
+	* nptl/pthreadP.h (pthread_cleanup_push, pthread_cleanup_pop):
+	Remove macro redefinitions.
+	* nptl/tst-once5.cc: New test.
+
 2015-06-09  Andrew Senkevich  <andrew.senkevich@intel.com>
 
 	* sysdeps/x86_64/fpu/Makefile: New file.
diff --git a/NEWS b/NEWS
index 53f244d..196cf3f 100644
--- a/NEWS
+++ b/NEWS
@@ -19,8 +19,8 @@ Version 2.22
   18043, 18046, 18047, 18049, 18068, 18080, 18093, 18100, 18104, 18110,
   18111, 18116, 18125, 18128, 18138, 18185, 18196, 18197, 18206, 18210,
   18211, 18217, 18220, 18221, 18234, 18244, 18247, 18287, 18319, 18324,
-  18333, 18346, 18397, 18409, 18410, 18412, 18418, 18422, 18434, 18444,
-  18468, 18469, 18470, 18483, 18495, 18496, 18498.
+  18333, 18346, 18397, 18409, 18410, 18412, 18418, 18422, 18434, 18435,
+  18444, 18468, 18469, 18470, 18483, 18495, 18496, 18498.
 
 * Cache information can be queried via sysconf() function on s390 e.g. with
   _SC_LEVEL1_ICACHE_SIZE as argument.
diff --git a/nptl/Makefile b/nptl/Makefile
index d58324d..00aedae 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -210,6 +210,7 @@ CFLAGS-recvfrom.c = -fexceptions -fasynchronous-unwind-tables
 
 CFLAGS-pt-system.c = -fexceptions
 
+LDLIBS-tst-once5 = -lstdc++
 
 tests = tst-typesizes \
 	tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
@@ -232,7 +233,7 @@ tests = tst-typesizes \
 	tst-rwlock5 tst-rwlock6 tst-rwlock7 tst-rwlock8 tst-rwlock9 \
 	tst-rwlock10 tst-rwlock11 tst-rwlock12 tst-rwlock13 tst-rwlock14 \
 	tst-rwlock15 tst-rwlock16 \
-	tst-once1 tst-once2 tst-once3 tst-once4 \
+	tst-once1 tst-once2 tst-once3 tst-once4 tst-once5 \
 	tst-key1 tst-key2 tst-key3 tst-key4 \
 	tst-sem1 tst-sem2 tst-sem3 tst-sem4 tst-sem5 tst-sem6 tst-sem7 \
 	tst-sem8 tst-sem9 tst-sem10 tst-sem11 tst-sem12 tst-sem13 tst-sem14 \
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 84a7105..72d3e23 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -536,16 +536,9 @@ extern void __librt_disable_asynccancel (int oldtype)
 extern void __pthread_cleanup_push (struct _pthread_cleanup_buffer *buffer,
 				    void (*routine) (void *), void *arg)
      attribute_hidden;
-# undef pthread_cleanup_push
-# define pthread_cleanup_push(routine,arg) \
-  { struct _pthread_cleanup_buffer _buffer;				      \
-    __pthread_cleanup_push (&_buffer, (routine), (arg));
 
 extern void __pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer,
 				   int execute) attribute_hidden;
-# undef pthread_cleanup_pop
-# define pthread_cleanup_pop(execute) \
-    __pthread_cleanup_pop (&_buffer, (execute)); }
 #endif
 
 extern void __pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer,
diff --git a/nptl/tst-once5.cc b/nptl/tst-once5.cc
new file mode 100644
index 0000000..60bc78a
--- /dev/null
+++ b/nptl/tst-once5.cc
@@ -0,0 +1,80 @@
+/* Copyright (C) 2015 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Ulrich Drepper <drepper@redhat.com>, 2002.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <string.h>
+
+
+static pthread_once_t once = PTHREAD_ONCE_INIT;
+
+// Exception type thrown from the pthread_once init routine.
+struct OnceException { };
+
+// Test iteration counter.
+static int niter;
+
+static void
+init_routine (void)
+{
+  if (niter < 2)
+    throw OnceException ();
+}
+
+// Verify that an exception thrown from the pthread_once init routine
+// is propagated to the pthread_once caller and that the function can
+// be subsequently invoked to attempt the initialization again.
+static int
+do_test (void)
+{
+  int result = 1;
+
+  // Repeat three times, having the init routine throw the first two
+  // times and succeed on the final attempt.
+  for (niter = 0; niter != 3; ++niter) {
+
+    try {
+      int rc = pthread_once (&once, init_routine);
+      if (rc)
+        fprintf (stderr, "pthread_once failed: %i (%s)\n",
+                 rc, strerror (rc));
+
+      if (niter < 2)
+        fputs ("pthread_once unexpectedly returned without"
+               " throwing an exception", stderr);
+    }
+    catch (OnceException) {
+      if (1 < niter)
+        fputs ("pthread_once unexpectedly threw", stderr);
+      result = 0;
+    }
+    catch (...) {
+      fputs ("pthread_once threw an unknown exception", stderr);
+    }
+
+    // Abort the test on the first failure.
+    if (result)
+      break;
+  }
+
+  return result;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"

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

[Martin Sebor]

On 06/09/2015 01:43 PM, Martin Sebor wrote:
> Attached is an updated version of the patch that addresses
> the LDFLAGS -> LDLIBS comment. Retested on ppc64.
>
> Is it okay to commit?

Are there any outstanding concerns with this version of
the patch or is it good to commit?

Martin

>
> Martin
>
> On 05/31/2015 03: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.  */
>>
>> Martin
>

[PING 2] Re: [PATCH] pthread_once hangs when init routine throws an exception [BZ #18435]

[Martin Sebor]

Are there any concerns with this patch?

   https://sourceware.org/ml/libc-alpha/2015-06/msg00348.html

Martin

On 06/15/2015 01:23 PM, Martin Sebor wrote:
> On 06/09/2015 01:43 PM, Martin Sebor wrote:
>> Attached is an updated version of the patch that addresses
>> the LDFLAGS -> LDLIBS comment. Retested on ppc64.
>>
>> Is it okay to commit?
>
> Are there any outstanding concerns with this version of
> the patch or is it good to commit?
>
> Martin
>
>>
>> Martin
>>
>> On 05/31/2015 03: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.  */
>>>
>>> Martin
>>
>

Re: [PING 3] Re: [PATCH] pthread_once hangs when init routine throws an exception [BZ #18435]

[Rich Felker]

On Tue, Jun 30, 2015 at 05:25:31PM -0600, Martin Sebor wrote:
> Are there any unresolved concerns with or objections to this
> patch?
> 
>   https://sourceware.org/ml/libc-alpha/2015-06/msg00348.html

I still object to enshrining this usage as a supported feature, but I
don't object to the patch. In particular I think GCC should fix its
invalid usage of pthread_once to implement the 'equivalent' C++
threads primitive.

Rich

Re: [PING 3] Re: [PATCH] pthread_once hangs when init routine throws an exception [BZ #18435]

[Martin Sebor]

On 06/30/2015 06:06 PM, Rich Felker wrote:
> On Tue, Jun 30, 2015 at 05:25:31PM -0600, Martin Sebor wrote:
>> Are there any unresolved concerns with or objections to this
>> patch?
>>
>>    https://sourceware.org/ml/libc-alpha/2015-06/msg00348.html
>
> I still object to enshrining this usage as a supported feature, but I
> don't object to the patch. In particular I think GCC should fix its
> invalid usage of pthread_once to implement the 'equivalent' C++
> threads primitive.

Thank you. The patch has been committed.

Martin

Re: [PING 3] Re: [PATCH] pthread_once hangs when init routine throws an exception [BZ #18435]

[Joseph Myers]

On Wed, 1 Jul 2015, Martin Sebor wrote:

> On 06/30/2015 06:06 PM, Rich Felker wrote:
> > On Tue, Jun 30, 2015 at 05:25:31PM -0600, Martin Sebor wrote:
> > > Are there any unresolved concerns with or objections to this
> > > patch?
> > > 
> > >    https://sourceware.org/ml/libc-alpha/2015-06/msg00348.html
> > 
> > I still object to enshrining this usage as a supported feature, but I
> > don't object to the patch. In particular I think GCC should fix its
> > invalid usage of pthread_once to implement the 'equivalent' C++
> > threads primitive.
> 
> Thank you. The patch has been committed.

If the patch fully fixes the bug, please close it as fixed.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Szabolcs Nagy]

On 09/06/15 20:43, Martin Sebor wrote:
> Attached is an updated version of the patch that addresses
> the LDFLAGS -> LDLIBS comment. Retested on ppc64.
> 
> Is it okay to commit?
> 
> Martin
> 
> On 05/31/2015 03: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.
...
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 84a7105..72d3e23 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -536,16 +536,9 @@ extern void __librt_disable_asynccancel (int oldtype)
>  extern void __pthread_cleanup_push (struct _pthread_cleanup_buffer *buffer,
>  				    void (*routine) (void *), void *arg)
>       attribute_hidden;
> -# undef pthread_cleanup_push
> -# define pthread_cleanup_push(routine,arg) \
> -  { struct _pthread_cleanup_buffer _buffer;				      \
> -    __pthread_cleanup_push (&_buffer, (routine), (arg));
>  
>  extern void __pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer,
>  				   int execute) attribute_hidden;
> -# undef pthread_cleanup_pop
> -# define pthread_cleanup_pop(execute) \
> -    __pthread_cleanup_pop (&_buffer, (execute)); }
>  #endif
>  
>  extern void __pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer,

this broke

nptl/tst-join5
nptl/tst-once3

tests on aarch64.

the cleanup handler of the pthread_once and pthread_join
implementation don't run when they are canceled.

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

[Martin Sebor]

> this broke
>
> nptl/tst-join5
> nptl/tst-once3
>
> tests on aarch64.
>
> the cleanup handler of the pthread_once and pthread_join
> implementation don't run when they are canceled.

I'll look into it as soon as I get access to an aarch64 machine.

Martin

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

[Adhemerval Zanella]

On 06-07-2015 11:16, Martin Sebor wrote:
>> this broke
>>
>> nptl/tst-join5
>> nptl/tst-once3
>>
>> tests on aarch64.
>>
>> the cleanup handler of the pthread_once and pthread_join
>> implementation don't run when they are canceled.
> 
> I'll look into it as soon as I get access to an aarch64 machine.
> 
> Martin
> 

And I see a regression with

nptl/tst-once3

for armhf.

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

[Szabolcs Nagy]

On 06/07/15 15:58, Adhemerval Zanella wrote:
> On 06-07-2015 11:16, Martin Sebor wrote:
>>> this broke
>>>
>>> nptl/tst-join5
>>> nptl/tst-once3
>>>
>>> tests on aarch64.
>>>
>>> the cleanup handler of the pthread_once and pthread_join
>>> implementation don't run when they are canceled.
>>
>> I'll look into it as soon as I get access to an aarch64 machine.
>>
>> Martin
>>
> 
> And I see a regression with
> 
> nptl/tst-once3
> 
> for armhf.
> 

in case of aarch64 the bug is somewhere in __pthread_unwind
(called from __do_cancel) so probably a libgcc issue.

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

[Szabolcs Nagy]

On 06/07/15 17:33, Szabolcs Nagy wrote:
> On 06/07/15 15:58, Adhemerval Zanella wrote:
>> On 06-07-2015 11:16, Martin Sebor wrote:
>>>> this broke
>>>>
>>>> nptl/tst-join5
>>>> nptl/tst-once3
>>>>
>>>> tests on aarch64.
>>>>
>>>> the cleanup handler of the pthread_once and pthread_join
>>>> implementation don't run when they are canceled.
>>>
>>> I'll look into it as soon as I get access to an aarch64 machine.
>>>
>>> Martin
>>>
>>
>> And I see a regression with
>>
>> nptl/tst-once3
>>
>> for armhf.
>>
> 
> in case of aarch64 the bug is somewhere in __pthread_unwind
> (called from __do_cancel) so probably a libgcc issue.
> 

the problem seems to be that gcc on x86_64 turns on
-fasynchronous-unwind-tables by default, but not on
aarch64 or arm.

now i added -fasynchronous-unwind-tables to the cflags
of the relevant tests, will send a patch if they pass.

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

[Szabolcs Nagy]

On 06/07/15 18:08, Szabolcs Nagy wrote:
> On 06/07/15 17:33, Szabolcs Nagy wrote:
>> On 06/07/15 15:58, Adhemerval Zanella wrote:
>>> On 06-07-2015 11:16, Martin Sebor wrote:
>>>>> this broke
>>>>>
>>>>> nptl/tst-join5
>>>>> nptl/tst-once3
>>>>>
>>>>> tests on aarch64.
>>>>>
>>>>> the cleanup handler of the pthread_once and pthread_join
>>>>> implementation don't run when they are canceled.
>>>>
>>>> I'll look into it as soon as I get access to an aarch64 machine.
>>>>
>>>> Martin
>>>>
>>>
>>> And I see a regression with
>>>
>>> nptl/tst-once3
>>>
>>> for armhf.
>>>
>>
>> in case of aarch64 the bug is somewhere in __pthread_unwind
>> (called from __do_cancel) so probably a libgcc issue.
>>
> 
> the problem seems to be that gcc on x86_64 turns on
> -fasynchronous-unwind-tables by default, but not on
> aarch64 or arm.
> 
> now i added -fasynchronous-unwind-tables to the cflags
> of the relevant tests, will send a patch if they pass.
> 

This uncovered a serious issue that affects other archs too.

Both test failures are caused by glibc switching the internal
mechanism of pthread cancellation clean up handling to use
__attribute__((cleanup(f))) and -fexceptions, but the two test
failures are independent:

(1) Should -fasynchronous-unwind-tables be on by default in gcc?

nptl/tst-once3 fails because the callback passed to pthread_once
now has to be compiled with -fasynchronous-unwind-tables which
is not on by default on arm and aarch64 gcc.  So does glibc
expect the users to use this flag correctly or does glibc
requires the compiler to have it on by default?

(My understanding: posix conforming c code cannot observe the
presence of -fasynchronous-unwind-tables without invoking UB, but
the glibc implementation of cancellation cleanup and backtrace
from signal handlers makes this detail observable.  Any function
which may be canceled needs this flag to make cleanup work, so
glibc seems to impose this as a requirement on the compiler: the
user may not be in control of all the code that may be canceled).


(2) Should gcc support exceptions from async signal handlers?

nptl/tst-join5 failure is more problematic: it fails because gcc
does not seem to implement -fexceptions with the assumption that
signal handlers can throw, in particular it assumes inline asm
does not throw exceptions.  If the syscall that is a cancellation
point appears between pthread_cleanup_push and pthread_cleanup_pop
in glibc internal code, the cleanup handler may not get run on
cancellation depending on where gcc moved the syscall inline asm.
(It is free to move it outside the code range that is marked for
exception handling, this is what happens on aarch64 in pthread_join).
This affects all archs, but some may get lucky.

(My understanding: gcc must be very strict about how it marks
the code range for exception handling and assume any instruction
may throw if it wants -fexceptions -fasynchronous-unwind-tables to
work from signal handlers.  Current compilers do not seem to support
this so glibc internal code should not rely on it, which means the
cancellation mechanism should not rely on exception handling at
least not when the exception is thrown from the cancel signal
handler.  I think the gnu toolchain should not try to make pthread
cancellation to interoperate with C++ exceptions nor to make
exceptions work from signal handlers: no standard requires this
behaviour and seems to cause problems).


Both issues cause silent omission of cleanup handlers running
on cancellation, leaving libc internal state inconsistent.

The second issue may be worth discussing on the gcc list.

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

[Carlos O'Donell]

On 07/08/2015 07:00 AM, Szabolcs Nagy wrote:
> On 06/07/15 18:08, Szabolcs Nagy wrote:
>> On 06/07/15 17:33, Szabolcs Nagy wrote:
>>> On 06/07/15 15:58, Adhemerval Zanella wrote:
>>>> On 06-07-2015 11:16, Martin Sebor wrote:
>>>>>> this broke
>>>>>>
>>>>>> nptl/tst-join5
>>>>>> nptl/tst-once3
>>>>>>
>>>>>> tests on aarch64.
>>>>>>
>>>>>> the cleanup handler of the pthread_once and pthread_join
>>>>>> implementation don't run when they are canceled.
>>>>>
>>>>> I'll look into it as soon as I get access to an aarch64 machine.
>>>>>
>>>>> Martin
>>>>>
>>>>
>>>> And I see a regression with
>>>>
>>>> nptl/tst-once3
>>>>
>>>> for armhf.
>>>>
>>>
>>> in case of aarch64 the bug is somewhere in __pthread_unwind
>>> (called from __do_cancel) so probably a libgcc issue.
>>>
>>
>> the problem seems to be that gcc on x86_64 turns on
>> -fasynchronous-unwind-tables by default, but not on
>> aarch64 or arm.
>>
>> now i added -fasynchronous-unwind-tables to the cflags
>> of the relevant tests, will send a patch if they pass.
>>
> 
> This uncovered a serious issue that affects other archs too.

Thanks.

> Both test failures are caused by glibc switching the internal
> mechanism of pthread cancellation clean up handling to use
> __attribute__((cleanup(f))) and -fexceptions, but the two test
> failures are independent:
> 
> (1) Should -fasynchronous-unwind-tables be on by default in gcc?
> 
> nptl/tst-once3 fails because the callback passed to pthread_once
> now has to be compiled with -fasynchronous-unwind-tables which
> is not on by default on arm and aarch64 gcc.  So does glibc
> expect the users to use this flag correctly or does glibc
> requires the compiler to have it on by default?

This is bad.

> (My understanding: posix conforming c code cannot observe the
> presence of -fasynchronous-unwind-tables without invoking UB, but
> the glibc implementation of cancellation cleanup and backtrace
> from signal handlers makes this detail observable.  Any function
> which may be canceled needs this flag to make cleanup work, so
> glibc seems to impose this as a requirement on the compiler: the
> user may not be in control of all the code that may be canceled).
 
We already impose the requirement that all such called code be
cancel safe anyway and it might not be unless all called code
uses cancel handlers to cleanup during cancellation. This would
be another requirement that imposes -fasynchronous-unwind-tables
on cancellation users. However, this is a new requirement and
old code can't be fixed, and thus we have problem that requires
versioning and documentation. All for the purposes of implementing
C++ std::call_once via pthread_once, which seems like is going
to be problematic.
 
> (2) Should gcc support exceptions from async signal handlers?

No. I don't think you can support it safely.

> nptl/tst-join5 failure is more problematic: it fails because gcc
> does not seem to implement -fexceptions with the assumption that
> signal handlers can throw, in particular it assumes inline asm
> does not throw exceptions.  If the syscall that is a cancellation
> point appears between pthread_cleanup_push and pthread_cleanup_pop
> in glibc internal code, the cleanup handler may not get run on
> cancellation depending on where gcc moved the syscall inline asm.
> (It is free to move it outside the code range that is marked for
> exception handling, this is what happens on aarch64 in pthread_join).
> This affects all archs, but some may get lucky.

Ah! That's truly a terrible scenario.

> (My understanding: gcc must be very strict about how it marks
> the code range for exception handling and assume any instruction
> may throw if it wants -fexceptions -fasynchronous-unwind-tables to
> work from signal handlers.  Current compilers do not seem to support
> this so glibc internal code should not rely on it, which means the
> cancellation mechanism should not rely on exception handling at
> least not when the exception is thrown from the cancel signal
> handler.  I think the gnu toolchain should not try to make pthread
> cancellation to interoperate with C++ exceptions nor to make
> exceptions work from signal handlers: no standard requires this
> behaviour and seems to cause problems).

No, we just need to revert this patch and have C++ implement
std::call_once by itself.
 
> Both issues cause silent omission of cleanup handlers running
> on cancellation, leaving libc internal state inconsistent.
> 
> The second issue may be worth discussing on the gcc list.
> 

Cheers,
Carlos.

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

[Torvald Riegel]

On Wed, 2015-07-08 at 12:09 -0400, Carlos O'Donell wrote:
> On 07/08/2015 07:00 AM, Szabolcs Nagy wrote:
> > (2) Should gcc support exceptions from async signal handlers?
> 
> No. I don't think you can support it safely.
> 
> > nptl/tst-join5 failure is more problematic: it fails because gcc
> > does not seem to implement -fexceptions with the assumption that
> > signal handlers can throw, in particular it assumes inline asm
> > does not throw exceptions.  If the syscall that is a cancellation
> > point appears between pthread_cleanup_push and pthread_cleanup_pop
> > in glibc internal code, the cleanup handler may not get run on
> > cancellation depending on where gcc moved the syscall inline asm.
> > (It is free to move it outside the code range that is marked for
> > exception handling, this is what happens on aarch64 in pthread_join).
> > This affects all archs, but some may get lucky.
> 
> Ah! That's truly a terrible scenario.
> 
> > (My understanding: gcc must be very strict about how it marks
> > the code range for exception handling and assume any instruction
> > may throw if it wants -fexceptions -fasynchronous-unwind-tables to
> > work from signal handlers.  Current compilers do not seem to support
> > this so glibc internal code should not rely on it, which means the
> > cancellation mechanism should not rely on exception handling at
> > least not when the exception is thrown from the cancel signal
> > handler.  I think the gnu toolchain should not try to make pthread
> > cancellation to interoperate with C++ exceptions nor to make
> > exceptions work from signal handlers: no standard requires this
> > behaviour and seems to cause problems).
> 
> No, we just need to revert this patch and have C++ implement
> std::call_once by itself.

Would point (2) be taken care of by Adhemerval's cancellation changes?

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

[Szabolcs Nagy]

On 08/07/15 17:33, Torvald Riegel wrote:
> On Wed, 2015-07-08 at 12:09 -0400, Carlos O'Donell wrote:
>> On 07/08/2015 07:00 AM, Szabolcs Nagy wrote:
>>> (2) Should gcc support exceptions from async signal handlers?
>>
>> No. I don't think you can support it safely.
>>
>>> nptl/tst-join5 failure is more problematic: it fails because gcc
>>> does not seem to implement -fexceptions with the assumption that
>>> signal handlers can throw, in particular it assumes inline asm
>>> does not throw exceptions.  If the syscall that is a cancellation
>>> point appears between pthread_cleanup_push and pthread_cleanup_pop
>>> in glibc internal code, the cleanup handler may not get run on
>>> cancellation depending on where gcc moved the syscall inline asm.
>>> (It is free to move it outside the code range that is marked for
>>> exception handling, this is what happens on aarch64 in pthread_join).
>>> This affects all archs, but some may get lucky.
>>
>> Ah! That's truly a terrible scenario.
>>
>>> (My understanding: gcc must be very strict about how it marks
>>> the code range for exception handling and assume any instruction
>>> may throw if it wants -fexceptions -fasynchronous-unwind-tables to
>>> work from signal handlers.  Current compilers do not seem to support
>>> this so glibc internal code should not rely on it, which means the
>>> cancellation mechanism should not rely on exception handling at
>>> least not when the exception is thrown from the cancel signal
>>> handler.  I think the gnu toolchain should not try to make pthread
>>> cancellation to interoperate with C++ exceptions nor to make
>>> exceptions work from signal handlers: no standard requires this
>>> behaviour and seems to cause problems).
>>
>> No, we just need to revert this patch and have C++ implement
>> std::call_once by itself.
> 
> Would point (2) be taken care of by Adhemerval's cancellation changes?
> 

yes: if the cancel point syscall is not inline asm,
but extern call (that is not marked with nothrow)
then gcc -fexceptions should handle it correctly.

asynchronous cancellation is still problematic,
but that is a special case.

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

[Carlos O'Donell]

On 07/08/2015 12:52 PM, Szabolcs Nagy wrote:
> On 08/07/15 17:33, Torvald Riegel wrote:
>> On Wed, 2015-07-08 at 12:09 -0400, Carlos O'Donell wrote:
>>> On 07/08/2015 07:00 AM, Szabolcs Nagy wrote:
>>>> (2) Should gcc support exceptions from async signal handlers?
>>>
>>> No. I don't think you can support it safely.
>>>
>>>> nptl/tst-join5 failure is more problematic: it fails because gcc
>>>> does not seem to implement -fexceptions with the assumption that
>>>> signal handlers can throw, in particular it assumes inline asm
>>>> does not throw exceptions.  If the syscall that is a cancellation
>>>> point appears between pthread_cleanup_push and pthread_cleanup_pop
>>>> in glibc internal code, the cleanup handler may not get run on
>>>> cancellation depending on where gcc moved the syscall inline asm.
>>>> (It is free to move it outside the code range that is marked for
>>>> exception handling, this is what happens on aarch64 in pthread_join).
>>>> This affects all archs, but some may get lucky.
>>>
>>> Ah! That's truly a terrible scenario.
>>>
>>>> (My understanding: gcc must be very strict about how it marks
>>>> the code range for exception handling and assume any instruction
>>>> may throw if it wants -fexceptions -fasynchronous-unwind-tables to
>>>> work from signal handlers.  Current compilers do not seem to support
>>>> this so glibc internal code should not rely on it, which means the
>>>> cancellation mechanism should not rely on exception handling at
>>>> least not when the exception is thrown from the cancel signal
>>>> handler.  I think the gnu toolchain should not try to make pthread
>>>> cancellation to interoperate with C++ exceptions nor to make
>>>> exceptions work from signal handlers: no standard requires this
>>>> behaviour and seems to cause problems).
>>>
>>> No, we just need to revert this patch and have C++ implement
>>> std::call_once by itself.
>>
>> Would point (2) be taken care of by Adhemerval's cancellation changes?
>>
> 
> yes: if the cancel point syscall is not inline asm,
> but extern call (that is not marked with nothrow)
> then gcc -fexceptions should handle it correctly.
> 
> asynchronous cancellation is still problematic,
> but that is a special case.

And we still have to support that case which makes this change
a net loss of functionality. Therefore I think we need to revert
this and try again 2.23.

Cheers,
Carlos.

 

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

[Jakub Jelinek]

On Wed, Jul 08, 2015 at 12:09:49PM -0400, Carlos O'Donell wrote:
> > (My understanding: gcc must be very strict about how it marks
> > the code range for exception handling and assume any instruction
> > may throw if it wants -fexceptions -fasynchronous-unwind-tables to
> > work from signal handlers.  Current compilers do not seem to support
> > this so glibc internal code should not rely on it, which means the
> > cancellation mechanism should not rely on exception handling at
> > least not when the exception is thrown from the cancel signal
> > handler.  I think the gnu toolchain should not try to make pthread
> > cancellation to interoperate with C++ exceptions nor to make
> > exceptions work from signal handlers: no standard requires this
> > behaviour and seems to cause problems).
> 
> No, we just need to revert this patch and have C++ implement
> std::call_once by itself.

As has been found, it is unfortunately impossible for C++ to implement it by
itself without breaking ABI.
What libstdc++ needs is either that a modified version of Martin's patch
is committed again, or a new entry-point which is ABI compatible with
pthread_once but ensures the exception behavior is added (or both,
make pthread_once work with exceptions and add an alias to pthread_once
that guarantees that behavior).

The only bug I see in Martin's patch is that he has removed the overrides
altogether.
sysdeps/generic/nptl/pthread.h
defines 3 different implementations of pthread_cleanup_{push,pop}:
1) C++ -fexceptions (using dtor)
2) C -fexceptions (using cleanup attribute)
3) -fno-exceptions one
The override in pthreadP.h which is used only if IS_IN_libpthread is
meant as a better replacement for 3), but isn't a good replacement for 2).

The overrides in pthreadP.h are only used by pthread_once.c, sem_*wait.c and
pthread_join_common.c.
Seems currently everything but pthread_join_common.c is compiled with
-fexceptions -fasynchronous-unwind-tables, so guarding the pthreadP.h
2 #defines (but probably not the function declarations) with
#if !defined(__GNU__) || !defined(__EXCEPTIONS)
seems the way to go to me.

And, perhaps incrementally consider compiling pthread_join_common.c
with -fexceptions -fasynchronous-unwind-tables too.
The pthread_once.c stuff is most important, sure, but if my fuzzy memory
serves me a little, I think for pthread_cancel once it hits a first
non-dtor/cleanup registered cleanup it will just handle the
3) method registered ones and not others, while if it uses the 1)/2)
cleanups, it will continue doing that until it reaches some 3) registered
ones if any.

	Jakub

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

[Carlos O'Donell]

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.

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

[Martin Sebor]

> 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

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

[Szabolcs Nagy]

* 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

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

[Martin Sebor]

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

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

[Szabolcs Nagy]

* 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).

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

[Martin Sebor]

> 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.

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

[Martin Sebor]

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(-)

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

[Jakub Jelinek]

This is another attempt at making pthread_once handle throwing exceptions
from the init routine callback.  As the new testcases show, just switching
to the cleanup attribute based cleanup does fix the tst-once5 test, but
breaks the new tst-oncey3 test.  That is because when throwing exceptions,
only the unwind info registered cleanups (i.e. C++ destructors or cleanup
attribute), when cancelling threads and there has been unwind info from the
cancellation point up to whatever needs cleanup both unwind info registered
cleanups and THREAD_SETMEM (self, cleanup, ...) registered cleanups are
invoked, but once we hit some frame with no unwind info, only the
THREAD_SETMEM (self, cleanup, ...) registered cleanups are invoked.
So, to stay fully backwards compatible (allow init routines without
unwind info which encounter cancellation points) and handle exception throwing
we actually need to register the pthread_once cleanups in both unwind info
and in the THREAD_SETMEM (self, cleanup, ...) way.
If an exception is thrown, only the former will happen and we in that case
need to also unregister the THREAD_SETMEM (self, cleanup, ...) registered
handler, because otherwise after catching the exception the user code could
call deeper into the stack some cancellation point, get cancelled and then
a stale cleanup handler would clobber stack and probably crash.
If a thread calling init routine is cancelled and unwind info ends before
the pthread_once frame, it will be cleaned up through self->cleanup as
before.  And if unwind info is present, unwind_stop first calls the
self->cleanup registered handler for the frame, then it will call the
unwind info registered handler but that will already see __do_it == 0
and do nothing.

diff --git a/nptl/Makefile b/nptl/Makefile
index 5f85dd7854..33766eaf7a 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -386,10 +386,6 @@ xtests += tst-eintr1
 
 test-srcs = tst-oddstacklimit
 
-# Test expected to fail on most targets (except x86_64) due to bug
-# 18435 - pthread_once hangs when init routine throws an exception.
-test-xfail-tst-once5 = yes
-
 gen-as-const-headers = unwindbuf.sym \
 		       pthread-pi-defines.sym
 
diff --git a/nptl/cleanup_compat.c b/nptl/cleanup_compat.c
index fec88c2f86..b5b848338a 100644
--- a/nptl/cleanup_compat.c
+++ b/nptl/cleanup_compat.c
@@ -48,3 +48,11 @@ _pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer, int execute)
     buffer->__routine (buffer->__arg);
 }
 strong_alias (_pthread_cleanup_pop, __pthread_cleanup_pop)
+
+void
+__pthread_cleanup_cleanup (struct _pthread_cleanup_buffer *buffer)
+{
+  struct pthread *self = THREAD_SELF;
+  if (THREAD_GETMEM (self, cleanup) == buffer)
+    THREAD_SETMEM (self, cleanup, buffer->__prev);
+}
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index d2fd0826fe..297426be1e 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -604,6 +604,67 @@ extern void __pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer,
 # undef pthread_cleanup_pop
 # define pthread_cleanup_pop(execute)                   \
   __pthread_cleanup_pop (&_buffer, (execute)); }
+
+extern void __pthread_cleanup_cleanup (struct _pthread_cleanup_buffer *buffer)
+     attribute_hidden;
+
+# if defined __GNUC__ && defined __EXCEPTIONS && !defined __cplusplus
+/* Structure to hold the cleanup handler information.  */
+struct __pthread_cleanup_combined_frame
+{
+  void (*__cancel_routine) (void *);
+  void *__cancel_arg;
+  int __do_it;
+  struct _pthread_cleanup_buffer __buffer;
+};
+
+/* Special cleanup macros which register cleanup both using
+   __pthread_cleanup_{push,pop} and using cleanup attribute.  This is needed
+   for pthread_once, so that it supports both throwing exceptions from the
+   pthread_once callback (only cleanup attribute works there) and cancellation
+   of the thread running the callback if the callback or some routines it
+   calls don't have unwind information.  */
+
+static inline void
+__pthread_cleanup_combined_routine (struct __pthread_cleanup_combined_frame
+				    *__frame)
+{
+  if (__frame->__do_it)
+    {
+      __frame->__cancel_routine (__frame->__cancel_arg);
+      __frame->__do_it = 0;
+      __pthread_cleanup_cleanup (&__frame->__buffer);
+    }
+}
+
+static inline void
+__pthread_cleanup_combined_routine_voidptr (void *__arg)
+{
+  struct __pthread_cleanup_combined_frame *__frame
+    = (struct __pthread_cleanup_combined_frame *) __arg;
+  if (__frame->__do_it)
+    {
+      __frame->__cancel_routine (__frame->__cancel_arg);
+      __frame->__do_it = 0;
+    }
+}
+
+#  define pthread_cleanup_combined_push(routine, arg) \
+  do {									      \
+    struct __pthread_cleanup_combined_frame __clframe			      \
+      __attribute__ ((__cleanup__ (__pthread_cleanup_combined_routine)))      \
+      = { .__cancel_routine = (routine), .__cancel_arg = (arg),	 	      \
+	  .__do_it = 1 };						      \
+    __pthread_cleanup_push (&__clframe.__buffer,			      \
+			    __pthread_cleanup_combined_routine_voidptr,	      \
+			    &__clframe);
+
+#  define pthread_cleanup_combined_pop(execute) \
+    __pthread_cleanup_pop (&__clframe.__buffer, 0);			      \
+    __clframe.__do_it = (execute);					      \
+  } while (0)
+
+# endif
 #endif
 
 extern void __pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer,
diff --git a/nptl/pthread_once.c b/nptl/pthread_once.c
index 28d97097c7..7645da222a 100644
--- a/nptl/pthread_once.c
+++ b/nptl/pthread_once.c
@@ -111,11 +111,11 @@ __pthread_once_slow (pthread_once_t *once_control, void (*init_routine) (void))
       /* This thread is the first here.  Do the initialization.
 	 Register a cleanup handler so that in case the thread gets
 	 interrupted the initialization can be restarted.  */
-      pthread_cleanup_push (clear_once_control, once_control);
+      pthread_cleanup_combined_push (clear_once_control, once_control);
 
       init_routine ();
 
-      pthread_cleanup_pop (0);
+      pthread_cleanup_combined_pop (0);
 
 
       /* Mark *once_control as having finished the initialization.  We need
diff --git a/nptl/tst-once5.cc b/nptl/tst-once5.cc
index b797ab3562..60fe1ef820 100644
--- a/nptl/tst-once5.cc
+++ b/nptl/tst-once5.cc
@@ -59,7 +59,7 @@ do_test (void)
                " throwing an exception", stderr);
     }
     catch (OnceException) {
-      if (1 < niter)
+      if (niter > 1)
         fputs ("pthread_once unexpectedly threw", stderr);
       result = 0;
     }
@@ -75,7 +75,5 @@ do_test (void)
   return result;
 }
 
-// The test currently hangs and is XFAILed.  Reduce the timeout.
-#define TIMEOUT 1
 #define TEST_FUNCTION do_test ()
 #include "../test-skeleton.c"
diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index eeb64f9fb0..53b65ef349 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -231,7 +231,7 @@ generated += $(objpfx)tst-atfork2.mtrace \
 
 tests-internal += tst-cancel25 tst-robust8
 
-tests += tst-oncex3 tst-oncex4
+tests += tst-oncex3 tst-oncex4 tst-oncey3 tst-oncey4
 
 modules-names += tst-join7mod
 
@@ -242,6 +242,8 @@ endif
 
 CFLAGS-tst-oncex3.c += -fexceptions
 CFLAGS-tst-oncex4.c += -fexceptions
+CFLAGS-tst-oncey3.c += -fno-exceptions -fno-asynchronous-unwind-tables
+CFLAGS-tst-oncey4.c += -fno-exceptions -fno-asynchronous-unwind-tables
 
 $(objpfx)tst-join7: $(libdl) $(shared-thread-library)
 $(objpfx)tst-join7.out: $(objpfx)tst-join7mod.so

	Jakub

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

[Florian Weimer]

* Jakub Jelinek via Libc-alpha:

> This is another attempt at making pthread_once handle throwing exceptions
> from the init routine callback.  As the new testcases show, just switching
> to the cleanup attribute based cleanup does fix the tst-once5 test, but
> breaks the new tst-oncey3 test.  That is because when throwing exceptions,
> only the unwind info registered cleanups (i.e. C++ destructors or cleanup
> attribute), when cancelling threads and there has been unwind info from the
> cancellation point up to whatever needs cleanup both unwind info registered
> cleanups and THREAD_SETMEM (self, cleanup, ...) registered cleanups are
> invoked, but once we hit some frame with no unwind info, only the
> THREAD_SETMEM (self, cleanup, ...) registered cleanups are invoked.
> So, to stay fully backwards compatible (allow init routines without
> unwind info which encounter cancellation points) and handle exception throwing
> we actually need to register the pthread_once cleanups in both unwind info
> and in the THREAD_SETMEM (self, cleanup, ...) way.
> If an exception is thrown, only the former will happen and we in that case
> need to also unregister the THREAD_SETMEM (self, cleanup, ...) registered
> handler, because otherwise after catching the exception the user code could
> call deeper into the stack some cancellation point, get cancelled and then
> a stale cleanup handler would clobber stack and probably crash.
> If a thread calling init routine is cancelled and unwind info ends before
> the pthread_once frame, it will be cleaned up through self->cleanup as
> before.  And if unwind info is present, unwind_stop first calls the
> self->cleanup registered handler for the frame, then it will call the
> unwind info registered handler but that will already see __do_it == 0
> and do nothing.

I agree that this solves the issue.

I initially thought that we had to use setjmp-based registration
approach, but that does not seem to be the case.

Any idea why we have the setjmp-based approach (for -fno-exceptions) for
external use, and not use the legacy approach internal to glibc?  One
difference is that it restores the value of callee-saved registers,
which could matter if we switch between two kinds of unwinding (callback
registration and DWARF).

Do you think this approach should used for all callback-based functions,
not just pthread_once?

> diff --git a/nptl/cleanup_compat.c b/nptl/cleanup_compat.c
> index fec88c2f86..b5b848338a 100644
> --- a/nptl/cleanup_compat.c
> +++ b/nptl/cleanup_compat.c
> @@ -48,3 +48,11 @@ _pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer, int execute)
>      buffer->__routine (buffer->__arg);
>  }
>  strong_alias (_pthread_cleanup_pop, __pthread_cleanup_pop)
> +
> +void
> +__pthread_cleanup_cleanup (struct _pthread_cleanup_buffer *buffer)
> +{
> +  struct pthread *self = THREAD_SELF;
> +  if (THREAD_GETMEM (self, cleanup) == buffer)
> +    THREAD_SETMEM (self, cleanup, buffer->__prev);
> +}

It's surprising that the cleanup field update is conditional.  How does
that happen?  Is it really safe?

> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index d2fd0826fe..297426be1e 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -604,6 +604,67 @@ extern void __pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer,

> +# if defined __GNUC__ && defined __EXCEPTIONS && !defined __cplusplus
> +/* Structure to hold the cleanup handler information.  */
> +struct __pthread_cleanup_combined_frame
> +{
> +  void (*__cancel_routine) (void *);
> +  void *__cancel_arg;
> +  int __do_it;
> +  struct _pthread_cleanup_buffer __buffer;
> +};

No __GNUC__ is necessary in internal headers.

> +/* Special cleanup macros which register cleanup both using
> +   __pthread_cleanup_{push,pop} and using cleanup attribute.  This is needed
> +   for pthread_once, so that it supports both throwing exceptions from the
> +   pthread_once callback (only cleanup attribute works there) and cancellation
> +   of the thread running the callback if the callback or some routines it
> +   calls don't have unwind information.  */
> +
> +static inline void
> +__pthread_cleanup_combined_routine (struct __pthread_cleanup_combined_frame
> +				    *__frame)
> +{
> +  if (__frame->__do_it)
> +    {
> +      __frame->__cancel_routine (__frame->__cancel_arg);
> +      __frame->__do_it = 0;
> +      __pthread_cleanup_cleanup (&__frame->__buffer);
> +    }
> +}

Is there a way to turn the call to __frame->__cancel_routine into a
direct call?  For more general use (outside pthread_once) that might be
desirable from a security hardening perspective.  I expect it will
happen if __pthread_cleanup_combined_routine_voidptr and
__pthread_cleanup_combined_routine do not use the sam variable.

I guess we can fix this later.  For pthread_once, the indirect call only
happens on the unwinding path.

> +static inline void
> +__pthread_cleanup_combined_routine_voidptr (void *__arg)
> +{
> +  struct __pthread_cleanup_combined_frame *__frame
> +    = (struct __pthread_cleanup_combined_frame *) __arg;
> +  if (__frame->__do_it)
> +    {
> +      __frame->__cancel_routine (__frame->__cancel_arg);
> +      __frame->__do_it = 0;
> +    }
> +}

I think you can make this an out-of-line routine because it can never be
inlined.  But given that this is called only once, it probably does not
matter.

> diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> index eeb64f9fb0..53b65ef349 100644
> --- a/sysdeps/pthread/Makefile
> +++ b/sysdeps/pthread/Makefile
> @@ -231,7 +231,7 @@ generated += $(objpfx)tst-atfork2.mtrace \
>  
>  tests-internal += tst-cancel25 tst-robust8
>  
> -tests += tst-oncex3 tst-oncex4
> +tests += tst-oncex3 tst-oncex4 tst-oncey3 tst-oncey4

tst-oncey3.c, tst-oncey4.c are missing, but I guessed their contents.

Thanks,
Florian

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

[Jakub Jelinek]

On Thu, Mar 04, 2021 at 12:50:54PM +0100, Florian Weimer wrote:
> I initially thought that we had to use setjmp-based registration
> approach, but that does not seem to be the case.
> 
> Any idea why we have the setjmp-based approach (for -fno-exceptions) for
> external use, and not use the legacy approach internal to glibc?  One
> difference is that it restores the value of callee-saved registers,
> which could matter if we switch between two kinds of unwinding (callback
> registration and DWARF).

I'm afraid I don't know, I thought the public pthread_cleanup_push/pop
for -fno-exceptions is the same as the pthreadP.h one except that it uses
_pthread_cleanup_* instead of __pthread_cleanup_*.
All I remember that the normal unwinding does a longjmp at the end after
processing all the self->cleanup registered cleanups, but I thought it is
just to the pthread_create start.

> Do you think this approach should used for all callback-based functions,
> not just pthread_once?

If we want to support throw from the callbacks that would do cleanups and
continue throwing outside of the libc routines and at the same time
support no unwind callbacks with cancellation points.
I guess it would need separate analysis of:
argp/Makefile:CFLAGS-argp-help.c += $(uses-callbacks) -fexceptions
argp/Makefile:CFLAGS-argp-parse.c += $(uses-callbacks)
dirent/Makefile:CFLAGS-scandir.c += $(uses-callbacks)
dirent/Makefile:CFLAGS-scandir64.c += $(uses-callbacks)
dirent/Makefile:CFLAGS-scandir-tail.c += $(uses-callbacks)
dirent/Makefile:CFLAGS-scandir64-tail.c += $(uses-callbacks)
elf/Makefile:CFLAGS-dl-iteratephdr.c += $(uses-callbacks)
io/Makefile:CFLAGS-fts.c += -Wno-uninitialized $(uses-callbacks) -fexceptions
io/Makefile:CFLAGS-fts64.c += -Wno-uninitialized $(uses-callbacks) -fexceptions
io/Makefile:CFLAGS-ftw.c += $(uses-callbacks) -fexceptions
io/Makefile:CFLAGS-ftw64.c += $(uses-callbacks) -fexceptions
malloc/Makefile:CFLAGS-obstack.c += $(uses-callbacks)
misc/Makefile:CFLAGS-tsearch.c += $(uses-callbacks)
misc/Makefile:CFLAGS-lsearch.c += $(uses-callbacks)
nptl/Makefile:CFLAGS-pthread_once.c += $(uses-callbacks) -fexceptions \
posix/Makefile:CFLAGS-glob.c += $(uses-callbacks) -fexceptions
posix/Makefile:CFLAGS-glob64.c += $(uses-callbacks) -fexceptions
stdlib/Makefile:CFLAGS-bsearch.c += $(uses-callbacks)
stdlib/Makefile:CFLAGS-msort.c += $(uses-callbacks)
stdlib/Makefile:CFLAGS-qsort.c += $(uses-callbacks)
find out what we support currently and what we need to support.

> > --- a/nptl/cleanup_compat.c
> > +++ b/nptl/cleanup_compat.c
> > @@ -48,3 +48,11 @@ _pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer, int execute)
> >      buffer->__routine (buffer->__arg);
> >  }
> >  strong_alias (_pthread_cleanup_pop, __pthread_cleanup_pop)
> > +
> > +void
> > +__pthread_cleanup_cleanup (struct _pthread_cleanup_buffer *buffer)
> > +{
> > +  struct pthread *self = THREAD_SELF;
> > +  if (THREAD_GETMEM (self, cleanup) == buffer)
> > +    THREAD_SETMEM (self, cleanup, buffer->__prev);
> > +}
> 
> It's surprising that the cleanup field update is conditional.  How does
> that happen?  Is it really safe?

As I've tried to explain, there are several cases.
One is throw in the callback, that invokes __pthread_cleanup_combined_routine
only and not the other routine and we need to cleanup.  In that case we need
to do the cleanup and self->cleanup ought to be equal to the buffer.

Another is cancellation with unwind info in callback, from debugging it
seems that the self->cleanup registered callbacks are invoked first and
those unregister the cleanup.  In earlier versions of the patch
__pthread_cleanup_cleanup has been called unconditionally from
__pthread_cleanup_combined_routine and in that case __pthread_cleanup_cleanup
would need to be conditional, but in the current version that is not needed.

Another is cancellation without unwind info in callback, that does
just self->cleanup registered callbacks and no problematic cases.

Another is normal return from the callback, in that case whether the
cleanups are run depends on the execute argument (which is 0 for
pthread_once).  The self->cleanup cleanup is unregistered with hardcoded 0
so callback isn't invoked, and then the cleanup attribute based one uses
the execute passed to the macro.  If it would be non-zero, we need
conditional cleanup because __pthread_cleanup_pop has already unregistered
it, if we only support 0, then __pthread_cleanup_cleanup can be
unconditional.

> > +# if defined __GNUC__ && defined __EXCEPTIONS && !defined __cplusplus
> > +/* Structure to hold the cleanup handler information.  */
> > +struct __pthread_cleanup_combined_frame
> > +{
> > +  void (*__cancel_routine) (void *);
> > +  void *__cancel_arg;
> > +  int __do_it;
> > +  struct _pthread_cleanup_buffer __buffer;
> > +};
> 
> No __GNUC__ is necessary in internal headers.

Ok, will change.

> > +/* Special cleanup macros which register cleanup both using
> > +   __pthread_cleanup_{push,pop} and using cleanup attribute.  This is needed
> > +   for pthread_once, so that it supports both throwing exceptions from the
> > +   pthread_once callback (only cleanup attribute works there) and cancellation
> > +   of the thread running the callback if the callback or some routines it
> > +   calls don't have unwind information.  */
> > +
> > +static inline void
> > +__pthread_cleanup_combined_routine (struct __pthread_cleanup_combined_frame
> > +				    *__frame)
> > +{
> > +  if (__frame->__do_it)
> > +    {
> > +      __frame->__cancel_routine (__frame->__cancel_arg);
> > +      __frame->__do_it = 0;
> > +      __pthread_cleanup_cleanup (&__frame->__buffer);
> > +    }
> > +}
> 
> Is there a way to turn the call to __frame->__cancel_routine into a
> direct call?  For more general use (outside pthread_once) that might be
> desirable from a security hardening perspective.  I expect it will
> happen if __pthread_cleanup_combined_routine_voidptr and
> __pthread_cleanup_combined_routine do not use the sam variable.

Looking at GCC tree dumps, __pthread_cleanup_combined_routine is inlined
(but because it is considered expensive, it is inlined only partially (i.e.
the __frame->__do_it test).
__attribute__((__always_inline__)) can force it to be inlined fully.
The other thing is the __cancel_routine call, and that is sadly indirect
even if I change it so that while __clframe.__do_it = 1; is done before
__pthread_cleanup_push, __clframe.__cancel_{routine,arg} is initialized
after it returns.  For the compiler, the address of __clframe escaped
and so it doesn't know if init_routine will not modify it.

For __pthread_cleanup_push registered callback, I'm afraid there is no way
out of that.
For the unwind info registered callback, maybe having one structure that
wouldn't be really address taken (containing __cancel_routine, __cancel_arg
and pointer to another structure containing the pthread cleanup buffer
and __do_it var) could do the job.

> I guess we can fix this later.  For pthread_once, the indirect call only
> happens on the unwinding path.

Yes.
> 
> > +static inline void
> > +__pthread_cleanup_combined_routine_voidptr (void *__arg)
> > +{
> > +  struct __pthread_cleanup_combined_frame *__frame
> > +    = (struct __pthread_cleanup_combined_frame *) __arg;
> > +  if (__frame->__do_it)
> > +    {
> > +      __frame->__cancel_routine (__frame->__cancel_arg);
> > +      __frame->__do_it = 0;
> > +    }
> > +}
> 
> I think you can make this an out-of-line routine because it can never be
> inlined.  But given that this is called only once, it probably does not
> matter.

This one indeed could be out of line and the other could have out of line
copy and extern inline version for inlining (or static inline
always_inline).  I didn't bother when it is for a single spot, but if
it would be for multiple ones, sure.
> 
> > diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> > index eeb64f9fb0..53b65ef349 100644
> > --- a/sysdeps/pthread/Makefile
> > +++ b/sysdeps/pthread/Makefile
> > @@ -231,7 +231,7 @@ generated += $(objpfx)tst-atfork2.mtrace \
> >  
> >  tests-internal += tst-cancel25 tst-robust8
> >  
> > -tests += tst-oncex3 tst-oncex4
> > +tests += tst-oncex3 tst-oncex4 tst-oncey3 tst-oncey4
> 
> tst-oncey3.c, tst-oncey4.c are missing, but I guessed their contents.

Oops, sorry.  Their content is the same as of tst-oncex3.c and tst-oncex4.c.

	Jakub

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

[Florian Weimer]

* Jakub Jelinek:

> On Thu, Mar 04, 2021 at 12:50:54PM +0100, Florian Weimer wrote:
>> I initially thought that we had to use setjmp-based registration
>> approach, but that does not seem to be the case.
>> 
>> Any idea why we have the setjmp-based approach (for -fno-exceptions) for
>> external use, and not use the legacy approach internal to glibc?  One
>> difference is that it restores the value of callee-saved registers,
>> which could matter if we switch between two kinds of unwinding (callback
>> registration and DWARF).
>
> I'm afraid I don't know, I thought the public pthread_cleanup_push/pop
> for -fno-exceptions is the same as the pthreadP.h one except that it uses
> _pthread_cleanup_* instead of __pthread_cleanup_*.
> All I remember that the normal unwinding does a longjmp at the end after
> processing all the self->cleanup registered cleanups, but I thought it is
> just to the pthread_create start.

I'm going to assume it's the callee saved registers issue.

>> Do you think this approach should used for all callback-based functions,
>> not just pthread_once?
>
> If we want to support throw from the callbacks that would do cleanups and
> continue throwing outside of the libc routines and at the same time
> support no unwind callbacks with cancellation points.
> I guess it would need separate analysis of:
> argp/Makefile:CFLAGS-argp-help.c += $(uses-callbacks) -fexceptions
> argp/Makefile:CFLAGS-argp-parse.c += $(uses-callbacks)
> dirent/Makefile:CFLAGS-scandir.c += $(uses-callbacks)
> dirent/Makefile:CFLAGS-scandir64.c += $(uses-callbacks)
> dirent/Makefile:CFLAGS-scandir-tail.c += $(uses-callbacks)
> dirent/Makefile:CFLAGS-scandir64-tail.c += $(uses-callbacks)
> elf/Makefile:CFLAGS-dl-iteratephdr.c += $(uses-callbacks)
> io/Makefile:CFLAGS-fts.c += -Wno-uninitialized $(uses-callbacks) -fexceptions
> io/Makefile:CFLAGS-fts64.c += -Wno-uninitialized $(uses-callbacks) -fexceptions
> io/Makefile:CFLAGS-ftw.c += $(uses-callbacks) -fexceptions
> io/Makefile:CFLAGS-ftw64.c += $(uses-callbacks) -fexceptions
> malloc/Makefile:CFLAGS-obstack.c += $(uses-callbacks)
> misc/Makefile:CFLAGS-tsearch.c += $(uses-callbacks)
> misc/Makefile:CFLAGS-lsearch.c += $(uses-callbacks)
> nptl/Makefile:CFLAGS-pthread_once.c += $(uses-callbacks) -fexceptions \
> posix/Makefile:CFLAGS-glob.c += $(uses-callbacks) -fexceptions
> posix/Makefile:CFLAGS-glob64.c += $(uses-callbacks) -fexceptions
> stdlib/Makefile:CFLAGS-bsearch.c += $(uses-callbacks)
> stdlib/Makefile:CFLAGS-msort.c += $(uses-callbacks)
> stdlib/Makefile:CFLAGS-qsort.c += $(uses-callbacks)
> find out what we support currently and what we need to support.

Or rename the macros and fix the stuff that breaks, yes.

For the internal uses, we could use a centralized cleanup function which
uses a switch statement over the cleanup type.  This would likely be
easier to use, too, and the indirect calls go away.

>> > --- a/nptl/cleanup_compat.c
>> > +++ b/nptl/cleanup_compat.c
>> > @@ -48,3 +48,11 @@ _pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer, int execute)
>> >      buffer->__routine (buffer->__arg);
>> >  }
>> >  strong_alias (_pthread_cleanup_pop, __pthread_cleanup_pop)
>> > +
>> > +void
>> > +__pthread_cleanup_cleanup (struct _pthread_cleanup_buffer *buffer)
>> > +{
>> > +  struct pthread *self = THREAD_SELF;
>> > +  if (THREAD_GETMEM (self, cleanup) == buffer)
>> > +    THREAD_SETMEM (self, cleanup, buffer->__prev);
>> > +}
>> 
>> It's surprising that the cleanup field update is conditional.  How does
>> that happen?  Is it really safe?
>
> As I've tried to explain, there are several cases.
> One is throw in the callback, that invokes __pthread_cleanup_combined_routine
> only and not the other routine and we need to cleanup.  In that case we need
> to do the cleanup and self->cleanup ought to be equal to the buffer.
>
> Another is cancellation with unwind info in callback, from debugging it
> seems that the self->cleanup registered callbacks are invoked first and
> those unregister the cleanup.  In earlier versions of the patch
> __pthread_cleanup_cleanup has been called unconditionally from
> __pthread_cleanup_combined_routine and in that case __pthread_cleanup_cleanup
> would need to be conditional, but in the current version that is not needed.
>
> Another is cancellation without unwind info in callback, that does
> just self->cleanup registered callbacks and no problematic cases.
>
> Another is normal return from the callback, in that case whether the
> cleanups are run depends on the execute argument (which is 0 for
> pthread_once).  The self->cleanup cleanup is unregistered with hardcoded 0
> so callback isn't invoked, and then the cleanup attribute based one uses
> the execute passed to the macro.  If it would be non-zero, we need
> conditional cleanup because __pthread_cleanup_pop has already unregistered
> it, if we only support 0, then __pthread_cleanup_cleanup can be
> unconditional.

Okay, we'll have to revisit this if we ever change the interfaces and do
the consolidation.

>> Is there a way to turn the call to __frame->__cancel_routine into a
>> direct call?  For more general use (outside pthread_once) that might be
>> desirable from a security hardening perspective.  I expect it will
>> happen if __pthread_cleanup_combined_routine_voidptr and
>> __pthread_cleanup_combined_routine do not use the sam variable.
>
> Looking at GCC tree dumps, __pthread_cleanup_combined_routine is inlined
> (but because it is considered expensive, it is inlined only partially (i.e.
> the __frame->__do_it test).
> __attribute__((__always_inline__)) can force it to be inlined fully.
> The other thing is the __cancel_routine call, and that is sadly indirect
> even if I change it so that while __clframe.__do_it = 1; is done before
> __pthread_cleanup_push, __clframe.__cancel_{routine,arg} is initialized
> after it returns.  For the compiler, the address of __clframe escaped
> and so it doesn't know if init_routine will not modify it.
>
> For __pthread_cleanup_push registered callback, I'm afraid there is no way
> out of that.
> For the unwind info registered callback, maybe having one structure that
> wouldn't be really address taken (containing __cancel_routine, __cancel_arg
> and pointer to another structure containing the pthread cleanup buffer
> and __do_it var) could do the job.

For __pthread_cleanup_push, we can avoid the indirect call on the
non-exception path if we move it out of the function, like I did here:

  [PATCH 3/8] nptl: Move legacy unwinding implementation into libc
  <https://sourceware.org/pipermail/libc-alpha/2021-March/123205.html>

Again, for pthread_once this does not matter.

>> > +static inline void
>> > +__pthread_cleanup_combined_routine_voidptr (void *__arg)
>> > +{
>> > +  struct __pthread_cleanup_combined_frame *__frame
>> > +    = (struct __pthread_cleanup_combined_frame *) __arg;
>> > +  if (__frame->__do_it)
>> > +    {
>> > +      __frame->__cancel_routine (__frame->__cancel_arg);
>> > +      __frame->__do_it = 0;
>> > +    }
>> > +}
>> 
>> I think you can make this an out-of-line routine because it can never be
>> inlined.  But given that this is called only once, it probably does not
>> matter.
>
> This one indeed could be out of line and the other could have out of line
> copy and extern inline version for inlining (or static inline
> always_inline).  I didn't bother when it is for a single spot, but if
> it would be for multiple ones, sure.

Understood, thanks.

I think the patch is okay, except for the nits I pointed out.

Thanks,
Florian

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

[Jakub Jelinek]

On Thu, Mar 04, 2021 at 02:26:32PM +0100, Florian Weimer wrote:
> I think the patch is okay, except for the nits I pointed out.

Here is an updated patch.  Changes from the first one are
dropped __GNUC__ check, added tst-oncey*.c tests, avoiding indirect
function call in the theoretical execute != 0 case and dropping
__pthread_cleanup_cleanup because of that as it is now called
only in case the unwind cleanup does the cleaning up which means
neither unwind_stop nor __pthread_cleanup_pop did it.

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

This is another attempt at making pthread_once handle throwing exceptions
from the init routine callback.  As the new testcases show, just switching
to the cleanup attribute based cleanup does fix the tst-once5 test, but
breaks the new tst-oncey3 test.  That is because when throwing exceptions,
only the unwind info registered cleanups (i.e. C++ destructors or cleanup
attribute), when cancelling threads and there has been unwind info from the
cancellation point up to whatever needs cleanup both unwind info registered
cleanups and THREAD_SETMEM (self, cleanup, ...) registered cleanups are
invoked, but once we hit some frame with no unwind info, only the
THREAD_SETMEM (self, cleanup, ...) registered cleanups are invoked.
So, to stay fully backwards compatible (allow init routines without
unwind info which encounter cancellation points) and handle exception throwing
we actually need to register the pthread_once cleanups in both unwind info
and in the THREAD_SETMEM (self, cleanup, ...) way.
If an exception is thrown, only the former will happen and we in that case
need to also unregister the THREAD_SETMEM (self, cleanup, ...) registered
handler, because otherwise after catching the exception the user code could
call deeper into the stack some cancellation point, get cancelled and then
a stale cleanup handler would clobber stack and probably crash.
If a thread calling init routine is cancelled and unwind info ends before
the pthread_once frame, it will be cleaned up through self->cleanup as
before.  And if unwind info is present, unwind_stop first calls the
self->cleanup registered handler for the frame, then it will call the
unwind info registered handler but that will already see __do_it == 0
and do nothing.

diff --git a/nptl/Makefile b/nptl/Makefile
index 5f85dd7854..33766eaf7a 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -386,10 +386,6 @@ xtests += tst-eintr1
 
 test-srcs = tst-oddstacklimit
 
-# Test expected to fail on most targets (except x86_64) due to bug
-# 18435 - pthread_once hangs when init routine throws an exception.
-test-xfail-tst-once5 = yes
-
 gen-as-const-headers = unwindbuf.sym \
 		       pthread-pi-defines.sym
 
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index d2fd0826fe..93f3cef00f 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -604,6 +604,67 @@ extern void __pthread_cleanup_pop (struct _pthread_cleanup_buffer *buffer,
 # undef pthread_cleanup_pop
 # define pthread_cleanup_pop(execute)                   \
   __pthread_cleanup_pop (&_buffer, (execute)); }
+
+# if defined __EXCEPTIONS && !defined __cplusplus
+/* Structure to hold the cleanup handler information.  */
+struct __pthread_cleanup_combined_frame
+{
+  void (*__cancel_routine) (void *);
+  void *__cancel_arg;
+  int __do_it;
+  struct _pthread_cleanup_buffer __buffer;
+};
+
+/* Special cleanup macros which register cleanup both using
+   __pthread_cleanup_{push,pop} and using cleanup attribute.  This is needed
+   for pthread_once, so that it supports both throwing exceptions from the
+   pthread_once callback (only cleanup attribute works there) and cancellation
+   of the thread running the callback if the callback or some routines it
+   calls don't have unwind information.  */
+
+static __always_inline void
+__pthread_cleanup_combined_routine (struct __pthread_cleanup_combined_frame
+				    *__frame)
+{
+  if (__frame->__do_it)
+    {
+      __frame->__cancel_routine (__frame->__cancel_arg);
+      __frame->__do_it = 0;
+      __pthread_cleanup_pop (&__frame->__buffer, 0);
+    }
+}
+
+static inline void
+__pthread_cleanup_combined_routine_voidptr (void *__arg)
+{
+  struct __pthread_cleanup_combined_frame *__frame
+    = (struct __pthread_cleanup_combined_frame *) __arg;
+  if (__frame->__do_it)
+    {
+      __frame->__cancel_routine (__frame->__cancel_arg);
+      __frame->__do_it = 0;
+    }
+}
+
+#  define pthread_cleanup_combined_push(routine, arg) \
+  do {									      \
+    void (*__cancel_routine) (void *) = (routine);			      \
+    struct __pthread_cleanup_combined_frame __clframe			      \
+      __attribute__ ((__cleanup__ (__pthread_cleanup_combined_routine)))      \
+      = { .__cancel_routine = __cancel_routine, .__cancel_arg = (arg),	      \
+	  .__do_it = 1 };						      \
+    __pthread_cleanup_push (&__clframe.__buffer,			      \
+			    __pthread_cleanup_combined_routine_voidptr,	      \
+			    &__clframe);
+
+#  define pthread_cleanup_combined_pop(execute) \
+    __pthread_cleanup_pop (&__clframe.__buffer, 0);			      \
+    __clframe.__do_it = 0;						      \
+    if (execute)							      \
+      __cancel_routine (__clframe.__cancel_arg);			      \
+  } while (0)
+
+# endif
 #endif
 
 extern void __pthread_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer,
diff --git a/nptl/pthread_once.c b/nptl/pthread_once.c
index 28d97097c7..7645da222a 100644
--- a/nptl/pthread_once.c
+++ b/nptl/pthread_once.c
@@ -111,11 +111,11 @@ __pthread_once_slow (pthread_once_t *once_control, void (*init_routine) (void))
       /* This thread is the first here.  Do the initialization.
 	 Register a cleanup handler so that in case the thread gets
 	 interrupted the initialization can be restarted.  */
-      pthread_cleanup_push (clear_once_control, once_control);
+      pthread_cleanup_combined_push (clear_once_control, once_control);
 
       init_routine ();
 
-      pthread_cleanup_pop (0);
+      pthread_cleanup_combined_pop (0);
 
 
       /* Mark *once_control as having finished the initialization.  We need
diff --git a/nptl/tst-once5.cc b/nptl/tst-once5.cc
index b797ab3562..60fe1ef820 100644
--- a/nptl/tst-once5.cc
+++ b/nptl/tst-once5.cc
@@ -59,7 +59,7 @@ do_test (void)
                " throwing an exception", stderr);
     }
     catch (OnceException) {
-      if (1 < niter)
+      if (niter > 1)
         fputs ("pthread_once unexpectedly threw", stderr);
       result = 0;
     }
@@ -75,7 +75,5 @@ do_test (void)
   return result;
 }
 
-// The test currently hangs and is XFAILed.  Reduce the timeout.
-#define TIMEOUT 1
 #define TEST_FUNCTION do_test ()
 #include "../test-skeleton.c"
diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index eeb64f9fb0..53b65ef349 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -231,7 +231,7 @@ generated += $(objpfx)tst-atfork2.mtrace \
 
 tests-internal += tst-cancel25 tst-robust8
 
-tests += tst-oncex3 tst-oncex4
+tests += tst-oncex3 tst-oncex4 tst-oncey3 tst-oncey4
 
 modules-names += tst-join7mod
 
@@ -242,6 +242,8 @@ endif
 
 CFLAGS-tst-oncex3.c += -fexceptions
 CFLAGS-tst-oncex4.c += -fexceptions
+CFLAGS-tst-oncey3.c += -fno-exceptions -fno-asynchronous-unwind-tables
+CFLAGS-tst-oncey4.c += -fno-exceptions -fno-asynchronous-unwind-tables
 
 $(objpfx)tst-join7: $(libdl) $(shared-thread-library)
 $(objpfx)tst-join7.out: $(objpfx)tst-join7mod.so
diff --git a/sysdeps/pthread/tst-oncey3.c b/sysdeps/pthread/tst-oncey3.c
new file mode 100644
index 0000000000..08225b88dc
--- /dev/null
+++ b/sysdeps/pthread/tst-oncey3.c
@@ -0,0 +1 @@
+#include "tst-once3.c"
diff --git a/sysdeps/pthread/tst-oncey4.c b/sysdeps/pthread/tst-oncey4.c
new file mode 100644
index 0000000000..9b4d98f3f1
--- /dev/null
+++ b/sysdeps/pthread/tst-oncey4.c
@@ -0,0 +1 @@
+#include "tst-once4.c"

	Jakub