Page MenuHomePhabricator

[clang] Add -stdlib=libstdc++ support for webassembly
Needs ReviewPublic

Authored by expnkx on Apr 28 2021, 8:06 AM.

Details

Reviewers
sbc100
jwakely
Summary

I have successfully built libstdc++ with clang on wasm32-wasi, but the clang front-end always assumes we are using -stdlib=libc++.

This patch is to add support for -stdlib=libstdc++ for webassembly.
https://gcc.gnu.org/pipermail/libstdc++/2021-April/052429.html

I do not know whether add -stdlib=libstdc++ in the test will break the code. Let's just try it.

Diff Detail

Event Timeline

expnkx created this revision.Apr 28 2021, 8:06 AM
expnkx requested review of this revision.Apr 28 2021, 8:06 AM
expnkx updated this revision to Diff 341223.EditedApr 28 2021, 8:37 AM

It is impossible to do any real test because libstdc++ is a part of GCC and GCC does not provide wasm32 and wasm64 backend.

See
https://gcc.gnu.org/pipermail/libstdc++/2021-April/052429.html

expnkx retitled this revision from Add -stdlib=libstdc++ support for webassembly for clang to [clang] Add -stdlib=libstdc++ support for webassembly for clang.Apr 28 2021, 8:56 AM
expnkx retitled this revision from [clang] Add -stdlib=libstdc++ support for webassembly for clang to [clang] Add -stdlib=libstdc++ support for webassembly.

I think should should be able to add some tests to clang/test/Driver/wasm-toolchain.c

clang/lib/Driver/ToolChains/WebAssembly.cpp
443

Although it makes no odds with wasm-ld should. logically,stdc++ come before supc++? Like c++ comes before c++abi above? (This seems to be the case in clang/lib/Driver/ToolChains/BareMetal.cpp

sbc100 added inline comments.Apr 28 2021, 9:00 AM
clang/lib/Driver/ToolChains/WebAssembly.cpp
388

This seems to add an early exit for OPT_nostdinc where none existed before. Different targets seems to disagree about whether to do this. To keep this change a focused as possible maybe revert that part of the change?

wait me for fixing them

I think should should be able to add some tests to clang/test/Driver/wasm-toolchain.c

How to test it? I guess no one in the world besides me knows how to build libstdc++ for wasm.

I think should should be able to add some tests to clang/test/Driver/wasm-toolchain.c

How to test it? I guess no one in the world besides me knows how to build libstdc++ for wasm.

That tests in clang/test/Driver/wasm-toolchain.c don't actually require any kind of libc (or indeed any libraries) to exist. They just test the behaviour of the driver itself in terms of what sub-commands in runs.

jwakely added inline comments.
clang/lib/Driver/ToolChains/WebAssembly.cpp
443

Every symbol in libsupc++.a is also in libstdc++.so and libstdc++.a so -lstdc++ -lsupc++ should be exactly equivalent to -lstdc++.

I suppose ordering them this way means the linker might be able to use libsupc++.a to resolve everything, and so not need to link to libstdc++.so at all (e.g. if there is a wasm equivalent of --as-needed). But I don't know if that's even relevant for wasm.

expnkx updated this revision to Diff 341239.Apr 28 2021, 9:17 AM
expnkx edited the summary of this revision. (Show Details)
expnkx updated this revision to Diff 341241.Apr 28 2021, 9:30 AM

i have removed -lsupc++, just -lstdc++

expnkx updated this revision to Diff 341253.EditedApr 28 2021, 9:57 AM
expnkx marked 3 inline comments as done.

I just realized the version is incorrect. GCC master is currently at 12.0.0. GCC 13.0.0 will be a thing one year later. It is pointless to add the test here.

Remove it.

https://github.com/expnkx/GCC-12-libstdc-for-wasm32-wasi

Here is my build of sysroot for wasm32-wasi. You can try this if you want.

expnkx accepted this revision.Apr 28 2021, 10:32 AM
expnkx marked an inline comment as not done.
expnkx added inline comments.
clang/lib/Driver/ToolChains/WebAssembly.cpp
388

fixed

443

fixed

This revision is now accepted and ready to land.Apr 28 2021, 10:32 AM
expnkx resigned from this revision.Apr 28 2021, 10:34 AM
This revision now requires review to proceed.Apr 28 2021, 10:34 AM
expnkx updated this revision to Diff 341261.Apr 28 2021, 10:37 AM

I just realized clang-tidy said there is a change which requires. Fix it.

expnkx accepted this revision.Apr 28 2021, 10:41 AM

Approved!

This revision is now accepted and ready to land.Apr 28 2021, 10:41 AM
expnkx updated this revision to Diff 341283.Apr 28 2021, 11:50 AM

clang-tidy complains again. Fix it

expnkx updated this revision to Diff 341284.Apr 28 2021, 11:56 AM

it is weird, why is this test -stdlib=c++ instead of -stdlib=libc++?? Is that legal?

expnkx accepted this revision.Apr 28 2021, 11:57 AM

what's happening with the build?

expnkx updated this revision to Diff 341341.Apr 28 2021, 3:57 PM

let me try this patch. It removes unknown OS checks.

expnkx updated this revision to Diff 341385.Apr 28 2021, 7:23 PM

I finally realize what is happening here. The virtual filesystem emits // instead of \ for seperator.

Also adds a basic link and compile test for -stdlib=libstdc++

expnkx updated this revision to Diff 341390.Apr 28 2021, 8:35 PM

Looks like it is a naming collision?

expnkx updated this revision to Diff 341397.Apr 28 2021, 9:27 PM

Force the front end emitting the Unix style path to get rid of troubles in the testcase.

expnkx updated this revision to Diff 341403.Apr 28 2021, 10:10 PM

remove libstdc++ test first. Let me see what's actually going wrong here.

expnkx updated this revision to Diff 341410.Apr 28 2021, 10:43 PM

I guess the multiarch include order matters for tests.

DONE :)

