Re: [MTASCsft PATCH WIP6 01/33] MT-, AS- and AC-Safety docs

["Carlos O'Donell" <carlos at redhat dot com>]

On 01/16/2014 07:56 PM, Alexandre Oliva wrote:
> Besides changes you proposed or requested, the following incremental
> patch introduces the following changes:
> 
> - a note about @mtasurace{:tcattr} under the definition of âtermâ;
> 
> - the new sigintr keyword and the @mtssigintr macro, for signal
> functions that read from (or @mtasurace on) the global _sigintr variable;
> 
> - the new @asurace macro, for the thread-local static buffer in
> inet_ntoa;
> 
> These changes are to be integrated with the v6 patch and submitted as a
> consolidated patch once we have agreement on the few remaining pending
> points in the review of the first v6 patch.
 
Changes here are OK modulo changes I sent in my other two reviews.

I think we're basically done.

Please post a final patch 01 tomorrow for a final review.
 
> post-v6-intro.patch
> 
> 
> Post v6 patches for intro.texi and macros.texi.
> 
> From: Alexandre Oliva <aoliva@redhat.com>
> 
> 
> ---
>  manual/intro.texi  |  151 +++++++++++++++++++++++++++++-----------------------
>  manual/macros.texi |   10 +++
>  2 files changed, 93 insertions(+), 68 deletions(-)
> 
> diff --git a/manual/intro.texi b/manual/intro.texi
> index 55234cc..fede2fd 100644
> --- a/manual/intro.texi
> +++ b/manual/intro.texi
> @@ -161,10 +161,9 @@ pattern matching facilities (@pxref{Pattern Matching}).
>  
>  @menu
>  * POSIX Safety Concepts::       Safety concepts from POSIX.
> -* Unconditional Unsafety::      Features that make functions
> -                                 unconditionally unsafe.
> -* Avoidable Unsafety::          Features that make functions unsafe,
> -                                 but that can be worked around.
> +* Unsafe Features::             Features that make functions unsafe.
> +* Conditionally Safe Features:: Features that make functions unsafe
> +                                 in the absence of work-arounds.

OK (ignoring work-arounds).

>  * Other Safety Remarks::        Additional safety features and remarks.
>  @end menu
>  
> @@ -181,12 +180,12 @@ pattern matching facilities (@pxref{Pattern Matching}).
>  @comment <wordexp.h> (not yet implemented)
>  @comment confstr
>  
> -@node POSIX Safety Concepts, Unconditional Unsafety, , POSIX
> +@node POSIX Safety Concepts, Unsafe Features, , POSIX
>  @subsubsection POSIX Safety Concepts
>  @cindex POSIX Safety Concepts
>  
>  This manual documents various safety properties of @glibcadj{}
> -functions, in lines that follow their prototypess and look like:
> +functions, in lines that follow their prototypes and look like:

OK.

>  @sampsafety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
>  
> @@ -200,9 +199,8 @@ attempting to capture the meaning of the standard definitions, follow.
>  @item
>  @cindex MT-Safe
>  @cindex Thread-Safe
> -
> -MT-Safe or Thread-Safe functions are safe to call in the presence of
> -other threads.  MT, in MT-Safe, stands for Multi Thread.
> +@code{MT-Safe} or Thread-Safe functions are safe to call in the presence
> +of other threads.  MT, in MT-Safe, stands for Multi Thread.

OK.

>  Being MT-Safe does not imply a function is atomic, nor that it uses any
>  of the memory synchronization mechanisms POSIX exposes to users.  It is
> @@ -226,8 +224,7 @@ documented MT-Safety status guaranteed under such arrangements.
>  @item
>  @cindex AS-Safe
>  @cindex Async-Signal-Safe
> -
> -AS-Safe or Async-Signal-Safe functions are safe to call from
> +@code{AS-Safe} or Async-Signal-Safe functions are safe to call from
>  asynchronous signal handlers.  AS, in AS-Safe, stands for Asynchronous
>  Signal.

