feat!: Static linking of Grain modules #584

ospencer · 2021-04-03T00:43:29Z

Closes #28.

feat: Support for more WebAssembly runtimes, including Wasmtime and Wasmer
fix!: Correct type signature for _start
chore!: Introduce _gmain for old behavior of _start
chore!: Tail calls must be enabled explicitly via --experimental-wasm-tail-call

Still a draft as this depends on a number of PRs in https://github.com/grain-lang/binaryen.ml/pulls, but everything is finished.

Static Linking

This PR implements a static linker for Grain modules, which allows us to keep our separate compilation but produce a single wasm file that works with the existing runtime and more runtimes than ever before, including Wasmtime, Wasmer, and Wasm3. This PR may not be enormous, but it's riding on the back of more than two months of hard work! 🎉

Important info

grainc will perform linking by default. This means that grain compile hello.gr && wasmtime hello.gr.wasm will work without having to worry about flags / wondering why that doesn't work. Linking can be disabled via the --no-link flag.

Since most wasm runtimes don't support tail calls yet, they're now off by default, and can be enabled via --experimental-wasm-tail-call.

-p in the grain CLI is now deprecated, as it doesn't (and can't) work for linked modules. The tests still rely on it, but once we implement snapshot testing, we can remove it.

Other info

Given this is implemented via Binaryen, I learned a ton and can probably contribute some info back to WebAssembly/binaryen#2767. The approach used isn't too far off from being able to work generically (and I even considered implementing the linker directly in Binaryen.ml), though modules would need to adhere to certain constraints.

Other thoughts

We may want to consider renaming the Compiled step in compile.re. It's sort of an odd name now, but I can't quite think of anything better.

compiler/esy.json

phated

Some thoughts, mostly about the binaryen.ml API design and about how to handle errors in the linker.

phated · 2021-04-05T00:18:00Z

compiler/dune-project

+    (reason (>= 3.6.0))
+    (ppx_sexp_conv (>= 0.14.0))
+    (sexplib (>= 0.14.0))
+    (binaryen (= 0.9.0))


Suggested change

(binaryen (= 0.9.0))

(binaryen (= 0.9.1))

compiler/esy.json

phated · 2021-04-05T00:24:04Z

compiler/src/compile.re

