llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.6k stars 11.82k forks source link

WASM backend cannot build libcxx - fails on AtomicLoad #34759

Closed llvmbot closed 6 years ago

llvmbot commented 6 years ago
Bugzilla Link 35411
Resolution FIXED
Resolved on Nov 28, 2017 03:41
Version trunk
OS Linux
Reporter LLVM Bugzilla Contributor
CC @sunfishcode

Extended Description

I have to say, it's taken quite a while to come up with the right toolchain, so that CMake is able to work with the libcxx build system!

However, the error I'm getting below seems to be a problem with the WASM backend, and isn't related to libcxx itself (I don't think). It looks like the __atomic_load_n builtin just doesn't work with the WASM backend.

Build error

Building CXX object lib/CMakeFiles/cxx_objects.dir//src/algorithm.cpp.obj In file included from /home/ncw/workspace/llvm/libcxx/src/algorithm.cpp:11: In file included from /home/ncw/workspace/llvm/libcxx/include/random:1646: /home/ncw/workspace/llvm/libcxx/include/istream:714:24: warning: comparison 'long' < -2147483648 is always false [-Wtautological-constant-compare] if (temp < numeric_limits::min())


/home/ncw/workspace/llvm/libcxx/include/istream:719:29: warning: comparison 'long' > 2147483647 is always false [-Wtautological-constant-compare]
            else if (__temp > numeric_limits<int>::max())
                     ~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
fatal error: error in backend: Cannot select: t19: i32,ch = AtomicLoad<Volatile LD1[bitcast (i32* @&#8203;_ZGVZNSt3__112__rs_defaultclEvE6__rs_g to i8*)](align=4)>
      t0, t25
  t25: i32 = WebAssemblyISD::Wrapper TargetGlobalAddress:i32<i32* @&#8203;_ZGVZNSt3__112__rs_defaultclEvE6__rs_g> 0
    t24: i32 = TargetGlobalAddress<i32* @&#8203;_ZGVZNSt3__112__rs_defaultclEvE6__rs_g> 0
In function: _ZNSt3__112__rs_defaultclEv
clang-6.0: error: clang frontend command failed with exit code 70 (use -v to see invocation)
clang version 6.0.0 (trunk 318652)
Target: wasm32-unknown-unknown-wasm
Thread model: posix
InstalledDir: /home/ncw/workspace/llvm/compile-root/bin
clang-6.0: note: diagnostic msg: PLEASE submit a bug report to http://llvm.org/bugs/ and include the crash backtrace, preprocessed source, and associated run script.
clang-6.0: note: diagnostic msg: 
********************

I could post the full source of the failing libcxx file, but I'll see first if I can distill something more minimal!

### How I configured libcxx ###

I have written some additions to Musl which allow it to build for Wasm. The C side of things is fine. Now I'm moving on to C++, building libcxx first.  I've installed the Wasm version of Musl to a sysroot, and also installed there the Clang configured to default to "--target=wasm32-unknown-unknown-wasm".

I configured libcxx as follows:

cmake -DCMAKE_TOOLCHAIN_FILE=~/workspace/llvm/toolchain-libcxx-bootstrap.cmake -DLLVM_PATH=../../llvm -DLIBCXX_CXX_ABI=libcxxabi -DLIBCXX_CXX_ABI_INCLUDE_PATHS=../../libcxxabi/include -DCMAKE_INSTALL_PREFIX=/home/ncw/workspace/llvm/compile-root -DLIBCXX_ENABLE_SHARED=OFF -DLIBCXX_ENABLE_THREADS=OFF -DLIBCXX_ENABLE_GLOBAL_FILESYSTEM_NAMESPACE=OFF -DLIBCXX_HAS_MUSL_LIBC=ON -DCMAKE_BUILD_TYPE=MinSizeRel

The CMake toolchain needs a number of hacks, because it runs clang++ to detect many different things, but we don't as yet have a working C++ compiler (since we're trying to build libcxx first). With some hacks, it does get itself off the ground however.
llvmbot commented 6 years ago

Great, thanks for that change, and for the pointers. Things are moving fast on this project!

Resolving as FIXED since my issue is sorted for now.

sunfishcode commented 6 years ago

r319101 enables -mthread-model single by default.

sunfishcode commented 6 years ago

Thanks for considering it. Since there's a workaround, it could be left until Atomics.

However, this does render C++ pretty-much unusable, since a function-local static is a construct that is widely-used even in single-threaded code.

I don't know when Atomics are arriving, but if it's more than a couple of months you might want to make Clang work out of the box with the C++ in the meantime.

That's a good point. It hasn't really been an issue until recently, but with everything that's happening recently, I think it does make sense to do this by default now.

As as aside, I'm currently hacking away at patches for some of the other C++ sticking points, like inlines (aka COMDAT support). All of these things that need to be implemented don't seem to be tracked here in bugzilla or in any of the "official" Wasm github projects I can find. Is there some place where this work is actually being organised, so I can see what other people are working on at the moment?

We discuss most of the big LLVM wasm backend patches on the review tracker https://reviews.llvm.org/, such as https://reviews.llvm.org/D39873 which recently disabled COMDAT support (which you may want to re-enable if you're working on that :-)). And now that wasm lld support has moved upstream, that should include major lld patches as well.

https://github.com/WebAssembly/tool-conventions/blob/master/Linking.md is the work-in-progress document that describes the extra metadata to describe relocations and additional linking features. So that's a place where we can file issues and PRs and discuss them.

llvmbot commented 6 years ago

Thanks for considering it. Since there's a workaround, it could be left until Atomics.

However, this does render C++ pretty-much unusable, since a function-local static is a construct that is widely-used even in single-threaded code.

I don't know when Atomics are arriving, but if it's more than a couple of months you might want to make Clang work out of the box with the C++ in the meantime.

As as aside, I'm currently hacking away at patches for some of the other C++ sticking points, like inlines (aka COMDAT support). All of these things that need to be implemented don't seem to be tracked here in bugzilla or in any of the "official" Wasm github projects I can find. Is there some place where this work is actually being organised, so I can see what other people are working on at the moment?

sunfishcode commented 6 years ago

So far, having users that encounter this add "-mthread-model single" to their compile commands has been sufficient. When WebAssembly gets threads, we'll just implement all the atomics, and this issue will be fixed.

"-mthread-model single" isn't currently enabled by default because it hasn't come up very often, and when it has, it's been convenient to discover when code being compiled thinks it's being compiled multi-threaded. And it'll mean we won't silently break anything when we do implement atomics.

It does seem like -fno-threadsafe-statics could be enabled by "-mthread-model single", though that seems like an independent optimization rather than a real fix.

llvmbot commented 6 years ago

I've been having a go at fixing this.

Some observations:

  1. The atomic_load comes from ItaniumCXXABI::EmitGuardedInit
  2. However, if you emit an atomic load using the "__atomic_load" intrinsic directly in the source code, then everything is fine.
  3. WebAssemblyPassConfig::addIRPasses has code for handling the situation, and can switch between POSIX/single-threaded mode using "-mthread-model single", and with that in place the static initialiser actually compiles.

I'm uncertain how to go ahead fixing it.

a) Should Webassembly default to using ThreadModel=Single? That would be consistent, and currently it's clear that Wasm can't really support anything else. b) ItaniumCXXABI has an option "ThreadsafeStatics" ("-fno-threadsafe-statics") that controls whether the offending atomic operations are emitted in the first place. Should that be set to false, when the target processor is using ThreadModel=Single, or should ItaniumCXXABI check the ThreadModel as well as the ThreadsafeStatics setting? c) Finally, it could be fixed in the WebAssembly lowering code - clearly there's another pass of lowering going on, which is able to lower direct use of an "__atomic_load" intrinsic, but somehow it just isn't kicking in to lower the AtomicLoad emitted by the threadsafe-static-initialiser.

llvmbot commented 6 years ago

Minimal Example

== compile-error-atomic.cxx == extern void extFn();

namespace { struct DummyStruct { DummyStruct() { extFn(); } }; }

void expFn() { static DummyStruct dummy; }

== To build ==

clang++ -o /dev/null compile-error-atomic.cxx

== Output ==

fatal error: error in backend: Cannot select: t10: i32,ch = AtomicLoad<Volatile LD1bitcast (i32 @​_ZGVZ5expFnvE5dummy to i8)> t0, t13 t13: i32 = WebAssemblyISD::Wrapper TargetGlobalAddress:i32<i32 @​_ZGVZ5expFnvE5dummy> 0 t12: i32 = TargetGlobalAddress<i32 @​_ZGVZ5expFnvE5dummy> 0 In function: _Z5expFnv clang-6.0: error: clang frontend command failed with exit code 70 (use -v to see invocation) clang version 6.0.0 (trunk 318652) Target: wasm32-unknown-unknown-wasm Thread model: posix InstalledDir: /home/ncw/workspace/llvm/compile-root/bin clang-6.0: note: diagnostic msg: PLEASE submit a bug report to http://llvm.org/bugs/ and include the crash backtrace, preprocessed source, and associated run script.