OK.

> @@ -244,11 +241,18 @@ save their original values, and restore them before returning.
>  @item
>  @cindex AC-Safe
>  @cindex Async-Cancel-Safe
> -
> -AC-Safe or Async-Cancel-Safe functions are safe to call when
> +@code{AC-Safe} or Async-Cancel-Safe functions are safe to call when
>  asynchronous cancellation is enabled.  AC in AC-Safe stands for
>  Asynchronous Cancellation.

OK.

> +The POSIX standard defines only three functions to be AC-Safe, namely
> +@code{pthread_cancel}, @code{pthread_setcancelstate}, and
> +@code{pthread_setcanceltype}.  At present @theglibc{} provides no
> +guarantees beyond these three functions, but does document which
> +functions are presently AC-Safe.  This documentation is provided for use
> +by @theglibc{} developers.

OK.

> +
> +
>  @item
>  @cindex MT-Unsafe
>  @cindex Thread-Unsafe
> @@ -256,10 +260,9 @@ Asynchronous Cancellation.
>  @cindex Async-Signal-Unsafe
>  @cindex AC-Unsafe
>  @cindex Async-Cancel-Unsafe
> -
> -MT-Unsafe, AS-Unsafe, AC-Unsafe functions are not safe to call within
> -the safety contexts described above.  Calling them within such contexts
> -invokes undefined behavior.
> +@code{MT-Unsafe}, @code{AS-Unsafe}, @code{AC-Unsafe} functions are not
> +safe to call within the safety contexts described above.  Calling them
> +within such contexts invokes undefined behavior.

OK.

>  Functions not explicitly documented as Safe in a safety context should
>  be regarded as Unsafe.
> @@ -267,8 +270,7 @@ be regarded as Unsafe.
>  
>  @item
>  @cindex Preliminary
> -
> -Preliminary safety properties are documented, indicating these
> +@code{Preliminary} safety properties are documented, indicating these
>  properties may @emph{not} be counted on in future releases of
>  @theglibc{}.

OK.
 
> @@ -309,9 +311,9 @@ Other keywords that appear in safety notes are defined in subsequent
>  sections.
>  
>  
> -@node Unconditional Unsafety, Avoidable Unsafety, POSIX Safety Concepts, POSIX
> -@subsubsection Unconditional Unsafety
> -@cindex Unconditional Unsafety
> +@node Unsafe Features, Conditionally Safe Features, POSIX Safety Concepts, POSIX
> +@subsubsection Unsafe Features
> +@cindex Unsafe Features

OK.

>  Functions that are unsafe to call in certain contexts are annotated with
>  keywords that document their features that make them unsafe to call.
> @@ -372,8 +374,8 @@ of the data structure may misbehave.
>  @cindex heap
>  
>  Functions marked with @code{heap} may call heap memory management
> -functions in the @code{malloc}/@code{free} family, which brings in their
> -safety issues.  This note is thus equivalent to:
> +functions from the @code{malloc}/@code{free} family of functions and are
> +only as safe as those functions.  This note is thus equivalent to:

OK.

>  @sampsafety{@asunsafe{@asulock{}}@acunsafe{@aculock{} @acsfd{} @acsmem{}}}
>  
> @@ -384,15 +386,15 @@ safety issues.  This note is thus equivalent to:
>  @item @code{dlopen}
>  @cindex dlopen
>  
> -Functions marked with @code{dlopen} use the dynamic loader to bring in
> -additional code modules.  This involves opening files, mapping them into
> -memory, allocating additional memory, resolving symbols, applying
> -relocations and more, all of this while holding internal dynamic loader
> -locks.
> +Functions marked with @code{dlopen} use the dynamic loader to load
> +shared libraries into the current execution image.  This involves
> +opening files, mapping them into memory, allocating additional memory,
> +resolving symbols, applying relocations and more, all of this while
> +holding internal dynamic loader locks.

