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

[Zack Weinberg <zackw at panix dot com>]

On 08/11/2015 09:33 AM, Juan Manuel Torres Palma wrote:
> This patch updates the manual and adds a new chapter to the manual,
> explaining types macros, constants and functions defined by ISO C11
> threads.h standard.

This has some substantive issues and some organizational problems.
Let me first outline a better organization, and then I'll run through
the whole thing pointing out the substantive issues as they pop up.

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'.  In this case, what that mostly means is the result codes need
to come first.  I also strongly recommend separating 'call_once' and
its associated constants and types to its own section -- right now
it's smeared over the mutex section and the thread-local storage
section, which is just weird.  So it should be something like this:

ISO C11 Threading
  Threading Result codes (current "C11 error types")
    thrd_success
    thrd_error
    thrd_timedout
    thrd_busy
    thrd_nomem

  Thread Creation and Control (current "Thread")
    thrd_t
    thrd_start_t
    thrd_current
    thrd_equal
    thrd_create
    thrd_exit
    thrd_join
    thrd_detach
    thrd_sleep
    thrd_yield

  Call Once (new)
    once_flag
    ONCE_FLAG_INIT
    call_once
    [an example]

  Thread-Local Storage
    thread_local
    tss_t
    tss_dtor_t
    TSS_DTOR_ITERATIONS
    tss_create
    tss_delete
    tss_get
    tss_set

  Mutexes
    mtx_t
    mtx_init
      mtx_plain  (* move the explanation of these constants inside
      mtx_timed     the @deftypefun for mtx_init *)
      mtx_recursive
    mtx_lock
    mtx_timedlock
    mtx_trylock
    mtx_unlock

  Condition Variables
    cnd_t
    cnd_init
    cnd_destroy
    cnd_wait
    cnd_timedwait
    cnd_signal
    cnd_broadcast

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 would be
making a different recommendation if I were asking you to write a full
guide to threads, but since we don't even have complete reference
documentation on POSIX threads right now, that seems like too much
extra work to put on you.)

Make sure everything appears in the appropriate index.

Widespread substantive issues:

You need to explain what the return value means for *every single
function* even if you think it's obvious.  (Having moved the
result codes up front, "This function returns a threading result code"
will often, but not always, be sufficient -- e.g. that would not be
enough for mtx_trylock and mtx_timedlock.)

The @safety{} annotations appear to be wrong across the board.  Most
of these functions probably are indeed multithread-safe (or they'd be
useless!), but they probably *aren't* either async-signal or
async-cancel safe.  Please recheck every single one.

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.

@deftp clauses for cnd_t, mtx_t, tss_t, tss_dtor_t, and once_flag are
missing.

Specific substantive issues:

> +@deftp {Data Type} {thrd_t}
> +Unique number that identifies a thread
> +unequivocally.
> +@end deftp

This should be described as an "opaque object", not a "number".

> +@deftp {Data Type} {thrd_start_t}
> +It is a function pointer that is passed to @code{thrd_create} when
> +creating a new thread. Should point to the first function that thread
> +will run.
> +@end deftp

This, you need to specify the exact definition of, (int (*)(void *)),
so people know how to write their thread-start procedures.

> +@deftypefun int thrd_equal (thrd_t @var{lhs}, thrd_t @var{rhs})
> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> +Checks whether @var{lhs} and @var{rhs} refer to the same thread.
> +@end deftypefun

This makes a nice example of how specific you need to be, everywhere,
about the return value:

"Returns a nonzero value if @var{lhs} and @var{rhs} refer to the same
thread, or zero if they don't."

> +@deftypefun int thrd_sleep (const struct timespec* @var{time_point},
struct timespec* @var{remaining})
> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> +Blocks the execution of the current thread for at least until
> +the TIME_UTC based time point pointed to by @var{time_point} has been
> +reached.

C11 says this function takes a *duration*, not an absolute time
point.  What did you actually implement?  Need to explicitly point out
that this is not the same as mtx_timedlock or cnd_timedwait.

TIME_UTC is not documented, say "wall-clock time" instead.

> +@deftypefun _Noreturn void thrd_exit (int @var{res})
> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> +Terminates execution of the calling thread and sets its result code
> +to @var{res}.
> +@end deftypefun

This needs to mention that returning from a thread-start procedure is
equivalent to calling thrd_exit, and that thrd_exit calls exit(0),
*regardless of the value of @var{res}*, if the calling thread is the
only thread in the process.

> +@deftypefun int thrd_detach (thrd_t @var{thr})
> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> +Detaches the thread identified by @var{thr} from the current
> +environment.
> +The resources held by the thread will be freed automatically once
> +the thread exits.
> +@end deftypefun

I know this is what the C standard says this does, but it's not
actually a helpful thing to say.  It would be better to say something
like "Informs the implementation that the program does not need any
sort of notification when @var{thr} exits."

> +@deftypefun int mtx_init (mtx_t* @var{mutex},int @var{type})
> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> +Creates a new mutex object with type @var{type}. The object pointed
> +to by @var{mutex} is set to an identifier of the newly created mutex.
> +@end deftypefun

As mentioned above, explain the various values @var{type} can take
*here*, not at the bottom of the section.

> +@deftypefun int mtx_timedlock (mtx_t *restrict @var{mutex}, const
struct timespec *restrict @var{time_point})
> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> +Blocks the current thread until the mutex pointed to by @var{mutex}
> +is locked or until the TIME_UTC based time point pointed to
> +by @var{time_point} has been reached.

Again, TIME_UTC is not documented, so just say "wall-clock time".
Point out specifically that this function differs from thr_sleep, in
that it takes an absolute time point rather than a duration.

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

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

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

> +@deftypefun int cnd_timedwait (cnd_t* restrict @var{cond}, mtx_t*
restrict @var{mutex}, const struct timespec* restrict @var{time_point})
> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> +Atomically unlocks the mutex pointed to by @var{mutex} and blocks on the
> +condition variable pointed to by @var{cond} until the thread is signalled
> +by @code{cnd_signal} or @code{cnd_broadcast}, or until the TIME_UTC based
> +time point pointed to by @var{time_point} has been reached. The mutex
> +is locked again before the function returns.
> +@end deftypefun

Same deal with TIME_UTC and being clear that this function, unlike
thrd_sleep, *does* take an absolute time.

> +@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 :)

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.

zw

Attachment: signature.asc
Description: OpenPGP digital signature