Re: [MTASCsft PATCH WIP5 01/33] Multi Thread, Async Signal and Async Cancel safety documentation: intro

[Alexandre Oliva <aoliva at redhat dot com>]

On Nov 19, 2013, Torvald Riegel <triegel@redhat.com> wrote:

> On Wed, 2013-11-13 at 06:11 -0200, Alexandre Oliva wrote:

>> +Intuitive definition of these properties, that attempt to capture
>> +the meaning of the standard definitions, follow:

> Spelling: I believe that you should either drop the (first) comma(s), or
> use "which" instead of "that".

Hmm, definitions was supposed to be plural, this may have made it more
confusing than it was.  How about this:

  Intuitive definitions of these properties, attempting to capture the
  meaning of the standard definitions, follow:

> If a program calls an MT-Unsafe function and thereby creates a data
> race, then this data race can manifest in undefined behavior even
> *before* the abstract machine would make the MT-Unsafe call.  This can
> happen because the compiler can assume that programs are data-race-free,
> and use certain optimizations that wouldn't be allowed on (properly
> synchronizing) MT-Safe versions of the called code.  For example, if
> there is sequential code after the call but depending on its outcome,
> then the compiler could speculative and move it before the MT-Unsafe
> call, which could cause a crash before the call actually happens.

What you write is true, but I don't think it's relevant.  All glibc
functions that matter are opaque to their callers, therefore compilers
couldn't possibly optimize assuming there aren't synchronization
primitives in them.  Do you have any evidence of the opposite, that
might justify further obfuscating the definition?

>> +Each of these unprotected uses will use either the previous or the
>> +subsequent locale information, so they won't cause crashes or access to
>> +uninitialized, unmapped or recycled memory.  However, since some cases
>> +use cached locale information while others access the effective locale
>> +object anew, concurrent changes to the global locale object may cause
>> +these functions to behave in ways that they could not behave should the
>> +execution of @code{setlocale} and of the so-annotated functions be
>> +atomic, or even should @code{setlocale} alone be atomic.

> Can you break up this sentence?

There are several sentences in the quoted paragraph.  Can you point out
which one you mean, ideally suggesting alternate wording?

>> +@item @code{envromt}
>> +@cindex envromt
>> +
>> +Functions marked with @code{envromt} access the environment with
>> +@code{getenv} or similar, requiring the environment to be effectively
>> +read-only for MT-Safe operation.
>> +
>> +Environment-modifying functions do not protect in any way against
>> +concurrent modifications or access, so calling @code{envromt}-marked
>> +functions concurrently with @code{setenv}, @code{putenv},
>> +@code{unsetenv} or direct modifications of the global environment data
>> +structures is ill-advised; external concurrency control must be
>> +introduced by callers of these environment-modifying and
>> +@code{envromt}-marked functions.
>> +
>> +Functions that modify the environment are also marked with
>> +@code{envromt}, but they are not MT-Safe for the reasons above.  Since
>> +all environment-modifying functions are MT-Unsafe, functions that only
>> +access the environment are marked as MT-Safe when no other safety issue
>> +applies.

> I know you spelled the exception out here, but I'm wondering whether
> everyone will remember this when seeing MT-Safe somewhere.  Why didn't
> you use a different MT-Safe tag (eg, MT-Safe-Env) for those functions,
> but instead merged them with the "true" MT-Safe?

Same reason as glocale, really: once all methods to modify something are
decreed unsafe to call in MT programs, this something becomes constant
in conformant MT programs, and therefore the functions are indeed
perfectly safe to call.

It occurs to me that it might be useful to distinguish between read-only
and read-write cases, for example, if there's a 1stcall mark: if you
call the function in ST mode so as to eliminate that safety issue, does
envromt indicate the function remains unsafe to call, or that it just
reads from the environment so that it's fine to call after a first
single-threaded call.

>> +
>> +@item @code{oncesafe}
>> +@cindex oncesafe
>> +
>> +Functions marked with @code{oncesafe} use the internal @code{libc_once}
>> +machinery or similar to initialize internal 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.

> If "oncesafe" marks cases where a deadlock can happen, shouldn't it be
> called "onceunsafe" or something like this instead?

Read it aloud, collapsing the final phoneme from once with the first
from safe ;-)  I tried other spellings such as wunsafe, but this one
didn't bring libc_once to mind.  It's to be understood as unsafe because
libc_once is unsafe; can you suggest an alternate keyword or spelling?

>> +@item @code{1stcall}
>> +@cindex 1stcall
>> +
>> +Functions marked with @code{1stcall} perform thread-unsafe
>> +initialization when they are first called.

> I agree with Carlos on this (ie, it sounds as if it's better to be
> listed as an exception to MT-Safe

But...  That's what it is!  It's MT-unsafe (1stcall).

> MT-Safe | oncesafe | 1stcall

oncesafe is about libc_once initialization.  libc_once is MT-Safe; so
oncesafe is a note for AS-Unsafe and AC-Unsafe.  1stcall is for
non-libc_once initializations that are not even MT-Safe.

> with the definition above, you need to add that there could be
> other restrictions additionally).

This is the case of all notes; there's no reason to single this one out.