OK.

>  The locks are enough for these functions to be AS- and AC-Unsafe, but
> -other issues may arise; the safety documentation of @code{dlopen}, when
> -available, shall list all the keywords implied by this one.
> +other issues may arise.  At present this is a placeholder for all
> +potential safety issues raised by @code{dlopen}.

OK.
  
>  @c dlopen runs init and fini sections of the module; does this mean
>  @c dlopen always implies plugin?
> @@ -404,7 +406,10 @@ available, shall list all the keywords implied by this one.
>  Functions annotated with @code{plugin} may run code from plugins that
>  may be external to @theglibc{}.  Such plugin functions are assumed to be
>  MT-Safe, AS-Unsafe and AC-Unsafe.  Examples of such plugins are stack
> -unwinding libraries and nss back-ends.
> +@cindex NSS
> +unwinding libraries, name service switch (NSS) and character set
> +@cindex iconv
> +conversion (iconv) back-ends.

OK.
 
>  Although the plugins mentioned as examples are all brought in by means
>  of dlopen, the @code{plugin} keyword does not imply any direct
> @@ -420,8 +425,8 @@ a single function takes all of these actions, then it gets both marks.
>  @cindex i18n
>  
>  Functions marked with @code{i18n} may call internationalization
> -functions of the @code{gettext} family, which brings in their safety
> -issues.  This note is thus equivalent to:
> +functions of the @code{gettext} family and will be only as safe as those
> +functions.  This note is thus equivalent to:

OK.

>  @sampsafety{@mtsafe{@mtsenv{}}@asunsafe{@asucorrupt{} @ascuheap{} @ascudlopen{}}@acunsafe{@acucorrupt{}}}
>  
> @@ -433,15 +438,15 @@ Functions marked with @code{timer} use the @code{alarm} function or
>  similar to set a time-out for a system call or a long-running operation.
>  In a multi-threaded program, there is a risk that the time-out signal
>  will be delivered to a different thread, thus failing to interrupt the
> -intended thread and thus disabling the time-out.
> +intended thread.
>

OK.

>  
>  @end itemize
>  
>  
> -@node Avoidable Unsafety, Other Safety Remarks, Unconditional Unsafety, POSIX
> -@subsubsection Avoidable Unsafety
> -@cindex Avoidable Unsafety
> +@node Conditionally Safe Features, Other Safety Remarks, Unsafe Features, POSIX
> +@subsubsection Conditionally Safe Features
> +@cindex Conditionally Safe Features

OK.

>  For some features that make functions unsafe to call in certain
>  contexts, there are known ways to avoid the safety problem other than
> @@ -472,13 +477,12 @@ data structures.
>  
>  If a signal handler interrupts such an initializer, and calls any
>  function that also performs @code{libc_once} initialization, it will
> -deadlock if the thread library is linked in.
> +deadlock if the thread library has been loaded.

OK.

>  Furthermore, if an initializer is partially complete before it is
>  canceled or interrupted by a signal whose handler requires the same
>  initialization, some or all of the initialization may be performed more
> -than once, leaking resources or even result in corrupt internal data
> -structures.
> +than once, leaking resources or even resulting in corrupt internal data.

OK.
 
>  Applications that need to call functions marked with @code{init} as an
>  AS- or AC-Unsafe feature should ensure the initialization is performed
> @@ -504,7 +508,7 @@ even exposed to users.
>  When the objects are passed by users, in some cases there might be
>  uncertainty as to whether the library takes care of synchronization on
>  its own, or whether callers are responsible for it.  We draw the line
> -for objects passed by usersas follows: objects whose types are exposed
> +for objects passed by users as follows: objects whose types are exposed

OK.

