This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ed on Mar 30 2015, 7:35 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

ed updated this revision to Diff 22881.Mar 30 2015, 7:35 AM
ed retitled this revision from to libc++: Make support for thread-unsafe C functions optional.
ed updated this object.
ed edited the test plan for this revision. (Show Details)
ed added reviewers: jroelofs, mclow.lists.
ed set the repository for this revision to rL LLVM.
ed added a subscriber: Unknown Object (MLST).
EricWF added a subscriber: EricWF.Mar 30 2015, 7:50 AM

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 added a comment.Mar 30 2015, 11:29 AM

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?

In D8703#149021, @ed 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 updated this revision to Diff 22954.Mar 31 2015, 6:56 AM
ed updated this revision to Diff 22955.
ed removed rL LLVM as the repository for this revision.
ed added a comment.Mar 31 2015, 6:59 AM

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.

emaste added a subscriber: emaste.Apr 9 2015, 12:52 PM
ed added a comment.May 10 2015, 1:30 PM

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

jroelofs added a subscriber: jroelofs.

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

mclow.lists edited edge metadata.Jun 23 2015, 8:32 AM

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 ↗(On Diff #22955)

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

This revision was automatically updated to reflect the committed changes.
ed added a comment.Jun 24 2015, 1:50 AM

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.