libc++: Make support for thread-unsafe C functions optional

[ed]

Summary

One of the aspects of CloudABI is that it aims to help you write code that is thread-safe out of the box. This is very important if you want to write libraries that are easy to reuse. For CloudABI we decided to not provide the thread-unsafe functions. So far this is working out pretty well, as thread-unsafety issues are detected really early on.

The following patch adds a knob to libc++, _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS, that can be set to disable thread-unsafe functions that can easily be avoided in practice. The following functions are not thread-safe:

• <clocale>: locale handles should be preferred over setlocale().
• <cstdlib>: mbrlen(), mbrtowc() and wcrtomb() should be preferred over their non-restartable counterparts.
• <ctime>: asctime(), ctime(), gmtime() and localtime() are not thread-safe. The first two are also deprecated by POSIX.

[EricWF]

The CMake and LIT stuff LGTM. However I would rather keep the directory structure in test/std the same as the structure of the C++ standard clauses. Could you rename the fail tests, put them in the correct directories and use // REQUIRES: libcpp-has-no-thread-unsafe-c-functions?

[ed]

In D8703#148896, @EricWF wrote:

The CMake and LIT stuff LGTM. However I would rather keep the directory structure in test/std the same as the structure of the C++ standard clauses. Could you rename the fail tests, put them in the correct directories and use // REQUIRES: libcpp-has-no-thread-unsafe-c-functions?

Sure! Just to make sure I understand it correctly: in this specific case, the tests should be called:

test/std/utilities/date.time/asctime.fail.cpp
test/std/utilities/date.time/ctime.fail.cpp
test/std/utilities/date.time/gmtime.fail.cpp
test/std/utilities/date.time/localtime.fail.cpp

As in, the same as they are right now, but without the no.thread.unsafe.c.functions component in between?

[EricWF]

In D8703#149021, @ed wrote:

In D8703#148896, @EricWF wrote:

The CMake and LIT stuff LGTM. However I would rather keep the directory structure in test/std the same as the structure of the C++ standard clauses. Could you rename the fail tests, put them in the correct directories and use // REQUIRES: libcpp-has-no-thread-unsafe-c-functions?

Sure! Just to make sure I understand it correctly: in this specific case, the tests should be called:

test/std/utilities/date.time/asctime.fail.cpp
test/std/utilities/date.time/ctime.fail.cpp
test/std/utilities/date.time/gmtime.fail.cpp
test/std/utilities/date.time/localtime.fail.cpp

As in, the same as they are right now, but without the no.thread.unsafe.c.functions component in between?

Pretty much, but I would make the test names more descriptive so they infer that these are failure tests for when we have no unsafe C functions. The test names don't *have* to follow the standard names.

[ed]

In D8703#149206, @EricWF wrote:

Pretty much, but I would make the test names more descriptive so they infer that these are failure tests for when we have no unsafe C functions. The test names don't *have* to follow the standard names.

Got it. What do you think about the following naming scheme? I've named them ${function}.thread-unsafe.fail.cpp.

[ed]

Friendly ping. Does this patch look all right to land? Thanks!

[jroelofs]

I'm deferring to @mclow.lists on this one.

[mclow.lists]

Except for the bit in locale.cpp, these changes look OK to me.

I'm concerned about the proliferation of "subsets" of the libc++ functionality (no threads, no filesystem, no thread-unsafe fns), but ...

src/locale.cpp

579

Unlike all the other changes, this is a behavior change.

[ed]

In D8703#192868, @mclow.lists wrote:

Except for the bit in locale.cpp, these changes look OK to me.

Thanks for the review!

Regarding the change to locale.cpp: CloudABI does not have setlocale(), which is why this call would need to be disabled. libc++ still has a global locale, but it's not the case that this is propagated to the C runtime.

I've reverted this part of the change for now, as we could argue it's not related to the change at hand. I suspect it may make more sense to #ifndef __CloudABI__ that part at one point.

CMakeLists.txt

 option(LIBCXX_ENABLE_STDIN "Build libc++ with support for stdin/std::cin." ON)
 option(LIBCXX_ENABLE_STDOUT "Build libc++ with support for stdout/std::cout." ON)
 option(LIBCXX_ENABLE_THREADS "Build libc++ with support for threads." ON)
