Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add shared library support #338

Merged
merged 4 commits into from
Dec 12, 2023
Merged

add shared library support #338

merged 4 commits into from
Dec 12, 2023

Conversation

dicej
Copy link
Contributor

@dicej dicej commented Jul 28, 2023

This adds support for building WASI shared libraries per https://github.com/WebAssembly/tool-conventions/blob/main/DynamicLinking.md.

For the time being, the goal is to allow "pseudo-dynamic" linking using the Component Model per
https://github.com/WebAssembly/component-model/blob/main/design/mvp/examples/SharedEverythingDynamicLinking.md. This requires all libraries to be available when the component is created, but still allows runtime symbol resolution via dlopen/dlsym backed by a static lookup table. This is sufficient to support Python native extensions, for example. A complete demo using wit-component is available at https://github.com/dicej/component-linking-demo.

This requires https://reviews.llvm.org/D153293, which we will need to backport to LLVM 16 until 17 is released, hence the llvm-D153293-backport.patch file.

.gitmodules Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@yamt
Copy link
Contributor

yamt commented Sep 29, 2023

i prefer not to release a binary with __wasm_call_ctors.
cf. dicej/component-linking-demo#3

@dicej
Copy link
Contributor Author

dicej commented Sep 29, 2023

I've rebased onto main and removed the patch. @yamt does that address your concern regarding __wasm_call_ctors?

@dicej dicej force-pushed the dynamic-linking branch 2 times, most recently from c8876db to e6e8ec4 Compare September 29, 2023 20:08
src/llvm-project Outdated Show resolved Hide resolved
@yamt
Copy link
Contributor

yamt commented Sep 30, 2023

I've rebased onto main and removed the patch. @yamt does that address your concern regarding __wasm_call_ctors?

no.
the simplest fix would be to include https://reviews.llvm.org/D156205, which is unfortunately not included in LLVM 17.

wasi-sdk.cmake Outdated
@@ -38,3 +38,5 @@ set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER)
set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY)
set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY)
set(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE ONLY)

set(CMAKE_POSITION_INDEPENDENT_CODE On)
Copy link
Contributor

@yamt yamt Sep 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure how these cmake files are supposed to be used wrt shared library. can you explain a bit?
is it intended to always build pic even if shared lib is not used? why?

Copy link
Contributor Author

@dicej dicej Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I added CMAKE_POSITION_INDEPENDENT_CODE to LIBCXX_CMAKE_FLAGS in the Makefile, but it wasn't working (i.e. libc++.so and libc++abi.so still contained position-dependent code), so I added the lines to wasi-sdk.cmake and wasi-sdk-pthread.cmake. However, I just tried moving it back to Makefile, it that works fine now, so I'll push an update.

@dicej
Copy link
Contributor Author

dicej commented Oct 3, 2023

I've rebased onto main and removed the patch. @yamt does that address your concern regarding __wasm_call_ctors?

no. the simplest fix would be to include https://reviews.llvm.org/D156205, which is unfortunately not included in LLVM 17.

How does this plan sound to you?

  1. Create a new wasi-libc PR to remove the -nostdlib parameter from libc.so link command (i.e. let wasm-ld link in crt-reactor.o).
  2. Add a llvm-D156205-backport.patch to this PR and apply it as part of the build/llvm.BUILT target.

@sbc100
Copy link
Member

sbc100 commented Oct 3, 2023

I've rebased onto main and removed the patch. @yamt does that address your concern regarding __wasm_call_ctors?

no. the simplest fix would be to include https://reviews.llvm.org/D156205, which is unfortunately not included in LLVM 17.

Its possible you could get that patch backported to the 17 branch.. it seems very low risk.

How does this plan sound to you?

  1. Create a new wasi-libc PR to remove the -nostdlib parameter from libc.so link command (i.e. let wasm-ld link in crt-reactor.o).
  2. Add a llvm-D156205-backport.patch to this PR and apply it as part of the build/llvm.BUILT target.

@dicej
Copy link
Contributor Author

dicej commented Oct 3, 2023

More generally, I wonder if we should follow the Rust project's example and create a WebAssembly/llvm-project fork as a home for backports (cf. https://github.com/rust-lang/llvm-project). I do agree that backporting upstream is ideal whenever possible, but it would be nice to have a fallback option that doesn't involve applying patches at build time.

@sbc100
Copy link
Member

sbc100 commented Oct 3, 2023

More generally, I wonder if we should follow the Rust project's example and create a WebAssembly/llvm-project fork as a home for backports (cf. https://github.com/rust-lang/llvm-project). I do agree that backporting upstream is ideal whenever possible, but it would be nice to have a fallback option that doesn't involve applying patches at build time.

Personally, I think the fact that we build wasi-sdk directly from upstream llvm is a really nice feature. It means the sdk is truly just a vanilla clang. Nothing more and nothing less.

wasi-libc also supports a range of stable llvm releases which is really nice. Folks can take wasi-libc and use it with their own pre-built llvm binaries.

If wasi-sdk started to diverge from vanilla llvm I think it could cause confusion and mean that such users would not have the same experience as wasi-sdk users.

@yamt
Copy link
Contributor

yamt commented Oct 6, 2023

Its possible you could get that patch backported to the 17 branch.. it seems very low risk.

i tried to make a backport request, following https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches.
but i couldn't create an issue associated to the milestone.
does it require some special permission?

@dicej
Copy link
Contributor Author

dicej commented Oct 6, 2023

