This is an archive of the discontinued LLVM Phabricator instance.

[libcxxabi] Introduce an externally threaded libc++abi variant (take-2)
ClosedPublic

Authored by rmaprath on Dec 8 2016, 9:05 AM.

Diff Detail

Event Timeline

rmaprath updated this revision to Diff 80763.Dec 8 2016, 9:05 AM
rmaprath retitled this revision from to [libcxxabi] Introduce an externally threaded libc++abi variant (take-2).
rmaprath updated this object.
rmaprath added reviewers: EricWF, mclow.lists.
rmaprath added a subscriber: cfe-commits.
EricWF edited edge metadata.Dec 10 2016, 11:54 PM

This LGTM. I'll approve after the inline comments are addressed.

CMakeLists.txt
121

Maybe use a dependent option that sets it to the value of LIBCXX_ENABLE_THREADS when it is defined?

src/fallback_malloc.cpp
37

Can't we replace this class with std::mutex directly?

test/test_exception_storage.pass.cpp
42

What happened to the initializer?

rmaprath added inline comments.Dec 12 2016, 2:34 AM
CMakeLists.txt
121

Are you referring to CMAKE_DEPENDENT_OPTION macro? I don't see this macro used anywhere else in llvm, but I suppose that's not a concern?

Although, it would have to be something like:

CMAKE_DEPENDENT_OPTION(LIBCXXABI_HAS_EXTERNAL_THREAD_API "Externally Threaded libcxxabi" OFF  "LIBCXX_ENABLE_THREADS" OFF)

Because, LIBCXX_ENABLE_THREADS=ON should not mean LIBCXXABI_HAS_EXTERNAL_THREAD_API=ON.

src/fallback_malloc.cpp
37

Again, I should've included more context to the patch :(

The mutexor here is supposed to degrade to a nop when threading is disabled. I think we cannot use std::mutex without threading support.

Will update the patch with more context.

test/test_exception_storage.pass.cpp
42

I don't think it needs one here?

std::__licpp_thread_create() does not require std::__libcpp_thread_t argument to be statically initialized.

rmaprath updated this revision to Diff 81062.Dec 12 2016, 2:36 AM
rmaprath edited edge metadata.

Patch updated with more context.

Gentle ping.

EricWF accepted this revision.Dec 23 2016, 7:54 PM
EricWF edited edge metadata.

LGTM. I'll re-review the mutexor changes post-commit.

CMakeLists.txt
121

Ah OK. This looks good as is then.

Although it would be really nice if LIBCXX_HAS_EXTERNAL_THREAD_API=ON also enabled this option unless otherwise specified, but I don't see an easy way to do that.

src/fallback_malloc.cpp
37

We cannot use std::mutex without threading support but I would still rather use it.

I would typedef std::lock_guard<std::mutex> mutexor when threading is enabled and otherwise I would just #ifdef all usages away.

Also please add _LIBCPP_SAFE_STATIC to the heap_mutex declaration.

This revision is now accepted and ready to land.Dec 23 2016, 7:54 PM
rmaprath added inline comments.Dec 29 2016, 7:15 AM
src/fallback_malloc.cpp
37

Hmmm, using std::mutex here breaks the shared library builds of libcxxabi. The problem is that methods like std::mutex::lock() and ~std::mutex() are only declared in the <mutex> header, with the implementation going into the dylib.

I was just about to update the patch because it worked for all my other configurations, but those configurations are either static builds or ones that suppress the -Wl, -z defs linker option passed to the shared library builds by default (this is an inherited option).

Perhaps it's time to get rid of -Wl, -z defs for all the libcxxabi configurations? That makes sense if libcxxabi is inherently dependent on libcxx.

Why don't we commit this as is and we can deal with the mutex later?

This revision was automatically updated to reflect the committed changes.
src/fallback_malloc.cpp