+option(LIBCXX_ENABLE_THREAD_UNSAFE_C_FUNCTIONS "Build libc++ with support for thread-unsafe C functions" ON)
 option(LIBCXX_BUILD_32_BITS "Build 32 bit libc++" OFF)
 option(LIBCXX_ENABLE_MONOTONIC_CLOCK
   "Build libc++ with support for a monotonic clock.

                       " when LIBCXX_ENABLE_THREADS is also set to OFF.")
 endif()
 
+# LIBCXX_ENABLE_THREAD_UNSAFE_C_FUNCTIONS configuration
+if (NOT LIBCXX_ENABLE_THREAD_UNSAFE_C_FUNCTIONS)
+  add_definitions(-D_LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS)
+endif()
+
 # Configure for sanitizers. If LIBCXX_BUILT_STANDALONE then we have to do
 # the flag translation ourselves. Othewise LLVM's CMakeList.txt will handle it.
 if (LIBCXX_BUILT_STANDALONE)

include/__config

 #define _LIBCPP_PROVIDES_DEFAULT_RUNE_TABLE
 #endif
 
+// Thread-unsafe functions such as strtok(), mbtowc() and localtime()
+// are not available.
+#ifdef __CloudABI__
+#define _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
+#endif
+
 #endif  // _LIBCPP_CONFIG

include/clocale

 _LIBCPP_BEGIN_NAMESPACE_STD
 
 using ::lconv;
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
 using ::setlocale;
+#endif
 using ::localeconv;
 
 _LIBCPP_END_NAMESPACE_STD

include/cstdlib

 #undef lldiv
 using ::lldiv;
 #endif // _LIBCPP_HAS_NO_LONG_LONG
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
 using ::mblen;
 using ::mbtowc;
 using ::wctomb;
+#endif
 using ::mbstowcs;
 using ::wcstombs;
 #ifdef _LIBCPP_HAS_QUICK_EXIT

include/cstring

 inline _LIBCPP_INLINE_VISIBILITY       char* strstr(      char* __s1, const char* __s2) {return ::strstr(__s1, __s2);}
 #endif
 
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
 using ::strtok;
+#endif
 using ::memset;
 using ::strerror;
 using ::strlen;

include/ctime

 using ::difftime;
 using ::mktime;
 using ::time;
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
 using ::asctime;
 using ::ctime;
 using ::gmtime;
 using ::localtime;
+#endif
 using ::strftime;
 
 _LIBCPP_END_NAMESPACE_STD

src/locale.cpp

     locale& g = __global();
     locale r = g;
     g = loc;
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
     if (g.name() != "*")
         setlocale(LC_ALL, g.name().c_str());
+#endif
     return r;
 }
 

test/CMakeLists.txt

   pythonize_bool(LIBCXX_ENABLE_STDIN)
   pythonize_bool(LIBCXX_ENABLE_STDOUT)
   pythonize_bool(LIBCXX_ENABLE_THREADS)
+  pythonize_bool(LIBCXX_ENABLE_THREAD_UNSAFE_C_FUNCTIONS)
   pythonize_bool(LIBCXX_ENABLE_MONOTONIC_CLOCK)
   # The tests shouldn't link to any ABI library when it has been linked into
   # libc++ statically.

test/libcxx/test/config.py

         elif not enable_monotonic_clock:
             self.lit_config.fatal('enable_monotonic_clock cannot be false when'
                                   ' enable_threads is true.')
+        self.configure_compile_flags_no_thread_unsafe_c_functions()
+
         # Use verbose output for better errors
         self.cxx.flags += ['-v']
         sysroot = self.get_lit_conf('sysroot')

         self.cxx.compile_flags += ['-D_LIBCPP_HAS_NO_THREADS']
         self.config.available_features.add('libcpp-has-no-threads')
 
