-
Notifications
You must be signed in to change notification settings - Fork 50
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
[interpreter] Fix registry to be per-thread #219
Open
rossberg
wants to merge
2
commits into
upstream-rebuild
Choose a base branch
from
fix.registry
base: upstream-rebuild
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
# This Makefile uses dune but does not rely on ocamlfind or the Opam | ||
# package manager to build. However, Opam package management is available | ||
# optionally through the check/install/uninstall targets. | ||
# optionally through the install target. | ||
# | ||
# The $(JSLIB).js target requires Js_of_ocaml (using ocamlfind). | ||
# | ||
|
@@ -9,80 +9,92 @@ | |
|
||
# Configuration | ||
|
||
NAME = wasm | ||
OPT = $(NAME).exe | ||
ZIP = $(NAME).zip | ||
NAME = wasm | ||
LIB = $(NAME) | ||
JSLIB = wast.js | ||
ZIP = $(NAME).zip | ||
|
||
BUILDDIR = _build/default | ||
BUILDDIR = _build/default | ||
|
||
JS = # set to JS shell command to run JS tests, empty to skip | ||
|
||
|
||
# Main targets | ||
|
||
.PHONY: default opt jslib all zip smallint | ||
.PHONY: default all ci jslib zip | ||
|
||
default: $(NAME) | ||
all: default partest | ||
ci: all jslib zip | ||
|
||
default: $(OPT) | ||
jslib: $(JSLIB) | ||
all: $(OPT) test | ||
zip: $(ZIP) | ||
smallint: smallint.exe | ||
ci: all jslib | ||
|
||
# Building executable | ||
.PHONY: $(NAME).exe | ||
$(NAME).exe: | ||
rm -f $(NAME) | ||
dune build $@ | ||
cp $(BUILDDIR)/$(OPT) $(NAME) | ||
|
||
.PHONY: smallint.exe | ||
smallint.exe: | ||
dune build $@ | ||
# Building | ||
|
||
.PHONY: $(NAME) $(JSLIB) | ||
|
||
$(NAME): | ||
rm -f $@ | ||
dune build [email protected] | ||
ln $(BUILDDIR)/[email protected] $@ | ||
|
||
$(JSLIB): | ||
rm -f $@ | ||
dune build $(@:%.js=%.bc.js) | ||
ln $(BUILDDIR)/$(@:%.js=%.bc.js) $@ | ||
|
||
|
||
# Unit tests | ||
|
||
UNITTESTDIR = unittest | ||
UNITTESTFILES = $(shell cd $(UNITTESTDIR) > /dev/null; ls *.ml) | ||
UNITTESTS = $(UNITTESTFILES:%.ml=%) | ||
|
||
# Building JavaScript library | ||
.PHONY: unittest | ||
|
||
$(JSLIB): $(BUILDDIR)/$(JSLIB) | ||
cp $< $@ | ||
unittest: $(UNITTESTS:%=unittest/%) | ||
|
||
$(BUILDDIR)/$(JSLIB): | ||
dune build $(JSLIB) | ||
unittest/%: | ||
dune build $(@F).exe | ||
dune exec ./$(@F).exe | ||
|
||
# Executing test suite | ||
|
||
# Test suite | ||
|
||
TESTDIR = ../test/core | ||
# Skip _output directory, since that's a tmp directory, and list all other wast files. | ||
TESTFILES = $(shell cd $(TESTDIR); ls *.wast; ls [a-z]*/*.wast) | ||
TESTFILES = $(shell cd $(TESTDIR) > /dev/null; ls *.wast; ls [a-z]*/*.wast) | ||
TESTS = $(TESTFILES:%.wast=%) | ||
|
||
.PHONY: test partest dune-test | ||
.PHONY: test partest quiettest | ||
|
||
test: $(NAME) unittest | ||
$(TESTDIR)/run.py --wasm `pwd`/$(NAME) $(if $(JS),--js '$(JS)',) | ||
|
||
test: $(OPT) smallint | ||
$(TESTDIR)/run.py --wasm `pwd`/$(BUILDDIR)/$(OPT) $(if $(JS),--js '$(JS)',) | ||
dune exec ./smallint.exe | ||
test/%: $(NAME) | ||
$(TESTDIR)/run.py --wasm `pwd`/$(NAME) $(if $(JS),--js '$(JS)',) $(TESTDIR)/$*.wast | ||
|
||
test/%: $(OPT) | ||
$(TESTDIR)/run.py --wasm `pwd`/$(BUILDDIR)/$(OPT) $(if $(JS),--js '$(JS)',) $(TESTDIR)/$*.wast | ||
run/%: $(NAME) | ||
./$(NAME) $(TESTDIR)/$*.wast | ||
|
||
run/%: $(OPT) | ||
./$(OPT) $(TESTDIR)/$*.wast | ||
partest: $(NAME) unittest | ||
make -j10 quiettest | ||
|
||
partest: $(TESTS:%=quiettest/%) | ||
@echo All tests passed. | ||
quiettest: $(TESTS:%=quiettest/%) | ||
@echo All tests passed. | ||
|
||
quiettest/%: $(OPT) | ||
@ ( \ | ||
$(TESTDIR)/run.py 2>$(@F).out --wasm `pwd`/$(BUILDDIR)/$(OPT) $(if $(JS),--js '$(JS)',) $(TESTDIR)/$*.wast && \ | ||
rm $(@F).out \ | ||
) || \ | ||
cat $(@F).out || rm $(@F).out || exit 1 | ||
quiettest/%: $(NAME) | ||
@ ( \ | ||
$(TESTDIR)/run.py 2>$(@F).out --wasm `pwd`/$(NAME) $(if $(JS),--js '$(JS)',) $(TESTDIR)/$*.wast && \ | ||
rm $(@F).out \ | ||
) || \ | ||
(cat $(@F).out && rm $(@F).out && exit 1) | ||
|
||
smallinttest: smallint | ||
dune exec ./smallint.exe | ||
|
||
dunetest: | ||
dune test | ||
# Packaging | ||
|
||
.PHONY: install | ||
|
||
install: | ||
dune build -p $(NAME) @install | ||
|
@@ -101,15 +113,16 @@ opam-release/%: | |
rm opam-$*.zip | ||
@echo Created file ./opam, submit to github opam-repository/packages/wasm/wasm.$*/opam | ||
|
||
# Miscellaneous targets | ||
|
||
.PHONY: clean | ||
|
||
$(ZIP): | ||
git archive --format=zip --prefix=$(NAME)/ -o $@ HEAD | ||
|
||
|
||
# Cleanup | ||
|
||
.PHONY: clean distclean | ||
|
||
clean: | ||
dune clean | ||
|
||
distclean: clean | ||
rm -f $(NAME) $(JSLIB) | ||
rm -f $(NAME) $(JSLIB) $(ZIP) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,36 @@ | ||
open Source | ||
open Types | ||
open Ast | ||
|
||
module Unknown = Error.Make () | ||
exception Unknown = Unknown.Error (* indicates unknown import name *) | ||
|
||
module Registry = Map.Make(struct type t = Ast.name let compare = compare end) | ||
let registry = ref Registry.empty | ||
|
||
let register name lookup = registry := Registry.add name lookup !registry | ||
type registry = (name -> extern_type -> Instance.extern option) Registry.t ref | ||
|
||
let lookup (m : module_) (im : import) : Instance.extern = | ||
type lookup_export = name -> extern_type -> Instance.extern option | ||
|
||
|
||
let global : registry = ref Registry.empty | ||
let registry () : registry = ref !global | ||
|
||
let register r name lookup = r := Registry.add name lookup !r | ||
let register_global name lookup = register global name lookup | ||
|
||
let lookup r module_name item_name et : Instance.extern option = | ||
match Registry.find_opt module_name !r with | ||
| Some f -> f item_name et | ||
| None -> None | ||
|
||
let lookup_import r (m : module_) (im : import) : Instance.extern = | ||
let {module_name; item_name; idesc} = im.it in | ||
let t = import_type m im in | ||
try Registry.find module_name !registry item_name t with Not_found -> | ||
Unknown.error im.at | ||
("unknown import \"" ^ string_of_name module_name ^ | ||
"\".\"" ^ string_of_name item_name ^ "\"") | ||
let et = import_type m im in | ||
match lookup r module_name item_name et with | ||
| Some ext -> ext | ||
| None -> | ||
Unknown.error im.at | ||
("unknown import \"" ^ string_of_name module_name ^ | ||
"\".\"" ^ string_of_name item_name ^ "\"") | ||
|
||
let link m = List.map (lookup m) m.it.imports | ||
let link r m = List.map (lookup_import r m) m.it.imports |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,17 @@ | ||
open Types | ||
open Ast | ||
|
||
type registry | ||
|
||
exception Unknown of Source.region * string | ||
|
||
val link : Ast.module_ -> Instance.extern list (* raises Unknown *) | ||
val registry : unit -> registry | ||
|
||
val lookup : registry -> name -> name -> extern_type -> Instance.extern option | ||
val link : registry -> module_ -> Instance.extern list (* raises Unknown *) | ||
|
||
|
||
type lookup_export = name -> extern_type -> Instance.extern option | ||
|
||
val register : | ||
Ast.name -> | ||
(Ast.name -> Types.extern_type -> Instance.extern (* raises Not_found *)) -> | ||
unit | ||
val register : registry -> name -> lookup_export -> unit | ||
val register_global : name -> lookup_export -> unit |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to have been slow to iterate on this PR. Is this behaviour preferable to having the
failwith
behaviour? My understanding is that this effectively causes garbled JS tests to be silently spat out instead of the translation explicitly failing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The practical problem is that as is, this makes the test runner fail, which also breaks CI. The conceptual problem is that the current assumption is that every core test can be converted to JS. If we want to exempt some tests, we'll need additional infrastructure for that. But what would that imply for the role of the test suite?
Is there truly no way to transform the thread construct to JS faithfully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is - it would have to be a follow-up PR though. I'm just trying to clarify if generating garbled tests would be a preferable behaviour in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we at least turn the thread and wait constructs into some kind of dynamic "assert false" instead?
EDIT: maybe something like "assert_malformed <obviously malformed module>" with a comment that this is caused by a NYI construct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. The idea of opening an issue for it was to do it separately. Adding a "correct" temporary work-around probably is more work than is worthwhile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The thread construct is in a completely different category than the arguments of assertions, so we cannot simply wrap it into one.)