Re: [PATCH 1/6] Consolidate Linux sigprocmask implementation (BZ #22391)

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 05/11/2017 20:46, Zack Weinberg wrote:
> On Fri, Nov 3, 2017 at 5:40 PM, Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>> This patch consolidates the sigprocmask Linux syscall implementation on
>> sysdeps/unix/sysv/linux/sigprocmask.c
> 
> This looks like a virtuous change overall, but I don't know the code
> well enough to singlehandedly review it.  I'd like to provide some
> feedback on the manual change, though:
> 
>> +On Linux pthreads internally uses some signals to implement asynchronous
>> +cancellation, effective ID synchronization for POSIX conformance, and
>> +posix timer management.  These signals are filtered out on signal set
>> +function manipulation.
> 
> This has grammar problems, and also, not all of the signal-set
> functions filter the special signals.  For instance, sigfillset does,
> but sigaddset doesn't (if I'm reading the code right, anyway).  And
> this isn't even the right place to document this, because the main
> reason why we need to document it is to explain why the kernel's idea
> of the real-time signal range differs from what glibc exposes to
> applications ... and the real-time signal range isn't documented at
> all, *headdesk*.

Indeed not all signal-set function filter the special signals and
the second part of this patchset is indeed to fix all the remaining
cases (tracked also by BZ#22391 [1], I might have forgot some
symbol on my bug report).

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=22391

> 
> So I suggest this _instead of_ the above change:

This change looks reasonable.

> 
> diff --git a/manual/signal.texi b/manual/signal.texi
> index 9577ff091d..4f0ef59fb7 100644
> --- a/manual/signal.texi
> +++ b/manual/signal.texi
> @@ -235,6 +235,7 @@ defined.  Since the signal numbers are allocated
> consecutively,
>  * Job Control Signals::         Signals used to support job control.
>  * Operation Error Signals::     Used to report operational system errors.
>  * Miscellaneous Signals::       Miscellaneous Signals.
> +* Internally-Used Signals::     Signals used internally by the C library.
>  * Signal Messages::             Printing a message describing a signal.
>  @end menu
> 
> @@ -794,6 +795,26 @@ in @ref{Signaling Another Process}.
>  The default action is to terminate the process.
>  @end deftypevr
> 
> +@deftypevr Macro int SIGRTMIN
> +@deftypevrx Macro int SIGRTMAX
> +@standards{POSIX.1, signal.h}
> +@cindex real-time signals
> +The range of signal numbers @code{SIGRTMIN}, @code{SIGRTMIN+1},
> +@dots{}, @code{SIGRTMAX} is also set aside for you to use any way you
> +want.  In addition, these signals (and no others) are guaranteed to
> +support @dfn{real-time} signal semantics, which unfortunately this
> +manual does not yet document.

I think we can omit the last sentence "which unfortunately this
manual does not yet document.".

> +
> +Unlike all of the other signal number macros, @code{SIGRTMIN} and
> +@code{SIGRTMAX} are not compile-time constants, because some operating
> +systems make the number of real-time signals tunable on a
> +per-installation or even per-process basis.  However, POSIX guarantees
> +that there will be at least 8 signal numbers in this range.
> +
> +The default action for all signals in this range is to terminate the
> +process.
> +@end deftypevr
> +
>  @deftypevr Macro int SIGWINCH
>  @standards{BSD, signal.h}
>  Window size change.  This is generated on some systems (including GNU)
> @@ -817,6 +838,22 @@ to print some status information about the system
> and what the process
>  is doing.  Otherwise the default is to do nothing.
>  @end deftypevr
> 
> +@node Internally-Used Signals
> +@subsection Internally-Used Signals
> +@cindex internally used signals
> +
> +On some operating systems, @theglibc{} needs to use a few signals from
> +the ``true'' real-time range internally, to implement thread
> +cancellation, cross-thread effective ID synchronization, POSIX timer
> +management, etc.  @Theglibc{} adjusts @code{SIGRTMIN} and
> +@code{SIGRTMAX} to exclude these signals, and it also takes steps to
> +prevent application code from altering their state: @code{sigprocmask}
> +cannot block them and @code{sigaction} cannot redefine their handlers,
> +for instance.  Therefore, most programs do not need to know or care
> +about these signals.  We mainly document their existence for the sake
> +of anyone who has ever wondered why there is a gap between the
> +highest-numbered ``normal'' signal and @code{SIGRTMIN} on Linux.
> +
>  @node Signal Messages
>  @subsection Signal Messages
>  @cindex signal messages
> 

LGTM this addition, I will incorporate this on my next submission.