>> +Functions marked with @code{uunguard} modify non-atomically arguments or
>> +global objects that other functions access without synchronization.  To
>> +ensure MT- and AS-Safe behavior, callers should refrain from calling
>> +so-marked functions concurrently with readers of those objects.  A
>> +consequence of regarding modifiers of these objects as unsafe is that
>> +the covered objects can be regarded as constant (subject to the
>> +observation of safety constraints), so that all readers can be
>> +considered safe in this regard.

> AFAIU, uunguard are functions that if called, override the safety
> properties of MT-Safe or AS-Safe functions?

They're functions that, if called in MT programs, cause undefined
behavior, like any MT-Unsafe functions.

uunguard is a catch-all general case for envromt and glocale, discussed
earlier in this message.

> Functions marked with @code{uunguard} modify arguments or global objects
> that other, potentially MT-Safe or AS-Safe, functions access without
> synchronization.  Thus, calls to so-marked functions potentially make
> all concurrent calls to otherwise MT-Safe or AS-Safe functions MT-Unsafe
> and/or AS-Unsafe.

That's a consequence of calling any MT-Unsafe function.  No reason to
single this one out, is there?

> There should also be a link in the MT-Safe and AS-Safe paragraphs to
> this paragraph here, because both depend on this exception (ie, it's
> only MT-Safe if no uunguard is called concurrently).

This reasoning is backwards.  Those other functions *are* MT-Safe
*because* MT-Unsafe functions will not be called.  If they are, all bets
are off in very many different ways.


>> +@item @code{xguargs}
>> +@cindex xguargs
>> +
>> +Functions marked with @code{xguargs} may use or modify objects passed
>> +(indirectly) as arguments, without any guards to guarantee consistency.
>> +To ensure MT- and AS-Safe behavior, callers must ensure that the objects
>> +passed in are not accessed or modified concurrently by other threads or
>> +signal handlers.
>> +
>> +This mark is only applied with regard to an object when the
>> +@code{uunguard} mark is not applied because of the same object, and the
>> +object is opaque or not intended to be modified by code outside of
>> +@theglibc{}.

> Wouldn't you need to apply the mark only if the object is not opaque and
> might be considered to be fine to be modified outside of glibc?

No, the rationale you quoted explains why *this* note covers the opaque
or presumed-constant objects.

> When specifying this more formally, I believe we need to be careful to
> specify the "lifetime" of this constraint (eg, is it just for the
> duration of this call?).

The description in the first paragraph permits this reading, yeah.
E.g., functions that hold pointers to passed-in (sub)objects and later
on, asynchronously, proceed to modify them, would be marked with a
broader, bigger red flag ;-)

>> +@item @code{tempsig}
>> +@cindex tempsig

>> +Functions marked with @code{tempsig} may temporarily install signal
>> +handlers for internal purposes, which may interfere with other uses of
>> +those signals.  

>> +This makes such functions MT-Unsafe and AS-Unsafe to begin with.

> Why does it make such functions MT-Unsafe in every case?

Err...  Because they may interfere with other uses of those signals, and
there's not much one can do to avoid this interference.  Calling the
function concurrent from two different threads is a no-no.  Heck, since
signals are not thread-aware, it's not even safe to call the function in
a single thread; the signal it expects may be delivered to a different
thread, failing to fulfill its purpose.

> Spelling: cummulative -> cumulative, I believe.

Thanks.

> Given that we control the allocator, I believe that it can be made
> atomic (without disabling signals or reentrancy).

That's not enough; we'd have to make *storing* the address of the
allocated memory into its destination an atomic part of the allocation
atomic unit, otherwise memory would still leak after we returned it, if
cancellation occurred before the returned value was stored in the
destination.  The existing APIs just doesn't make memleak-safe
implementations possible.

>> +Functions annotated with @code{lockleak} may leak locks if asynchronous
>> +thread cancellation interrupts their execution.

> So this is not a leaking lock, but a non-cleaned-up acquisition of a
> lock?  "lockacquisitionleak" is longer, but might be more clear?  Or
> "lockacqleak"?

They take a lock and don't release it, so the taken lock leaks.  I
understand the distinction you make, but I fail to see what good would
come out of this sort of hair splitting.  Like, what else might lockleak
be expected to mean?

>> +@item @code{asynconsist}
>> +@cindex asynconsist
>> +
>> +Functions marked with @code{asynconsist} take a recursive lock to ensure
>> +MT-Safety while accessing or modifying data structures guarded by the
>> +lock.

> Should this be "asyncconcist" (ie, don't drop one of the "c"'s)?
> I was reading "async-on-sist" and wondering what it means...

It's a partial overlap of async (signal) and inconsist (for
inconsistency).  Doubling the c wouldn't make it any less misleading ;-)

>> +Functions marked with @code{incansist} modify data structures in a
>> +non-atomic way.

> What does "incansist" stand for?

potential
inconsistency
   ^
if |
 *can*celled


The difficulties in guessing the precise meanings of the keywords, the
risk of their misleading as to the issue they refer to, and even my own
difficulty in keeping the precise meanings in mind and avoiding their
drift over time might indicate that made-up keywords weren't such a
great idea, and we might be better off with some numerical rather than
symbolic code.  Personally, I prefer symbols over numbers, but then, I'm
biased ;-)  Your thoughts?


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer