Should pthread_kill be marked __THROW?

Should pthread_kill be marked __THROW?

[Carlos O'Donell]

In glibc we mark pthread_kill as __THROW.

/* Send signal SIGNO to the given thread. */
extern int pthread_kill (pthread_t __threadid, int __signo) __THROW;

Which imparts the 'leaf' attribute on the declaration.

Should it be __THROWNL to avoid problems with compiler optimizations
and signal handlers?

In this case the function is going to deliver a signal to the process
and this is exactly one of the complicated use cases described in the
GCC manual:

~~~
Note that leaf functions might invoke signals and signal handlers might be
defined in the current compilation unit and use static variables.  The only
compliant way to write such a signal handler is to declare such variables
@code{volatile}.
~~~

I sent a gcc doc patch upstream to clarify the text a bit more:
https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00788.html

There I show an example C program that might expect exit with code 0.
When compiled at -O3 it exists with code 1, when compiled at -O0 it
exits with code 0. The problem is that the compiler elides several
stores which it thinks are not observable. The recommendation is that
you use volatile in this case for the variable tst.

One might argue that volatile is heavy handed, and that until and unless
you wait for the signal delivery you can't assume the signal is
ever delivered or that the signal handler observes the effect before
you decrement tst again. Adding sem_post/sem_wait to the example does
fix the issue, as does using C11 atomics. That doesn't mean that the
C or C++ or POSIX standard have anything to say about this particular
case and memory synchronization with signal handlers.

I'm looking at this from a developer perspective and trying to apply
the principle of least surprise. It's suprising that pthread_kill is
not a memory synchronization point. It's surprising when the compiler
elides the store to the global static.

What should we do?

Florian Weimer also allerted me to the fact that there are other
functions which have __THROW and that such markup might be questionable.
I suggest we start another thread for those.

-- 
Cheers,
Carlos.

Re: Should pthread_kill be marked __THROW?

[Roland McGrath]

I don't really understand the rationale by which you think pthread_kill (or
anything else) should be any sort of barrier if it isn't clearly specified
in POSIX (or appropriate standard for something else) that it must be one.

Whatever conclusion applies to pthread_kill should also apply to kill,
sigqueue, killpg, gsignal, and possibly raise.

Re: Should pthread_kill be marked __THROW?

[Florian Weimer]

On 03/18/2016 11:35 PM, Roland McGrath wrote:
> I don't really understand the rationale by which you think pthread_kill (or
> anything else) should be any sort of barrier if it isn't clearly specified
> in POSIX (or appropriate standard for something else) that it must be one.

pthread_kill is specified as calling the signal handler synchronously if
it used to send a signal to the current thread.

Even without that, GCC still generates incorrect code if there is *any*
way to prove that the callback has run because with the leaf attribute,
the code assumes that no user code runs, and (for example) static
variables are not reloaded.

> Whatever conclusion applies to pthread_kill should also apply to kill,
> sigqueue, killpg, gsignal, and possibly raise.

With FUSE, even functions such as fstat can result into a callback from
the kernel to the current process, which means that anything accessing
the file system cannot really be labeled as a leaf function.

For malloc & friends, we have official (but deprecated) replacement
hooks, yet the functions are marked __THROW.  malloc interposition is
another official sanctioned callback mechanism.  No function which
touches the heap can be considered a leaf function as a result.

Similarly, the networking functions in libresolv call a user-supplied
callback, yet they are marked __THROW.  (Even without the callback, the
__THROW attribute is probably what caused problems with some of my
resolver tests because the queries are sent to the same process.)

The most blatant misuse of the __THROW attribute is probably exit, which
calls the exit handlers.

I think the decision to mark all __THROW functions as leaf functions was
a mistake.  Leaf functions are an exception, even among __TRHOW functions.

Florian

Re: Should pthread_kill be marked __THROW?

[Roland McGrath]

> pthread_kill is specified as calling the signal handler synchronously if
> it used to send a signal to the current thread.

Also true of kill, sigqueue, and raise.

> With FUSE, even functions such as fstat can result into a callback from
> the kernel to the current process, 

The issue only arises if it's a callback defined in the same translation
unit as the call (to fstat or whatever), according to the GCC manual.
In the general case, a FUSE callback could indeed be in the same TU in
the same process as the call.  But let's be precise in what we're saying.

> I think the decision to mark all __THROW functions as leaf functions was
> a mistake.  Leaf functions are an exception, even among __TRHOW functions.

I'm not sure I believe that.  Can you elaborate?

The meaning of "leaf" relevant here is the one documented by GCC for
__attribute__ ((leaf)), which is a definition that covers many more
situations than does the common parlance of "leaf function" (a function
that does not call other functions).

Can you give an example of a function that it's safe to mark with
throw () but it's not safe to mark with __attribute__ ((leaf))?

From what I can see off hand, the mistake was to mark so many things
with __THROW.  Making __THROW imply __attribute__ ((leaf)) was probably OK.


Thanks,
Roland

Re: Should pthread_kill be marked __THROW?

[Torvald Riegel]

On Fri, 2016-03-18 at 15:35 -0700, Roland McGrath wrote:
> I don't really understand the rationale by which you think pthread_kill (or
> anything else) should be any sort of barrier if it isn't clearly specified
> in POSIX (or appropriate standard for something else) that it must be one.

I agree.  Calling a signal handler synchronously is not a concurrency
problem in the sense of synchronization with other threads.  The signal
handler can be considered concurrency, but then we should rather tell
the compiler that pthread_kill is like a raise, or not?

Re: Should pthread_kill be marked __THROW?

[Roland McGrath]

> I agree.  Calling a signal handler synchronously is not a concurrency
> problem in the sense of synchronization with other threads.  The signal
> handler can be considered concurrency, but then we should rather tell
> the compiler that pthread_kill is like a raise, or not?

I first read "a raise" as "a call to the 'raise' function", but now I think
you actually meant "raising a C++ exception".  Is that right?

Currently pthread_kill, kill{,pg}, sigqueue, and raise are all annotated
in the same way (with our __THROW macro).  All of these have the same
specified requirement that (in some circumstances) the signal handler run
"inside" the function.  

So raise is marked as never doing a raise.  That is the problem being cited.

Re: Should pthread_kill be marked __THROW?

[Torvald Riegel]

On Fri, 2016-04-08 at 11:30 -0700, Roland McGrath wrote:
> > I agree.  Calling a signal handler synchronously is not a concurrency
> > problem in the sense of synchronization with other threads.  The signal
> > handler can be considered concurrency, but then we should rather tell
> > the compiler that pthread_kill is like a raise, or not?
> 
> I first read "a raise" as "a call to the 'raise' function", but now I think
> you actually meant "raising a C++ exception".  Is that right?

No.  I wasn't aware that ...

> Currently pthread_kill, kill{,pg}, sigqueue, and raise are all annotated
> in the same way (with our __THROW macro).  All of these have the same
> specified requirement that (in some circumstances) the signal handler run
> "inside" the function.  
> 
> So raise is marked as never doing a raise.  That is the problem being cited.

... this is the problem, and thought that raise would be marked
correctly, and only pthread_kill would lack the marking.