Thread Safety Notes
I've been looking over the thread safe additions to localtime.c, and have a few comments. 1. Use of C's 'volatile' declaration is not sufficient to implement fully thread-safe double-checked locking. The complications are confusing and subtle. Here is a paper that goes into the question in depth: http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf The bottom line is that doing it right needs memory barrier operations - a read acquire and write release or equivalent for the flag variable being tested. Volatile does not guarantee these. C11 has the necessary functions, but is not yet widely available. [ http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1570.pdf ] Compilers mostly have non-standard extensions available that will do the job, often modeled after those in gcc. 2. The function gmtcheck() needs to recheck the gmt_is_set flag after acquiring the lock. As it stands, multiple threads could replicate the initialization, overwriting a previously set gmtptr in the process. This is unrelated to the volatile question. 3. The volatile variable lcl_is_set is referenced while not holding the lock in only one place, in localtime_r(). And from there the function localtime_tzset() is unconditionally called, which unconditionally acquires the lock. With minor refactoring, all access to lcl_is_set could be from code that is holding the lock, which would allow lcl_is_set to be an ordinary variable with no special thread safety considerations. If it were up to me, I would probably remove use of the double-checked idiom, and rely on solely lock(). Slightly slower, but simple, portable and safe. The alternative is to #define some compiler-dependent atomic operations. Thanks, -- Andy Heninger
Andy Heninger wrote:
If it were up to me, I would probably remove use of the double-checked idiom, and rely on solely lock(). Slightly slower, but simple, portable and safe.
Thanks very much for the review. Your suggestion makes gmtime 13% slower on my platform (Fedora 20 x86-64) when THREAD_SAFE is defined, due to the locking overhead, but safe is better than potentially-buggy. Attached are three proposed patches that should address the problems you raise. Patch 1 fixes the memory leak race in gmtcheck. Patch 2 should port to the C11 memory model. Patch 3 should work around the resulting gmtime slowdown in zdump by having zdump use localtime_rz instead of gmtime to convert time stamps to UTC.
participants (2)
-
Andy Heninger -
Paul Eggert