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

[Macros] Add support for wasm macros #73031

Open
wants to merge 96 commits into
base: main
Choose a base branch
from

Conversation

kabiroberai
Copy link
Contributor

@kabiroberai kabiroberai commented Apr 15, 2024

This PR introduces support for WebAssembly-based macros via -load-plugin-executable Foo.wasm#Foo.

Wasm macros are blazing fast (no SwiftSyntax build required for consumers!) and secure (fully sandboxed on all OSes)

time $BIN/swiftc Client.swift \
  -load-plugin-executable ExampleRaw.wasm#ExampleRaw
Ninja-RelWithDebInfoAssert/swift-macosx-arm64/bin/swiftc -c 0.06s user 0.03s system 33% cpu 0.244 total

The approach I've taken here is to update swift-plugin-server to add special handling for Wasm macros. These macros are executed using WasmKit, but in the future we can add support for other (faster, JIT) runtimes like JavaScriptCore.

Note also that these wasm macros currently have an ad-hoc ABI as I wanted to avoid a hard dependency on WASI. It's thus theoretically possible to build macros without a dependency on WASI, though I've had trouble doing so because Swift currently builds for wasm as WASI xor Embedded Mode — we'd need a middle ground with support for String etc. The current ABI is based on the one I built for Wacro. We'll have to iron out the execution model though: my current mental model is that plugins are request-response style (i.e. every HostToPluginMessage must be followed by exactly one PluginToHostMessage) and so the macro effectively exports a char *macro_parse(char *request_json) but I don't know if this is a guaranteed contract.

There's currently a few things that still need to be done:

  • Support other platforms with WasmKit. (*)
  • Unit tests

Followups:

  • Figure out the story for building macro targets — the approach I'm using right now is to have the "source" macro depend on Wacro but we'll probably want to upstream some sort of Wasm macro module into swift-syntax. (*)
  • SwiftPM support + evolution proposal? I'm envisioning a .binaryMacro target similar to .binaryTarget, though this will make the edit-compile-run cycle non-trivial as it splits building the macro into a separate phase. (*)

Marking this as a draft until the above items are done but I wanted to make this PR so I could get some input as well as to brainstorm (particularly about the items marked as (*))

Depends on swiftlang/swift-syntax#2623

There's also a corresponding PR in swift-driver, which is blocked by this one: swiftlang/swift-driver#1582

lib/AST/PluginLoader.cpp Outdated Show resolved Hide resolved
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http:https://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>com.apple.security.cs.allow-jit</key>
Copy link
Contributor Author

@kabiroberai kabiroberai Apr 15, 2024

Choose a reason for hiding this comment

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

Necessary in order for JSC to expose globalThis.WebAssembly — I don't think there's any security risk here because /System/Library/Frameworks/JavaScriptCore.framework/Versions/Current/Helpers/jsc already exposes significantly more surface area. Plus we're able to keep the restrictive sandbox from Sandbox::apply intact when running this via swiftc.

@kabiroberai kabiroberai marked this pull request as draft April 15, 2024 18:15
@kabiroberai
Copy link
Contributor Author

paging @DougGregor @MaxDesiatov as stakeholders, would love to get your thoughts on this

@kabiroberai
Copy link
Contributor Author

also wondering how I should add end-to-end tests for this: I would include some wasm blob fixtures but I don't want to be mistaken for the Second Coming of Jia Tan

@kabiroberai
Copy link
Contributor Author

Another question: how much should we generalize this right now? As in, should we have a specialized wasm executor embedded into swift-wasm-plugin-server or would it maybe make sense to ship a general swift-wasm-runtime that can be used for other things (such as Swift Package Plugins) as well? I'm leaning towards the former because I think we shouldn't prematurely abstract but there are pros to the other approach as well.

@DougGregor
Copy link
Member

I like @kateinoigakukun 's idea of seeing whether we can slim down the WASI build to include those things that already work across the platforms and are needed by macros.

To include Windows support of the WASI implementation.
@kateinoigakukun
Copy link
Member

@swift-ci smoke test

The support for `pipe` on Windows was added by apple/swift-system@cfd3f9d
but it is not included in the latest swift-system release 1.3.1.
This commit adds a temporary inlined implementation of `pipe` API for
Windows until we update to the next release of swift-system.
@kateinoigakukun
Copy link
Member

@swift-ci smoke test

`%c-flags` and `%exe-linker-flags` are derived from `CMAKE_C_FLAGS` and
`CMAKE_EXE_LINKER_FLAGS` respectively, which are not configured for Wasm
targets. And also those variables are not set in Windows build. So, this
patch removes the usage of those variables.
@kateinoigakukun
Copy link
Member

@swift-ci smoke test

@kateinoigakukun
Copy link
Member

T:\5\tools\swift\test-windows-x86_64\Macros\Output\macro_plugin_wasm_guest_error.swift.tmp/test.swift:1:33: error: unexpected warning produced: external macro implementation type 'MacroDefinition.ConstMacro' could not be found for macro 'constInt()'; no such file or directory

@freestanding(expression) macro constInt() -> Int = #externalMacro(module: "MacroDefinition", type: "ConstMacro")

@kabiroberai Hmm, something wrong is happening on Windows 🤔

@kabiroberai
Copy link
Contributor Author

thanks for all the Windows-related fixes @kateinoigakukun, looks like we're near the finish line! I wonder if the current failures are related to dos/unix path formats (backslash vs forward slash)?

ended up being busier than expected last week and might not have much time until EOW, but I'll try to configure a local testing setup on Windows by Saturday so that I can poke around. Wouldn't be surprised if it's a 1-2 line fix.

@kateinoigakukun
Copy link
Member

@swift-ci smoke test

The ':' character can cause conflicts in Windows paths (e.g., C:\path\to\file).
The '#' character is less likely to appear in file paths and we already
use it for the second path separator in `-load-plugin` option
@kateinoigakukun
Copy link
Member

@swift-ci smoke test

1 similar comment
@kateinoigakukun
Copy link
Member

@swift-ci smoke test

@kateinoigakukun
Copy link
Member

Okay, finally CI checks are all green!

@etcwilde
Copy link
Contributor

@swift-ci please Build Toolchain Linux Platform

@etcwilde
Copy link
Contributor

@swift-ci please Build Toolchain macOS Platform

@MaxDesiatov
Copy link
Contributor

@swift-ci build toolchain windows

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

This has ended up... almost straightforward in its integration. Very nice!

Comment on lines +4 to +9
file(TO_CMAKE_PATH "${SWIFT_PATH_TO_SWIFT_SYNTAX_SOURCE}" swift_syntax_path)
FetchContent_Declare(SwiftSystem SOURCE_DIR "${swift_syntax_path}/../swift-system")

set(WASMKIT_BUILD_CLI OFF)
FetchContent_Declare(WasmKit SOURCE_DIR "${swift_syntax_path}/../wasmkit")
FetchContent_MakeAvailable(WasmKit)
Copy link
Member

Choose a reason for hiding this comment

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

I suppose the alternative to this is to thread new variables for these paths throughout build-script and build.ps1, which is... a lot of boilerplate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, do you think it's worth doing that right now? Or would it be preferable to merge as-is?

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.

None yet

6 participants