wasmtime / PR #1391 Use `Linker` in `*.wast` testing · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1391 Use `Linker` in `*.wast` testing

Wasmtime GitHub notifications bot (Mar 24 2020 at 16:40):

alexcrichton opened PR #1391 from wast-linker to master:

By default Linker disallows shadowing previously defined items, but it
looks like the *.wast test suites rely on this so this commit adds a
boolean flag to Linker as well indicating whether duplicates are
allowed.

Wasmtime GitHub notifications bot (Mar 24 2020 at 17:01):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 17:01):

bjorn3 created PR Review Comment:

On most systems the symbols loaded first have priority I believe. (Otherwise LD_PRELOAD would be pretty much useless.)

Wasmtime GitHub notifications bot (Mar 24 2020 at 17:02):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 17:02):

bjorn3 created PR Review Comment:

                "import of `{}::{}` with kind {:?} defined twice",

Wasmtime GitHub notifications bot (Mar 24 2020 at 18:10):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 18:10):

peterhuene created PR Review Comment:

I agree that in the context of LD_PRELOAD that makes sense, but from from an API perspective, last-in-wins might be more intuitive for developers (a la virtual inheritance). For example, I'd probably tell the linker to link in an engine-provided WASI implementation and then my host functions, which could potentially override the WASI implementation.

Wasmtime GitHub notifications bot (Mar 24 2020 at 18:18):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 18:18):

peterhuene created PR Review Comment:

Teensy naming nit: perhaps allow_shadowing or enable_shadowing? The singular shadow reads a little strange to me for some reason.

Wasmtime GitHub notifications bot (Mar 24 2020 at 18:22):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 18:23):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Mar 24 2020 at 18:28):

sunfishcode created PR Review Comment:

LD_PRELOAD-style symbol preemption is fairly limited and awkward though. It's possible for libraries to make calls to their own symbols which can't be preempted, or to make assumptions that break if their symbols get preempted. So you have to depend on the library not doing those things.

I think it would be better to encourage wasm programs and tools to focus on interposition rather than preemption. Load the replacement after the thing it is replacing rather than before. One nice thing about this approach is that it means you can easy wrap functions and call them from your wrappers. For things like malloc replacements, it may require libraries to be structured differently, but it has the advantage of not depending on any implicit contracts with the libc to only access the malloc heap through the public interface.

Wasmtime GitHub notifications bot (Mar 24 2020 at 18:28):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 18:42):

alexcrichton updated PR #1391 from wast-linker to master:

By default Linker disallows shadowing previously defined items, but it
looks like the *.wast test suites rely on this so this commit adds a
boolean flag to Linker as well indicating whether duplicates are
allowed.

Wasmtime GitHub notifications bot (Mar 24 2020 at 18:43):

alexcrichton created PR Review Comment:

FWIW I think all behaviors are fine to implement. They're all simple enough and it's largely just a matter of defaults. If necessary it would be quite easy to implement a LD_PRELOAD-style first-one-always-wins, but I think it's best to wait for a use case to come up first.

Wasmtime GitHub notifications bot (Mar 24 2020 at 18:43):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 19:23):

alexcrichton updated PR #1391 from wast-linker to master:

By default Linker disallows shadowing previously defined items, but it
looks like the *.wast test suites rely on this so this commit adds a
boolean flag to Linker as well indicating whether duplicates are
allowed.

Wasmtime GitHub notifications bot (Mar 24 2020 at 19:27):

alexcrichton updated PR #1391 from wast-linker to master:

By default Linker disallows shadowing previously defined items, but it
looks like the *.wast test suites rely on this so this commit adds a
boolean flag to Linker as well indicating whether duplicates are
allowed.

Wasmtime GitHub notifications bot (Mar 24 2020 at 20:24):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 20:24):

sunfishcode created PR Review Comment:

Can you reformat this to avoid tab characters?

Wasmtime GitHub notifications bot (Mar 24 2020 at 20:24):

sunfishcode created PR Review Comment:

It feels awkward to have both instances and linker, since they're largely overlapping. Code like fn invoke currently looks up a method to invoke by doing get_instance and then looking up the export. Can it user the linker to do this instead? It seems like we should be able to completely eliminate instances.

Wasmtime GitHub notifications bot (Mar 24 2020 at 20:24):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:10):

alexcrichton updated PR #1391 from wast-linker to master:

By default Linker disallows shadowing previously defined items, but it
looks like the *.wast test suites rely on this so this commit adds a
boolean flag to Linker as well indicating whether duplicates are
allowed.

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:11):

alexcrichton created PR Review Comment:

I agree yeah it's a bit awkward but I don't think we can easily change this. This would involve adding some sort of accesor to Linker and it wouldn't work well for the register command which takes a whole instance and puts it in the linker. Would you be ok trying to clean this up at a later date?

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:11):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:33):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:33):

sunfishcode created PR Review Comment:

Yeah. In fact, I think I have an idea of how to do it, so let's go ahead and land this and I'll see if my idea works in a separate PR.

Wasmtime GitHub notifications bot (Mar 24 2020 at 21:33):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 22:37):

alexcrichton merged PR #1391.

Wasmtime GitHub notifications bot (Mar 24 2020 at 22:44):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 22:44):

sunfishcode created PR Review Comment:

I took a quick look at this, and it turns out to be harder than I thought. The Linker interface doesn't have a way to look up a function without knowing its return types, which is something that the wast invoke command does. We may eventually want to add a way to do this, but we can figure that out later.

Last updated: Dec 23 2024 at 12:05 UTC