>  to callers, and that callers are expected to access directly, such as
>  memory buffers, strings, and various non-opaque structs, are @emph{not}
>  annotated with @code{race}, because it would be noisy and redundant with
> @@ -538,7 +542,7 @@ control.  The non-recursive mutex avoids the MT-Safety issue, but it
>  trades one AS-Safety issue for another, so use in asynchronous signals
>  remains undefined.
>  
> -When the identifier relates with a static buffer used to hold return
> +When the identifier relates to a static buffer used to hold return

OK.

>  values, the mutex must be held for as long as the buffer remains in use
>  by the caller.  Many functions that return pointers to static buffers
>  offer reentrant variants that store return values in caller-supplied
> @@ -553,9 +557,9 @@ internal buffers, also documented with @code{race} notes.
>  @item @code{const}
>  @cindex const
>  
> -Functions marked with @code{const} as an MT-Safety issue modify
> -non-atomically internal objects that are better regarded as constant,
> -because a substantial portion of @theglibc{} accesses them without
> +Functions marked with @code{const} as an MT-Safety issue non-atomically
> +modify internal objects that are better regarded as constant, because a
> +substantial portion of @theglibc{} accesses them without

OK.

>  synchronization.  Unlike @code{race}, that causes both readers and
>  writers of internal objects to be regarded as MT-Unsafe and AS-Unsafe,
>  this mark is applied to writers only.  Writers remain equally MT- and
> @@ -614,9 +618,9 @@ handler afterwards is recommended.
>  In functions marked with @code{sig} also as an AC-Safety issue, the
>  problem is more complex: the temporarily-installed signal handler may
>  remain configured and enabled for the whole process if the thread is
> -cancelled, even synchronously.
> +cancelled.

OK.

> -@c fixme: at least sync cancellation should get it right, and would
> +@c fixme: at least deferred cancellation should get it right, and would

OK.

>  @c obviate the restoring bit below, and the qualifier above.
>  
>  Besides the measures recommended to work around the MT- and AS-Safety
> @@ -640,14 +644,16 @@ also AS-Unsafe, but the corresponding mark is omitted as redundant.
>  It is thus advisable for applications using the terminal to avoid
>  concurrent and reentrant interactions with it, by not using it in signal
>  handlers or blocking signals that might use it, and holding a lock while
> -calling these functions and interacting with the terminal.
> +calling these functions and interacting with the terminal.  This lock
> +should also be used for mutual exclusion with functions marked with
> +@code{@mtasurace{:tcattr}}.
>

OK.

>  Functions marked with @code{term} as an AC-Safety issue are supposed to
>  restore terminal settings to their original state, after temporarily
>  changing them, but they may fail to do so if cancelled, even
>  synchronously.
>  
> -@c fixme: at least sync cancellation should get it right, and would
> +@c fixme: at least deferred cancellation should get it right, and would

OK.

>  @c obviate the restoring bit below, and the qualifier above.
>  
>  Besides the measures recommended to work around the MT- and AS-Safety
> @@ -660,7 +666,7 @@ mutex are recommended.
>  @end itemize
>  
>  
> -@node Other Safety Remarks, , Avoidable Unsafety, POSIX
> +@node Other Safety Remarks, , Conditionally Safe Features, POSIX

OK.

>  @subsubsection Other Safety Remarks
>  @cindex Other Safety Remarks
>  
> @@ -687,8 +693,6 @@ are not to be called when multiple threads are running or asynchronous
>  signals are enabled, and so the locale can be considered effectively
>  constant in these contexts, which makes the former safe.
>  
> -@c Should the following be mentioned in the manual at all?
> -

OK.

