This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [MTASCsft PATCH WIP6 01/33] MT-, AS- and AC-Safety docs
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Alexandre Oliva <aoliva at redhat dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Wed, 22 Jan 2014 15:03:38 -0500
- Subject: Re: [MTASCsft PATCH WIP6 01/33] MT-, AS- and AC-Safety docs
- Authentication-results: sourceware.org; auth=none
- References: <ortxelb5zd dot fsf at livre dot home> <52D6162F dot 50204 at redhat dot com> <ork3dzh36r dot fsf at livre dot home>
On 01/16/2014 07:17 PM, Alexandre Oliva wrote:
> 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.
Thanks.
>> @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.
Right, and I think it's OK to have all one bullet with 3 entries.
> 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
That's fine except that `@item\nText' still gives a bullet without any
text beside it and that confusingly looks like missing text.
>>> +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 ;-)
It would not help to add periods. You'd have to duplicate a lot of text.
Please leave it as is for now.
>>> +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.
Thanks.
>> At present there is no dlopen documentation.
>
>> Is this a placeholder of all dlopen uses?
>
> Yeah.
Perfect.
>> 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.
OK.
>> 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.
Perfect.
>>> +@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.
Add this to the comment :-)
>>> +@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.
Thanks. The goal here is to harmonize our language with POSIX.
>>> +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.
I meant (a).
I don't see the need to clarify and it simplifies the wording.
>>> +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.
Great work splitting that into multiple 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.
s/in which//g
s/in the normal flow//g
> 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.
s/normal flow/it/g
OK with those simplifications.
>>> +@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.
I like "!posix" :-)
> 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â :-)
Almost there.
Cheers,
Carlos.