Re: [PATCH 2/2] manual: Document __libc_single_threaded

[Rich Felker <dalias@libc.org>]

On Wed, May 27, 2020 at 01:54:01PM +0200, Florian Weimer wrote:
> * Rich Felker:
> 
> > On Mon, May 25, 2020 at 10:08:37AM +0200, Florian Weimer via Libc-alpha wrote:
> >> * Rich Felker:
> >> 
> >> >> this can allow earlier single threaded detection than only
> >> >> considering pthread_join: e.g. stdio, malloc etc may do a
> >> >> check and update the global after an acquire barrier, however
> >> >> the compiler must not cache globals across libc calls for this
> >> >> to work.
> >> >
> >> > It can't cache globals across non-pure functions whose definitions it
> >> > cant't see (and if it saw the definition it would know the global is
> >> > modified).
> >> 
> >> Sorry about that, hit C-c C-c while I thought I was in a terminal. 8-/
> >> 
> >> For most standard C functions, it is well-known to which global
> >> variables (if any) they write.  Of course, compilers exploit this fact.
> >> 
> >> > malloc is something of a special case where clang treats it
> >> > not as a function but having "pure malloc semantics", but even then I
> >> > don't think it matters if it caches it;
> >> 
> >> And of course malloc is the most common example of a standard function
> >> that has observable side effects beyond those specified in the standard:
> >> most implementations have a statistics interface.
> >> 
> >> > at worst you see the old value of __libc_single_threaded (false)
> >> > rather than the new one (true) and that direction is safe.
> >> 
> >> It's still a data race.  The compiler can easily generate invalid code
> >> if it incorrectly assumes that __libc_single_threaded remains stable.  I
> >> don't know if Clang will do this.  But I think the C library
> >> implementation should be rather conservative here.
> >
> > If this is an issue, and even regardless of whether it is, I think the
> > type of __libc_single_threaded should be volatile qualified. This
> > ensures that it cannot be cached in any way that might be invalid.
> > That's not just a hack; it's the correct way to model that the value
> > is able to change asynchronously (such as by an operation that the
> > compiler would otherwise assume can't have side effects).
> 
> I think it makes more sense not to declare the object as volatile and
> make sure that only libc functions which imply the required barrier
> write to __libc_single_threaded.  For instance, I expect that this will
> allow compilers to generate tighter code around multiple (implied)
> reference count updates.

It really should be volatile just because whatever you make it is ABI,
and there might be good reasons to want to make it updated
"asynchronously" with respect to the compiler's model in the future.
If you don't make it volatile now you can't do that in the future.

The cost of volatile is rather trivial; compiler can still cache
address of it, and loading from a cache line you recently loaded from,
and to which no stores are happening, is virtually free.

Rich