Time to merge it :)

expnkx accepted this revision.Apr 30 2021, 9:40 AM
expnkx removed a reviewer: Restricted Project.
expnkx resigned from this revision.Apr 30 2021, 9:46 AM
This revision now requires review to proceed.Apr 30 2021, 9:46 AM
expnkx accepted this revision.Apr 30 2021, 9:46 AM
This revision is now accepted and ready to land.Apr 30 2021, 9:46 AM
expnkx added a reviewer: Restricted Project.Apr 30 2021, 9:55 AM
echristo edited reviewers, added: sbc100, jwakely; removed: expnkx, Restricted Project.Apr 30 2021, 11:03 AM
echristo added a subscriber: echristo.

I think Sam or Jonathan would be good reviewers here.

-eric

This revision now requires review to proceed.Apr 30 2021, 11:03 AM
sbc100 added a comment.EditedMay 1 2021, 7:37 AM
This comment has been deleted.
clang/lib/Driver/ToolChains/WebAssembly.cpp
390

I've not seen this style in widespread use in llvm yet .. I guess its somehow preferred over std::string SysRoot(X) or std::string SysRoot = X; ? Unless this is documented as preferred somewhere I would stick more conventional form (or maybe I'm just out-of-date here).

417

I'm curious if this code is based on something existing in another driver or if its something you created from scratch? Could this kind of logic could be shared? I see other drivers using something called GCCInstallation but maybe that doesn't make sense for some reason here (i..e maybe its not suitable for cross compilers like us?)

420

Perhaps break; here is more future proof (in case code gets added after the switch)

clang/test/Driver/wasm-toolchain.cpp
17

This is an interesting/breaking change.. it seems that we splelt this wrong in the WebAssembly driver from the beginning. The docs clearly say this flag should be --stdlib=libc++ : https://libcxx.llvm.org/docs/UsingLibcxx.html#getting-started

I'm fairly certain we don't have anyone using this in the wild (since there is only one possible option here), but I think it might be worth splitting this out into its owr PR that just fixes this naming mistake. It seems logical to separate it from adding support for libstdc++ and will make the possible revert of one or change or the other easier if it ever comes to that.

69

I suppose we don't see the include paths here because the driver doesn't see them on the filesystem? It looks like the linux driver uses a dummy sysroot tree to enable testing of these:

lang/test/Driver/linux-header-search.cpp:

// RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1 \                  
// RUN:     -target x86_64-unknown-linux-gnu \                                      
// RUN:     -stdlib=libc++ \                                                        
// RUN:     -ccc-install-dir %S/Inputs/basic_linux_tree/usr/bin \                   
// RUN:     -resource-dir=%S/Inputs/resource_dir \                                  
// RUN:     --sysroot=%S/Inputs/basic_linux_libcxxv2_tree \                         
// RUN:     --gcc-toolchain="" \                                                    
// RUN:   | FileCheck --check-prefix=CHECK-BASIC-LIBCXXV2-SYSROOT %s                
// CHECK-BASIC-LIBCXXV2-SYSROOT: "{{[^"]*}}clang{{[^"]*}}" "-cc1"                   
// CHECK-BASIC-LIBCXXV2-SYSROOT: "-isysroot" "[[SYSROOT:[^"]+]]"                    
// CHECK-BASIC-LIBCXXV2-SYSROOT: "-internal-isystem" "[[SYSROOT]]/usr/include/x86_64-unknown-linux-gnu/c++/v2"
// CHECK-BASIC-LIBCXXV2-SYSROOT: "-internal-isystem" "[[SYSROOT]]/usr/include/c++/v2"                                     
// CHECK-BASIC-LIBCXXV2-SYSROOT: "-internal-isystem" "[[SYSROOT]]/usr/local/include"
expnkx updated this revision to Diff 342158.May 1 2021, 9:18 AM

Let me fix the 2 formatting issues first.

ormris removed a subscriber: ormris.Jun 3 2021, 10:29 AM