>  @c Should the locking strategy suggested under @code{const} be used,
>  @c failure to guard locale uses is not as fatal as data races in
>  @c general: unguarded uses will @emph{not} follow dangling pointers or
> @@ -719,6 +723,21 @@ signals are enabled, and so the environment can be considered
>  effectively constant in these contexts, which makes the former safe.
>  
>  
> +@item @code{sigintr}
> +@cindex sigintr
> +
> +Functions marked with @code{sigintr} as an MT-Safety issue access the
> +@code{_sigintr} internal data structure without any guards to ensure
> +safety in the presence of concurrent modifications.
> +
> +We do not mark these functions as MT- or AS-Unsafe, however, because
> +functions that modify the this data structure are all marked with
> +@code{const:sigintr} and regarded as unsafe.  Being unsafe, the latter
> +are not to be called when multiple threads are running or asynchronous
> +signals are enabled, and so the data structure can be considered
> +effectively constant in these contexts, which makes the former safe.
> +
> +

OK.

>  @item @code{fd}
>  @cindex fd
>  
> @@ -727,19 +746,19 @@ descriptors if asynchronous thread cancellation interrupts their
>  execution.
>  
>  Functions that allocate or deallocate file descriptors will generally be
> -marked as such, because even if they attempted to protect the file
> -descriptor allocation and deallocation with cleanup regions, allocating
> -a new descriptor and storing its number where the cleanup region could
> -release it cannot be performed as a single atomic operation, just like
> -releasing it and taking it out of the data structure normally
> -responsible for releasing it cannot be performed atomically, always
> -leaving a window in which the descriptor cannot be released because it
> -was not stored in the cleanup handler argument yet, or in which it was
> -already taken out of it before releasing it in the normal flow (we
> -cannot keep it there because, in case of cancellation, we would not be
> -able to tell whether it was already released, and the same number could
> -have been already assigned to another descriptor by another thread, so
> -we could not just release it again).
> +marked as such.  Even if they attempted to protect the file descriptor
> +allocation and deallocation with cleanup regions, allocating a new
> +descriptor and storing its number where the cleanup region could release
> +it cannot be performed as a single atomic operation.  Similarly,
> +releasing the descriptor and taking it out of the data structure
> +normally responsible for releasing it cannot be performed atomically.
> +There will always be a window in which the descriptor cannot be released
> +because it was not stored in the cleanup handler argument yet, or in
> +which it was already taken out before releasing it in the normal flow.
> +It cannot be taken out after release: an open descriptor could mean
> +either that the descriptor still has to be closed, or that normal flow
> +already did so but the descriptor was reallocated by another thread or
> +signal handler.

OK (ignoring rewrite comments around use of "flow" raised in other review).

>  Such leaks could be internally avoided, with some performance penalty,
>  by temporarily disabling asynchronous thread cancellation.  However,
> diff --git a/manual/macros.texi b/manual/macros.texi
> index 24c20b9..ac691b8 100644
> --- a/manual/macros.texi
> +++ b/manual/macros.texi
> @@ -50,7 +50,7 @@ GNU/Linux systems
>  @c Document the safety functions as preliminary.  It does NOT expand its
>  @c comments.
>  @macro prelim {comments}
> -| Preliminary
> +Preliminary:

OK.

>  
>  @end macro
>  @c Document a function as thread safe.
> @@ -106,6 +106,9 @@ GNU/Linux systems
>  @macro mtasurace {comments}
>  race\comments\
>  @end macro
> +@macro asurace {comments}
> +race\comments\
> +@end macro

OK.

>  @macro mtsrace {comments}
>  race\comments\
>  @end macro
> @@ -118,6 +121,9 @@ locale\comments\
>  @macro mtsenv {comments}
>  env\comments\
>  @end macro
> +@macro mtssigintr {comments}
> +sigintr\comments\
> +@end macro

OK.

>  @macro mtuinit {comments}
>  init\comments\
>  @end macro
> @@ -148,7 +154,7 @@ dlopen\comments\
>  @macro ascuplugin {comments}
>  plugin\comments\
>  @end macro
> -@macro ascui18n {comments}
> +@macro ascuintl {comments}

OK.

>  i18n\comments\
>  @end macro
>  @macro acsfd {comments}

Cheers,
Carlos.