+    def configure_compile_flags_no_thread_unsafe_c_functions(self):
+        enable_thread_unsafe_c_functions = self.get_lit_bool(
+            'enable_thread_unsafe_c_functions', True)
+        if not enable_thread_unsafe_c_functions:
+            self.cxx.compile_flags += [
+                '-D_LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS']
+            self.config.available_features.add(
+                'libcpp-has-no-thread-unsafe-c-functions')
+
     def configure_compile_flags_no_monotonic_clock(self):
         self.cxx.compile_flags += ['-D_LIBCPP_HAS_NO_MONOTONIC_CLOCK']
         self.config.available_features.add('libcpp-has-no-monotonic-clock')

test/lit.site.cfg.in

 config.enable_stdin             = "@LIBCXX_ENABLE_STDIN@"
 config.enable_stdout            = "@LIBCXX_ENABLE_STDOUT@"
 config.enable_threads           = "@LIBCXX_ENABLE_THREADS@"
+config.enable_thread_unsafe_c_functions = "@LIBCXX_ENABLE_THREAD_UNSAFE_C_FUNCTIONS@"
 config.enable_monotonic_clock   = "@LIBCXX_ENABLE_MONOTONIC_CLOCK@"
 config.cxx_abi                  = "@LIBCXX_CXX_ABI_LIBNAME@"
 config.use_sanitizer            = "@LLVM_USE_SANITIZER@"

test/std/depr/depr.c.headers/stdlib_h.pass.cpp

     static_assert((std::is_same<decltype(div(0,0)), div_t>::value), "");
     static_assert((std::is_same<decltype(ldiv(0L,0L)), ldiv_t>::value), "");
     static_assert((std::is_same<decltype(lldiv(0LL,0LL)), lldiv_t>::value), "");
-    static_assert((std::is_same<decltype(mblen("",0)), int>::value), "");
     wchar_t* pw = 0;
     const wchar_t* pwc = 0;
     char* pc = 0;
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
+    static_assert((std::is_same<decltype(mblen("",0)), int>::value), "");
     static_assert((std::is_same<decltype(mbtowc(pw,"",0)), int>::value), "");
     static_assert((std::is_same<decltype(wctomb(pc,L' ')), int>::value), "");
+#endif
     static_assert((std::is_same<decltype(mbstowcs(pw,"",0)), size_t>::value), "");
     static_assert((std::is_same<decltype(wcstombs(pc,pwc,0)), size_t>::value), "");
 }

test/std/depr/depr.c.headers/string_h.pass.cpp

     static_assert((std::is_same<decltype(strrchr(cp, 0)), char*>::value), "");
     static_assert((std::is_same<decltype(strspn(cpc, cpc)), size_t>::value), "");
     static_assert((std::is_same<decltype(strstr(cp, cpc)), char*>::value), "");
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
     static_assert((std::is_same<decltype(strtok(cp, cpc)), char*>::value), "");
+#endif
     static_assert((std::is_same<decltype(memset(vp, 0, s)), void*>::value), "");
     static_assert((std::is_same<decltype(strerror(0)), char*>::value), "");
     static_assert((std::is_same<decltype(strlen(cpc)), size_t>::value), "");

test/std/language.support/support.runtime/cstdlib.pass.cpp

     static_assert((std::is_same<decltype(std::div(0LL,0LL)), std::lldiv_t>::value), "");
     static_assert((std::is_same<decltype(std::ldiv(0L,0L)), std::ldiv_t>::value), "");
     static_assert((std::is_same<decltype(std::lldiv(0LL,0LL)), std::lldiv_t>::value), "");
-    static_assert((std::is_same<decltype(std::mblen("",0)), int>::value), "");
     wchar_t* pw = 0;
     const wchar_t* pwc = 0;
     char* pc = 0;
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
+    static_assert((std::is_same<decltype(std::mblen("",0)), int>::value), "");
     static_assert((std::is_same<decltype(std::mbtowc(pw,"",0)), int>::value), "");
     static_assert((std::is_same<decltype(std::wctomb(pc,L' ')), int>::value), "");
+#endif
     static_assert((std::is_same<decltype(std::mbstowcs(pw,"",0)), std::size_t>::value), "");
     static_assert((std::is_same<decltype(std::wcstombs(pc,pwc,0)), std::size_t>::value), "");
 }

