Page MenuHomePhabricator

[clang][driver][wasm] Support -stdlib=libstdc++ for WebAssembly
ClosedPublic

Authored by tbaeder on Jan 21 2022, 6:12 AM.

Details

Summary

The WebAssembly toolchain currently supports only -stdlib=libc++
and implicitly assumes the c++ stdlib to be libc++. Change this to also
support libstdc++. The default is still libc++.

Diff Detail

Event Timeline

tbaeder created this revision.Jan 21 2022, 6:12 AM
tbaeder requested review of this revision.Jan 21 2022, 6:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2022, 6:12 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ormris removed a subscriber: ormris.Jan 24 2022, 11:09 AM

I haven't reviewed this yet, but since we got one of these before and never merged it (https://reviews.llvm.org/D101464) we should probably land one of these.

sbc100 added inline comments.Jan 28 2022, 3:51 PM
clang/lib/Driver/ToolChains/WebAssembly.cpp
434

Where does 11 come from here? Do other drivers hardcode this the same way?

tbaeder added inline comments.Jan 31 2022, 12:14 AM
clang/lib/Driver/ToolChains/WebAssembly.cpp
434

11 is just the current version. lib/Driver/ToolChains/Gnu.cpp figures out the correct version here via GCCInstallationDetector. I thought hard-coding is find for wasm because the v1 for libc++ above is also hardcoded and Gnu.cpp figures that out dynamically as well (in addLibCxxIncludePaths).

sbc100 added inline comments.Jan 31 2022, 8:15 AM
clang/lib/Driver/ToolChains/WebAssembly.cpp
434

I'd rather not hardcode that current version, that seems rather fragile/wrong.

I agree the v1 is probably also wrong (feel free to add a TODO or update that too).

tbaeder updated this revision to Diff 404849.Feb 1 2022, 2:11 AM
tbaeder added inline comments.Feb 1 2022, 2:14 AM
clang/lib/Driver/ToolChains/WebAssembly.cpp
434

Updated both. They both currently fall back to the old v1/11. I'm not sure how the test would otherwise work. As far as I know, the wasm sysroot will basically never contain an actual gcc installation, so GCCInstallationDetector can't be properly used.

sbc100 added inline comments.Feb 1 2022, 5:45 AM
clang/lib/Driver/ToolChains/WebAssembly.cpp
426–439

Can't these 6 lines be removed now? (don't they happen as part of addLibStdCXXIncludePaths?)

484

Looking at Gnu.cpp and Fuscia.cpp it seems like return (or return false) here should be fine.

535

Just early return here if no headers can be found? Looking at Gnu.cpp it seems that addLibStdCxxIncludePaths can simply to nothing if no GCC install is found.

tbaeder marked an inline comment as done.Feb 1 2022, 6:01 AM
tbaeder added inline comments.
clang/lib/Driver/ToolChains/WebAssembly.cpp
426–439

Yep, just a leftover, sorry.

535

I saw that, but I'm not sure if this is correct for wasm. The tests certainly break because they check for the /v1/ (and not /11/) include paths but also use -sysroot=/foo, so the new code doesn't add any flags. Is there a good way to update the tests so they stay functional and useful?

sbc100 added inline comments.Feb 1 2022, 6:31 AM
clang/lib/Driver/ToolChains/WebAssembly.cpp
535

I would take a look at how other platforms test this... perhaps they setup some kind of fake header tree? Or perhaps they don't test these paths at all. For sure they don't depend on the actual system where the tests run, right?

Can you point me to the tests that fail if you simply return empty string like on other platforms?

tbaeder updated this revision to Diff 404935.Feb 1 2022, 7:40 AM
tbaeder marked an inline comment as done.

Stop hardcoding v1 or 11 and make the tests work like that.

tbaeder marked 4 inline comments as done.Feb 1 2022, 7:41 AM
tbaeder added inline comments.
clang/lib/Driver/ToolChains/WebAssembly.cpp
535

The test using /foo as sysroot was just clang/test/Driver/wasm-toolchain.cpp.

The driver tests are shipping a file hierarchy to be used for this, so I switched the tests to use that as well.

tbaeder updated this revision to Diff 405156.Feb 1 2022, 11:38 PM
tbaeder marked an inline comment as done.
sbc100 accepted this revision.Feb 2 2022, 8:11 AM

Great! Thanks for working on this. I'm curious what toolchain you are working that uses GNU libstdc++?

This revision is now accepted and ready to land.Feb 2 2022, 8:11 AM
This revision was landed with ongoing or failed builds.Feb 3 2022, 7:39 AM
This revision was automatically updated to reflect the committed changes.

Great! Thanks for working on this. I'm curious what toolchain you are working that uses GNU libstdc++?

I'm looking at libstdc++ enablement in a wasm/emscripten toolchain, so nothing existing/public.

Thanks for the review!

expnkx reopened this revision.EditedFeb 4 2022, 10:02 PM
expnkx added a subscriber: expnkx.

This patch is not correct.

cqwrteur@Home-Server:~/fast_io_cleanup/fast_io/examples/0001.helloworld$ clang++ -o helloworld helloworld.cc -Ofast -std=c++2b -s -flto -fuse-ld=lld --target=wasm32-wasi --sysroot=$HOME/toolchains/sysroot/wasm32-wasi -fno-exceptions -fno-rtti -fno-unwind-tables -fno-asynchronous-unwind-tables -I../../include -stdlib=libstdc++
In file included from helloworld.cc:1:
In file included from ../../include/fast_io.h:9:
In file included from ../../include/fast_io_hosted.h:17:
In file included from ../../include/fast_io_freestanding.h:12:
In file included from ../../include/fast_io_core.h:11:
In file included from ../../include/fast_io_concept.h:20:
In file included from /home/cqwrteur/toolchains/sysroot/wasm32-wasi/include/c++/12.0.1/concepts:44:
/home/cqwrteur/toolchains/sysroot/wasm32-wasi/include/c++/12.0.1/type_traits:38:10: fatal error: 'bits/c++config.h' file not found
#include <bits/c++config.h>

^~~~~~~~~~~~~~~~~~

1 error generated.

It cannot find bits/c++config.h

wasm32-wasi\include\c++\12.0.1\wasm32-wasi\bits

I have started a revision.
https://reviews.llvm.org/D119056

This revision is now accepted and ready to land.Feb 4 2022, 10:02 PM
tbaeder closed this revision.Feb 10 2022, 11:30 PM

I screwed up using the right Differential Revision link, but this landed.