Re: [PATCH 4/5] [v2] Add manual documentation for threads.h

[Torvald Riegel <triegel at redhat dot com>]

Disclaimer:  I haven't had time to look through the actual
documentation, but want to comment nonetheless early so that we agree on
the general direction.

On Tue, 2015-08-11 at 14:06 -0400, Zack Weinberg wrote:
> It's very important to minimize forward references in documentation.
> That is, if someone starts reading at the beginning of a chapter, by
> the time they get to 'foo' they should already have encountered all
> the new concepts from that chapter that are necessary to understand
> 'foo'.

Agreed that this makes it easier, but understanding a concept does not
mean that one knows about all possible instances of it.  Specifically,
it's perfectly fine to introduce the concept of a Threading Result Code
without actually enumerating all the specific codes.  Putting the
enumeration further down in the manual can even be easier because the
codes aren't central to the understanding, I'd claim -- users would be
interested in the functions first, and then care about error conditions.

> General editorial comments:
> 
> We do not appear to have any documentation of C11 atomics right now,
> and that means you need to avoid *talking* about C11 atomics or
> anything that you need to know C11 atomic terminology to understand.
> All the "synchronizes-with" stuff -- just delete it.

I disagree.  First, synchronizes-with is not tied C11 atomics, but to
the C11 memory model.  The semantics of atomics are described based on
the memory model.

If we try to document the semantics of the thread functions at all, we
need to assume the reader is familiar with the memory model.  Otherwise,
we just shouldn't document any of the synchronization semantics (at
which point we can just omit the docs completely I'd say).


More generally, I'm also wondering why we even try to document semantics
that are (supposed to be) documented perfectly by C11.  I agree that the
C11 specs aren't as precise as we'd want them to be, but then we need to
make it clear that whatever we're documenting is just illustrative and
not the authorative spec (which is C11), or covers
implementation-defined behavior, or covers our interpretation of C11.

> I'm cross-checking your work with C11 (N1570) and there are a bunch of
> places where the standard doesn't specify the behavior as precisely as
> I would like, which is really quite unfortunate; we should think about
> the extent to which we want to pin down unspecified cases.

If we do so, we should also look at C++14.  It specifies the
synchronization stuff more thoroughly, and arguably C++ and C should be
aligned where possible.

> > +@deftypefun int mtx_trylock (mtx_t *@var{mutex})
> > +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> > +Tries to lock the mutex pointed to by @var{mutex} without blocking.
> > +Returns immediately if the mutex is already locked.
> 
> "Returns thrd_success if it locked the mutex, or if this thread has
> already locked the mutex.  (This does not count as a recursive lock.)
> Returns thrd_busy if another thread holds the lock."
> 
> (Does the case where the lock is already held indeed not count as a
> recursive lock?  That could be extremely bad.)

I believe calling mtx_trylock on a nonrecursive mutex is undefined
behavior.  It's not specifically called out in C (but is in C++), but
mtx_lock makes that requirement.

> > +@deftypefun int mtx_unlock (mtx_t *@var{mutex})
> > +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> > +Unlocks the mutex pointed to by @var{mutex}. The behavior is undefined
> > +if the mutex is not locked by the calling thread. This function
> > +synchronizes-with subsequent @code{mtx_lock}, @code{mtx_trylock},
> > +or @code{mtx_timedlock} on the same mutex. All lock/unlock
> > +operations on
> > +any given mutex form a single total order (similar to the
> > +modification order of an atomic).

The sc relation for sequential consistency is a better similarity.  But
we really should just point to C11 here instead of inventing our own
wording.

> > +@end deftypefun
> 
> If you have recursively locked a mutex N times, do you need to unlock
> it N times also?  (I see that C11 does not say.)

Yes, you have to.  If C11 should say that, this should be treated as a
C11 defect.

> > +Mutexes are not the only synchronization mechanisms available. For some
> > +more complex tasks, @theglibc{} also implements condition variables,
> > +that allow the user to think in a higher level to solve possible
> > +mutual exclusion issues.
> 
> This paragraph doesn't make any sense.

Agreed, it's incorrect.  condvars are not a mutual exclusion mechanism.

> > +@deftypevr Macro {} TSS_DTOR_ITERATIONS
> > +Integer constant expression representing the maximum number of
> > +times that destructors will be called when a thread terminates.
> > +@end deftypevr
> 
> This also doesn't make any sense.  I see that this text was copied
> verbatim from C11, but that isn't an excuse :)

Well, copying C11 is better than inventing our own semantics that might
diverge from C11.  Thus, the way forward is to clarify this upstream
first, and then specify what upstream does -- and upstream is ISO C in
this case.

> Ideally there would be something in here about when you should use
> thread_local versus tss_*.
> 
> > +@deftypefun int tss_create (tss_t* @var{tss_key}, tss_dtor_t
> @var{destructor})
> 
> When would the destructor be called?
> 
> > +The ISO C11 specification also provides new error types that belong
> > +specifically to @code{threads.h}. @Theglibc{} has also implemented
> > +this feature and every function in this API always returns one of the
> > +following error codes:
> 
> Does that mean none of them ever set errno?
> 
> > +@item thrd_error
> > +Value returned by a function to indicate that the requested operation
> > +failed.
> 
> Gosh, that's vague.

But that may be the right description, given that it's a catch-all code.