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

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

So I suggest this _instead of_ the above change:


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

On 11/05/2017 02:46 PM, 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*.
> 
> So I suggest this _instead of_ the above change:
> 
> 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 don't think we should say that at the end.  It will be wrong someday
(hopefully), without having to do anything to it.  It really just needs
a @pxref we don't have yet, so instead, say nothing, which will continue
to be correct both before and after real-time signal semantics are
documented, and after they are, the reference can be added to improve
the manual.

Maybe a bug report that real-time signal semantics aren't documented
would be a good place to both track the issue and keep a reminder to
update this paragraph.

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

Otherwise, I like the direction you took this, especially since it slips
in documentation for some previously undocumented macros along with the
concept.

Rical
  

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.
  

On 06/11/2017 00:59, Rical Jasan wrote:
> On 11/05/2017 02:46 PM, 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*.
>>
>> So I suggest this _instead of_ the above change:
>>
>> 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 don't think we should say that at the end.  It will be wrong someday
> (hopefully), without having to do anything to it.  It really just needs
> a @pxref we don't have yet, so instead, say nothing, which will continue
> to be correct both before and after real-time signal semantics are
> documented, and after they are, the reference can be added to improve
> the manual.

So your suggestion is just remove this snippet?

> 
> Maybe a bug report that real-time signal semantics aren't documented
> would be a good place to both track the issue and keep a reminder to
> update this paragraph.

I will create a bug report for it.

> 
>> +
>> +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
> 
> Otherwise, I like the direction you took this, especially since it slips
> in documentation for some previously undocumented macros along with the
> concept.
> 
> Rical
>
  

On Sun, Nov 5, 2017 at 9:59 PM, Rical Jasan <ricaljasan@pacific.net> wrote:
> On 11/05/2017 02:46 PM, Zack Weinberg wrote:
>> +@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 don't think we should say that at the end.  It will be wrong someday
> (hopefully), without having to do anything to it.  It really just needs
> a @pxref we don't have yet, so instead, say nothing, which will continue
> to be correct both before and after real-time signal semantics are
> documented, and after they are, the reference can be added to improve
> the manual.

I don't want to say nothing, because if we say nothing, it's an
undefined term and readers will be frustrated.  If we admit that this
is a gap in the manual, they'll still be frustrated but at least they
won't waste time searching for the missing piece.

> Maybe a bug report that real-time signal semantics aren't documented
> would be a good place to both track the issue and keep a reminder to
> update this paragraph.

Agree with this part.

zw