This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Support multilibs for wasm32 and add a wasm OS that uses it
ClosedPublic

Authored by sunfish on Jan 10 2019, 10:08 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sunfish created this revision.Jan 10 2019, 10:08 AM

From a survey and some testing, Emscripten doesn't appear to rely on these paths, so this just affects things like wasmception and the waterfall, which should be straightforward to update.

LGTM but is there really no test for this? I'd expect to see this on linker command lines or something, right? If not we should add one.

My understanding is that names like "lib32" and "lib64", at least for x86/amd64 are the old fashioned (legact) way of doing multi-arch. I things a better approach it to either provide and single-arch sysroot (where /usr/lib contains what you want) or a multi-arch setup (where you would use something like /usr/lib/x86_64-linux-gnu/). Since other parts of a sysroot need to be arch specific I suggest we stick with the former option and assume a separate sysroot for each architecture and sub-architecture.

So, while I don't support this change, I would support something like adding "/lib/wasm32-unknown-unknown" before "/lib" in the default library path.

sunfish retitled this revision from [WebAssembly] Change the default lib path for wasm32 to be lib32 to [WebAssembly] Support multilibs for wasm32.Jan 14 2019, 10:20 AM
sunfish edited the summary of this revision. (Show Details)
sunfish updated this revision to Diff 181584.Jan 14 2019, 10:22 AM

Per @sbc100's comment, this update switches to full multilib paths.

Ooo - a new OS is born!

Perhaps create a separate change to introduce the new OS? .. which will then make this CL tiny and specific to what the CL title says it is doing.

tools/clang/lib/Driver/ToolChains/WebAssembly.cpp
95 ↗(On Diff #181584)

For include paths you have multiarch + default but for library paths you have multiarch *or* default. Is that deliberate?

One difference between the multilib style and multi-arch style is whether any headers can be shared between the 2 arches. Historically at least, with bitness-flavored variants of the same architecture there were common headers used by both, and it was just the library directories that needed to be different. I've heard anecdotally that this is not the case for ARM though.

Would we expect that users might want to use a single sysroot to generate code for both 32-and 64-bit output?

sunfish marked an inline comment as done.Jan 14 2019, 11:59 AM

Ooo - a new OS is born!

Perhaps create a separate change to introduce the new OS? .. which will then make this CL tiny and specific to what the CL title says it is doing.

Without the new OS I wouldn't be able to fully test the new functionality, as we have no other wasm OS's in tree. Is the size of this patch an issue?

tools/clang/lib/Driver/ToolChains/WebAssembly.cpp
95 ↗(On Diff #181584)

Yes; I followed https://wiki.debian.org/Multiarch/Implementation . "include" can contain platform-independent headers, while libraries are inherently platform-dependent.

One difference between the multilib style and multi-arch style is whether any headers can be shared between the 2 arches. Historically at least, with bitness-flavored variants of the same architecture there were common headers used by both, and it was just the library directories that needed to be different. I've heard anecdotally that this is not the case for ARM though.

Would we expect that users might want to use a single sysroot to generate code for both 32-and 64-bit output?

Perhaps when we add support for wasm64, we could also add an additional include path "include/wasm-$OS", separate from "include/wasm32-$OS" and "include/wasm64-$OS", which could be shared by both wasm32 and wasm64, but still specific to wasm.

Or, here's another idea: What if we *just* add "include/wasm-$OS" and don't add "wasm32-$OS" or "wasm64-$OS"-specific include directories? Most headers for wasm will probably be shared between the two, and for anything that really can't be, we always have the __wasm32__ and __wasm64__ predefined macros.

Ooo - a new OS is born!

Perhaps create a separate change to introduce the new OS? .. which will then make this CL tiny and specific to what the CL title says it is doing.

Without the new OS I wouldn't be able to fully test the new functionality, as we have no other wasm OS's in tree. Is the size of this patch an issue?

Not so much the size of the patch, but more that atomicity. Introducing the new OS seems like an naturally separate change to me.

If you don't want to split it at least update the CL title and description.

Looking that the behaviour for x86 clang it looks like -L/lib -L/usr/lib are always appended for both -m32 and -m64 with the arch specific directories coming first.

sunfish retitled this revision from [WebAssembly] Support multilibs for wasm32 to [WebAssembly] Support multilibs for wasm32 and add a wasm OS that uses it.Jan 14 2019, 12:21 PM
sunfish edited the summary of this revision. (Show Details)

Can you make change to llvm and clang at the same time? I thought that was not possible..

Title updated.

Looking that the behaviour for x86 clang it looks like -L/lib -L/usr/lib are always appended for both -m32 and -m64 with the arch specific directories coming first.

My reading of https://wiki.debian.org/Multiarch/Implementation is that /lib and /usr/lib don't exist a full multi-arch world. I assume they're still around on x86 because the transition to multiarch is still ongoing and some packages still expect to install their libs there. I'd prefer to avoid adding it since it could cause trouble if anyone actually created a multilib sysroot with non-wasm libs in /lib.

Can you make change to llvm and clang at the same time? I thought that was not possible..

That's right, I'll commit the two parts separately.

dschuff accepted this revision.Jan 14 2019, 1:12 PM
dschuff added inline comments.
include/llvm/ADT/Triple.h
191 ↗(On Diff #181584)

The fact that COWS comes right after Hurd makes me way happier than it should.

This revision is now accepted and ready to land.Jan 14 2019, 1:12 PM
This revision was automatically updated to reflect the committed changes.