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

[Alexandre Oliva <aoliva at redhat dot com>]

On Jan 15, 2014, "Carlos O'Donell" <carlos@redhat.com> wrote:

> *
>   Text

> This is because of the newline between @item and the text.

*nod*, I'm taking the newlines out.


> @code{MT-Safe}

> Thread-Safe [...]

> Like you did with the other properties.

Unlike the sections that define safety keywords, in which each bullet
defines a separate term, in this list we have a single bullet for all of
the *-Unsafe terms, so I didn't think this would be appropriate.

I ended up changing it to:

@item
[...]
@code{MT-Safe} or Thread-Safe functions are safe to call in the presence
[...]
@item
[...]
@code{MT-Unsafe}, @code{AS-Unsafe}, @code{AC-Unsafe} functions are not


>> +In some cases, even expanding MT-Safe functions inline could cause
>> +combinations to become MT-Unsafe due to reordering that this could
>> +enable; this is never the case of @glibcadj{} functions defined in
>> +user-visible headers, because these are meant to be inlined, but it may
>> +be for functions that are exported by @glibcadj{} shared libraries;
>> +whole-program optimizations that might inline functions across library
>> +interfaces might expose this sort of problem, so performing inlining
>> +across the @glibcadj{} interface is not recommended, nor is the
>> +documented MT-Safety status guaranteed under such arrangements.

> OK, though that paragraph is a single sentence it makes and is hard to
> split without a lot of duplication.

How about replacing the semicolons with periods?  Would that help at
all?  (I wouldn't think so, but then, I don't even count the
semicolon-separated sentences as a single sentence ;-)



>> +AC-Safe or Async-Cancel-Safe functions are safe to call when
>> +asynchronous cancellation is enabled.  AC in AC-Safe stands for
>> +Asynchronous Cancellation.

> Do you think we need a little more here?

Hmm, I don't know.  If I had to add more, I'd probably explain the
cancellation modes, rather than list the mandated AC-Safe functions and
duplicate the preliminary status of the docs, but I wouldn't go as far
as objecting to the paragraph you proposed, so...  added.

> At present there is no dlopen documentation.

> Is this a placeholder of all dlopen uses?

Yeah.

> Can we make that clearer?

> Suggest the simpler:
> ~~~
> The locks are enough for these functions to be AS- and AC-Unsafe, but
> other issues may arise. At present this is a placeholder for all
> potential safety issues raised by @code{dlopen}.
> ~~~

Thanks, taken.

> s/nss/name service switch (NSS)/g.

> Since `nss' can actually be confused with `network security services'
> which glibc actually uses in some configurations to implement secure
> hashing.

> Use @cindex NSS?

'k.  I also mentioned character set conversion back-ends, with @cindex
iconv.


>> +@c But what if, instead of marking modifiers with const:id and readers
>> +@c with just id, we marked writers with race:id and readers with ro:id?

> This comment needs expanding. What would you gain by doing it this way?

Some clarity, I think.  Instead of having to define each instance of
âidâ, we'd have a general pattern governing all such âidâs, wherein
race:id would suggest the need for an exclusive/write lock to make the
function safe, whereas ro:id would indicate âidâ is expected to be
read-only, but if any modifiers are called (while holding an exclusive
lock), then ro:id-marked functions ought to be guarded with a read lock
for safe operation.  ro:env or ro:locale, for example, seems to convey
more clearly the expectations and the meaning, than just env or locale.


>> +@c fixme: at least sync cancellation should get it right, and would
>> +@c obviate the restoring bit below, and the qualifier above.

> What do you mean by "sync cancellation?" Deferred cancellation?

I meant sync as in the opposite of async; deferred is not a perfect
opposite IMHO.  E.g., I'd ragard
pthread_cancel(pthread_self()),pthread_testcancel() as immediate (rather
than deferred) synchronous cancellation, even though it happens to rely
on both async or deferred cancellation machinery to get immediate
cancellation.

In the context above, I guess it makes sense to write deferred, to avoid
having to define an additional term, since immediate synchronous
cancellation is not a possibility.  Both occurrences have been changed.


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

> s/, even synchronously//g.

Do you mean to take it out because (a) âcancelledâ covers both async and
deferred cancellation, (b) bugs in handling deferred cancellation are to
be fixed rather than turned into (mis)features by means of
documentation, or (c) because it was not clear that we have a bug that's
more serious than AC-Unsafe?  I assume you meant (a), but I'm concerned
people might fail to realize this is a case of C-Unsafe (even deferred
cancellation is mishandled), not just AC-Unsafe.



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

> Run on sentence. Please split this up into 2 or 3 sentences.

Functions that allocate or deallocate file descriptors will generally be
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 either: 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.


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

> This seems to violate the principle of least surprise in that "posix"
> seems like it should indicate POSIX compliance.

How about rendering it as !posix (to be read not posix)?  I've used
e.g. âwhatever/!bsdâ to indicate the whatever keyword applies to kernels
other than BSD's.


With this, I've completed the changes you've suggested.  Still pending
are the few questions above and the one in the separate email.

I have a few other small changes to integrate that I'd kept in separate
patches in the patchset; I'll post them shortly, so that we can complete
the review cycle and hopefully the next round of review of this patch
will yield an âok to installâ :-)

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