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

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

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
>