+          compile_file(
+            ~outfile,
+            ~reset=false,
+            ~hook=stop_after_object_file_emitted,


Why does this stop_after_object_file_emitted now?

Previously, it would just compile completely, but now since linking is a step that also happens, this skips linking when compiling dependencies (since they'll all be linked together at the end).

phated · 2021-04-05T00:36:51Z

compiler/src/linking/link.re

+let rec globalize_names = (local_names, expr) => {
+  let kind = Expression.get_kind(expr);
+  switch (kind) {
+  | Invalid => failwith("Invalid expression")


Do we want this to failwith or raise? I feel like most of the compiler uses raise

We typically use failwith with things that are "impossible" and raise for errors related to the user's program. The only way we'd ever hit this case is if we generated a wasm module with instructions that the linker didn't know about.

We can probably do a round up on errors, but that's probably for another PR :)

compiler/src/linking/link.re

phated · 2021-04-05T01:23:39Z

compiler/src/linking/link.re

+          let new_name =
+            Hashtbl.find(
+              Hashtbl.find(exported_names, imported_module),
+              imported_name,
+            );
+          Hashtbl.add(local_names, internal_name, new_name);


This code is repeated a couple times.

It's similar, but not quite the same. They essentially get to the same end result, but it takes different functions to get there.

phated · 2021-04-05T01:26:38Z

compiler/src/linking/link.re

+        let new_name = gensym(internal_name);
+        Hashtbl.add(local_names, internal_name, new_name);


Whenever you gensym a new name, you need to add it to the local_names hashtbl, right? Maybe those should be bundled so it isn't missed during a code cleanup or something.

Not always, no. I suppose we could have some utility that does this for where we do need to add it to the table.

phated · 2021-04-05T01:29:26Z

compiler/src/linking/link.re

+      ignore @@
+      Table.add_active_element_segment(
+        linked_mod,
+        "tbl",


I've seen this string a couple times in this file. Should it be a constant at the top?

phated · 2021-04-05T01:31:25Z

compiler/src/linking/link.re

+      [
+        Features.mvp,
+        Features.multivalue,
+        Features.tail_call,


We can always enable tail_call, even it not supported in a runtime?

phated · 2021-04-05T01:31:57Z

compiler/src/linking/link.re

+      ],
+    );
+  let _ = Module.set_low_memory_unused(1);
+  assert(Module.validate(linked_mod) == 1);


assert, failwith or raise?

phated · 2021-04-08T17:04:23Z

esy.lock/.gitattributes

@@ -0,0 +1,3 @@
+
+# Set eol to LF so files aren't converted to CRLF-eol on Windows.
+* text eol=lf linguist-generated


@ospencer Oh, I just noticed that you accidentally ran esy in the top of the monorepo and committed an extra esy.lock directory here.

phated

@ospencer you are an absolute genius. Amazing work on this!

It seems we both modified the esy.lock so you'll need to resolve that, but otherwise good to go 🎉

feat: Support for more WebAssembly runtimes, including Wasmtime and Wasmer fix!: Correct type signature for `_start` chore!: Introduce `_gmain` for old behavior of `_start` chore!: Tail calls must be enabled explicitly via `--experimental-wasm-tail-call` fix: Use proper return type for calls to external functions feat: Normalized wasm exports for linked modules

ospencer added enhancement compiler labels Apr 3, 2021

ospencer requested a review from a team April 3, 2021 00:43

ospencer self-assigned this Apr 3, 2021

phated reviewed Apr 3, 2021

View reviewed changes

compiler/esy.json Outdated Show resolved Hide resolved

ospencer force-pushed the static-linking branch from eb85814 to a9a3543 Compare April 3, 2021 04:58

phated reviewed Apr 5, 2021

View reviewed changes

phated reviewed Apr 8, 2021

View reviewed changes

ospencer force-pushed the static-linking branch 4 times, most recently from cc2e1c0 to 255c95f Compare April 18, 2021 03:45

ospencer marked this pull request as ready for review April 18, 2021 03:46

ospencer requested a review from a team April 18, 2021 03:47

ospencer force-pushed the static-linking branch from 255c95f to eadb6ea Compare April 18, 2021 18:53

phated approved these changes Apr 18, 2021

View reviewed changes

ospencer force-pushed the static-linking branch from eadb6ea to 63b29ea Compare April 18, 2021 19:49

ospencer merged commit 3d4ac6e into main Apr 18, 2021

ospencer deleted the static-linking branch April 18, 2021 20:13

phated mentioned this pull request Apr 19, 2021

chore: release testing-release #595

Closed

github-actions bot mentioned this pull request Apr 20, 2021

chore: release main #607

Merged

github-actions bot mentioned this pull request May 31, 2022

chore: release main #1292

Merged

github-actions bot mentioned this pull request Jul 25, 2022

chore: release main spotandjake/grain#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Static linking of Grain modules #584

feat!: Static linking of Grain modules #584

ospencer commented Apr 3, 2021 •

edited

Loading

phated left a comment

phated Apr 5, 2021

phated Apr 5, 2021

ospencer Apr 17, 2021

phated Apr 5, 2021

ospencer Apr 17, 2021

phated Apr 5, 2021

ospencer Apr 17, 2021

phated Apr 5, 2021

ospencer Apr 17, 2021

phated Apr 5, 2021

phated Apr 5, 2021

phated Apr 5, 2021

phated Apr 8, 2021

phated left a comment

		let new_name = gensym(internal_name);
		Hashtbl.add(local_names, internal_name, new_name);

		@@ -0,0 +1,3 @@

		# Set eol to LF so files aren't converted to CRLF-eol on Windows.
		* text eol=lf linguist-generated

feat!: Static linking of Grain modules #584

feat!: Static linking of Grain modules #584

Conversation

ospencer commented Apr 3, 2021 • edited Loading

Static Linking

Important info

Other info

Other thoughts

phated left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phated left a comment

Choose a reason for hiding this comment

ospencer commented Apr 3, 2021 •

edited

Loading