test/std/language.support/support.runtime/ctime.pass.cpp

     static_assert((std::is_same<decltype(std::difftime(t,t)), double>::value), "");
     static_assert((std::is_same<decltype(std::mktime(&tm)), std::time_t>::value), "");
     static_assert((std::is_same<decltype(std::time(&t)), std::time_t>::value), "");
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
     static_assert((std::is_same<decltype(std::asctime(&tm)), char*>::value), "");
     static_assert((std::is_same<decltype(std::ctime(&t)), char*>::value), "");
     static_assert((std::is_same<decltype(std::gmtime(&t)), std::tm*>::value), "");
     static_assert((std::is_same<decltype(std::localtime(&t)), std::tm*>::value), "");
+#endif
     char* c1 = 0;
     const char* c2 = 0;
     static_assert((std::is_same<decltype(std::strftime(c1,s,c2,&tm)), std::size_t>::value), "");

test/std/localization/c.locales/clocale.pass.cpp

 #include <clocale>
 #include <type_traits>
 
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
+
 #ifndef LC_ALL
 #error LC_ALL not defined
 #endif

 #error LC_TIME not defined
 #endif
 
+#endif // !_LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
+
 #ifndef NULL
 #error NULL not defined
 #endif

 int main()
 {
     std::lconv lc;
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
     static_assert((std::is_same<decltype(std::setlocale(0, "")), char*>::value), "");
+#endif
     static_assert((std::is_same<decltype(std::localeconv()), std::lconv*>::value), "");
 }

test/std/strings/c.strings/cstring.pass.cpp

     static_assert((std::is_same<decltype(std::strspn(cpc, cpc)), std::size_t>::value), "");
 //    static_assert((std::is_same<decltype(std::strstr(cpc, cpc)), const char*>::value), "");
     static_assert((std::is_same<decltype(std::strstr(cp, cpc)), char*>::value), "");
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
     static_assert((std::is_same<decltype(std::strtok(cp, cpc)), char*>::value), "");
+#endif
     static_assert((std::is_same<decltype(std::memset(vp, 0, s)), void*>::value), "");
     static_assert((std::is_same<decltype(std::strerror(0)), char*>::value), "");
     static_assert((std::is_same<decltype(std::strlen(cpc)), std::size_t>::value), "");

test/std/utilities/date.time/no.thread.unsafe.c.functions/asctime.fail.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <ctime>
+
+int main() {
+    // asctime is not thread-safe.
+    std::time_t t = 0;
+    std::asctime(&t);
+}

test/std/utilities/date.time/no.thread.unsafe.c.functions/ctime.fail.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <ctime>
+
+int main() {
+    // ctime is not thread-safe.
+    std::time_t t = 0;
+    std::ctime(&t);
+}

test/std/utilities/date.time/no.thread.unsafe.c.functions/gmtime.fail.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <ctime>
+
+int main() {
+    // gmtime is not thread-safe.
+    std::time_t t = 0;
+    std::gmtime(&t);
+}

test/std/utilities/date.time/no.thread.unsafe.c.functions/lit.local.cfg

+if 'libcpp-has-no-thread-unsafe-c-functions' not in config.available_features:
+    config.unsupported = True

test/std/utilities/date.time/no.thread.unsafe.c.functions/localtime.fail.cpp

+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <ctime>
+
+int main() {
+    // localtime is not thread-safe.
+    std::time_t t = 0;
+    std::localtime(&t);
+}

test/std/utilities/date.time/tested_elsewhere.pass.cpp

     static_assert((std::is_same<decltype(std::difftime(t,t)), double>::value), "");
     static_assert((std::is_same<decltype(std::mktime(&tm)), std::time_t>::value), "");
     static_assert((std::is_same<decltype(std::time(&t)), std::time_t>::value), "");
+#ifndef _LIBCPP_HAS_NO_THREAD_UNSAFE_C_FUNCTIONS
     static_assert((std::is_same<decltype(std::asctime(&tm)), char*>::value), "");
     static_assert((std::is_same<decltype(std::ctime(&t)), char*>::value), "");
     static_assert((std::is_same<decltype(std::gmtime(&t)), std::tm*>::value), "");
     static_assert((std::is_same<decltype(std::localtime(&t)), std::tm*>::value), "");
+#endif
     static_assert((std::is_same<decltype(std::strftime(str,s,"",&tm)), std::size_t>::value), "");
 }