Error building libcxx with recent LLVM due to -mthread-model posix default

[sunfishcode]

In file included from wasi-sdk/src/llvm-project/libcxx/src/barrier.cpp:9:
wasi-sdk/build/libcxx/include/c++/v1/__config:1195:2: error: _LIBCPP_HAS_NO_THREADS cannot be set when __STDCPP_THREADS__ is set.
#error _LIBCPP_HAS_NO_THREADS cannot be set when __STDCPP_THREADS__ is set.
 ^

This appears to be related to how https://reviews.llvm.org/D58742 changed the configuration of clang to use -mthread-model posix by default, which now leads clang to predefine __STDCPP_THREADS__. That is then incompatible with our configuring libcxx with _LIBCPP_HAS_NO_THREADS, since libcxx doesn't like those two both being defined at the same time. But _LIBCPP_HAS_NO_THREADS is necessary here to avoid libcxx using features not supported by all relevant engines—atomics.

@tlively This now seems to be a case where defaulting to -mthread-model single would actually have the behavior we want here, and there aren't any other obvious options.

How difficult would it be to switch the clang wasm32 target back to -mthread-model single by default?

Alternatively, should we use custom target checks in libcxx to tell it that it's ok to disable threads support even if __STDCPP_THREADS__ is defined? Or, should we add custom checks to clang to avoid definint __STDCPP_THREADS__?

[tlively]

Hmm, that patch is nearly two years old. Is there a more recent change that caused this to stop working?

Who would have to add -mthread-model single to their command lines to work around this issue without any code changes? Is it just wasi-sdk builders or would it have to be every wasi-sdk user? (or both?)

Looking at that old patch description, I wrote:

The default configuration for the backend is thread-model=posix and no
atomics, which was previously an invalid configuration. This change
makes the default valid because the thread model is ignored.

Clearly if it has some effect on libcxx, the thread model is not being ignored, so I suspect that you're right that changing the default to -mthread-model single makes sense. I'll try to figure out if that change would have any effect on Emscripten users. If it doesn't, then I'll go ahead and make this change in clang. If it does, we can figure out the best path forward from there.

[sunfishcode]

Ah, it looks like __STDCPP_THREADS__ macro was added in https://reviews.llvm.org/D91747 a few months ago.

If I understand all the pieces here, "c++/v1/__config" is a header that gets included in end-user compiles, so as it currently stands every user would have to pass -mthread-model single to compile C++ when using a libcxx configured with _LIBCPP_HAS_NO_THREADS, as wasi-sdk's libcxx currently is.

[sbc100]

I wonder if __STDCPP_THREADS__ should instead be defined based on LangOptions.POSIXThreads in the same way that C equivalent (_REENTRANT) is?

[sbc100]

Also, it looks like the #error in question was already removed from libc++ in https://reviews.llvm.org/D92349 to solve this very issue.

[sunfishcode]

Ooh, so I may have just gotten unlucky with the revision of LLVM I was using here. Rebuilding with a newer revision now...

[sbc100]

Still, it seems wrong that __STDCPP_THREADS__ is defined while _REENTRANT is not.

[sunfishcode]

Looks good; my build with a newer LLVM revision succeeded.

[tlively]

Ok, I won't bother looking into anything here right now then.

[sunfishcode]

__STDCPP_THREADS__ is a C++11 standard macro. It's still not great for it to be predefined that in a mode that doesn't have threads. I looked into it, and found a way in clang to unset it, so I've now posted https://reviews.llvm.org/D96091.

So I think we can close this issue now.