FYI, I've opened WebAssembly/wasi-libc#440 to remove the -nostdlib flag from the libc.so link command. Once that's merged, and the LLVM patch is backported, I can update both submodules in this PR.

@tschneidereit
Copy link
Member

@sbc100 do you have any advice on the issue @yamt ran into in trying to backport the patch to the 17 branch?

@sbc100
Copy link
Member

sbc100 commented Nov 9, 2023

@sbc100 do you have any advice on the issue @yamt ran into in trying to backport the patch to the 17 branch?

I don't think I've tried to backport anything since the switch to github, but it seems unlikely that they wouldn't accept request from external folks.

@yamt can you link to the issue you created and I can see if I can mark it correctly?

@yamt
Copy link
Contributor

yamt commented Nov 10, 2023

@sbc100 do you have any advice on the issue @yamt ran into in trying to backport the patch to the 17 branch?

I don't think I've tried to backport anything since the switch to github, but it seems unlikely that they wouldn't accept request from external folks.

@yamt can you link to the issue you created and I can see if I can mark it correctly?

llvm/llvm-project#68385
llvm/llvm-project#68386
llvm/llvm-project#68387
(multiple issues just because i made a few attempts)

@sbc100
Copy link
Member

sbc100 commented Nov 10, 2023

Looks like I was able to add the Milestone to llvm/llvm-project#68387

@yamt
Copy link
Contributor

yamt commented Nov 10, 2023

Looks like I was able to add the Milestone to llvm/llvm-project#68387

thank you

@dicej
Copy link
Contributor Author

dicej commented Nov 16, 2023

@sbc100 In case you missed it, looks like this is waiting on confirmation from you: llvm/llvm-project-release-prs#773 (comment)

@dicej
Copy link
Contributor Author

dicej commented Nov 26, 2023

I've updated both the wasi-libc and llvm-project submodules (including llvm-D156205), so I think this is ready to merge.

@yamt yamt mentioned this pull request Nov 29, 2023
@dicej
Copy link
Contributor Author

dicej commented Nov 29, 2023

Looks like the CI failure was due to a spurious network issue. Would someone who has the appropriate permissions please re-run the failed job?

@sbc100
Copy link
Member

sbc100 commented Nov 29, 2023

Can we do the llvm update as a separate PR maybe?

@sbc100
Copy link
Member

sbc100 commented Nov 29, 2023

Restarted failed CI jobs

@yamt
Copy link
Contributor

yamt commented Dec 7, 2023

Can we do the llvm update as a separate PR maybe?

@dicej do you want to make a separate PR?
or, i can reopen and update #359

@dicej
Copy link
Contributor Author

dicej commented Dec 7, 2023

Can we do the llvm update as a separate PR maybe?

@dicej do you want to make a separate PR? or, i can reopen and update #359

Done: #362

@abrown
Copy link
Collaborator

abrown commented Dec 8, 2023

Ok, it seems to me that we just need to remove the LLVM submodule update from this PR (since it happened in #362) and then this should be good to go?

This adds support for building WASI shared libraries per
https://github.com/WebAssembly/tool-conventions/blob/main/DynamicLinking.md.

For the time being, the goal is to allow "pseudo-dynamic" linking using the
Component Model per
https://github.com/WebAssembly/component-model/blob/main/design/mvp/examples/SharedEverythingDynamicLinking.md.
This requires all libraries to be available when the component is created, but
still allows runtime symbol resolution via `dlopen`/`dlsym` backed by a static
lookup table.  This is sufficient to support Python native extensions, for
example.  A complete demo using `wit-component` is available at
https://github.com/dicej/component-linking-demo.

This requires https://reviews.llvm.org/D153293, which we will need to backport
to LLVM 16 we're ready to upgrade to LLVM 17, hence the
llvm-D153293-backport.patch file.

Signed-off-by: Joel Dice <[email protected]>
This is where I originally meant the directive to go, but it didn't seem to work
the first time I tried it, hence the added lines in wasi-sdk.cmake and
wasi-sdk-pthread.cmake.  However, it seems to work fine now, so I'm switching it
back to be consistent with other similar CMake flags.

Signed-off-by: Joel Dice <[email protected]>
@dicej
Copy link
Contributor Author

dicej commented Dec 8, 2023

I just force-pushed a rebase onto main.

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm now!

(If I had my druthers to wasm-libc updated would also land separately..)

Makefile Outdated
$(MAKE) -C $(ROOT_DIR)/src/wasi-libc \
CC=$(BUILD_PREFIX)/bin/clang \
AR=$(BUILD_PREFIX)/bin/llvm-ar \
NM=$(BUILD_PREFIX)/bin/llvm-nm \
SYSROOT=$(BUILD_PREFIX)/share/wasi-sysroot \
BUILTINS_LIB=$(BUILD_PREFIX)/lib/clang/$(CLANG_VERSION)/lib/wasi/libclang_rt.builtins-wasm32.a \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is BUILTINS_LIB now needed where it wasn't before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's needed for the libc.so link step to succeed, but it looks like WebAssembly/wasi-libc#442 has removed the need to specify it explicitly, so I'll remove it.

@@ -3,7 +3,7 @@
url = https://github.com/llvm/llvm-project
[submodule "src/wasi-libc"]
path = src/wasi-libc
url = https://github.com/CraneStation/wasi-libc
url = https://github.com/WebAssembly/wasi-libc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unrelated.. but a good change.

As of WebAssembly/wasi-libc#442, these are determined
automatically.

Signed-off-by: Joel Dice <[email protected]>
@abrown abrown merged commit a35b453 into WebAssembly:main Dec 12, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants