Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

multiple wasm backends (aka interpreter) #1452

Merged

Conversation

wanderingbort
Copy link
Contributor

link in the Binaryen interpreter from the version installed as part of our build script into the wasm interface and allow hot swapping between that and the jitter (currently defaulting to the jitter but perhaps not for long?)

Note, this creates a code link to binaryen's 1.37.x release as the embedding API has changed at some point after that.

CMakeLists.txt Outdated
@@ -174,6 +174,8 @@ else()
message( FATAL_ERROR "No WASM compiler cound be found" )
endif()

FIND_PACKAGE(binaryen REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

this creates a code link to binaryen's 1.37.x

Is there a way we can enforce that somehow here? I know that will require changes to the FindBinaryen.cmake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sadly no, there seems to be no header level or file level build stamping. If we want to assume that binaryen's directory is a git repro we could look at the tag, however our standard install script does not survive the git repo

@wanderingbort
Copy link
Contributor Author

@heifner Travis is failing because the OCI repo has binary versions of binaryen but not the src directory needed

@ericiles It looks like Jenkins is failing because the previous version of the installer doesn't survive the parts of binaryen we need (the new installer does)

@spoonincode
Copy link
Contributor

Since we're basically dipping our fingers in to what is considered private source -- not the publicly defined API for the library -- why not just bring the source in as a submodule? It's bizarre and confusing for us to still depend on the external source tree of binaryen after doing a make install of binaryen. This isn't how it works for any other library.

@heifner
Copy link
Contributor

heifner commented Feb 21, 2018

We have decided to kill Travis build since we now have Jenkins in place.

@wanderingbort
Copy link
Contributor Author

So, the new Jenkins error, reproducible locally on Ubuntu 16.04 seems to have something to do with compiling binaryen as a dependency with the system compiler (aka GCC 5.4 on ubuntu 16.04) and linking those libraries to our executable compiled with ClangX. Using GCC 7.x shows no problem and using Clang to build binaryen also shows no problem.

The net result is that I will be passing the buck to @spoonincode (sorry) to link Binaryen is a a submodule instead of a build dependency. This is probably more apropos as we were not using the released libraries/headers as is and were techincally linking against internal libraries.

Marking this PR as in progress as a result.

Const *value = module.allocator.alloc<Const>();
value->finalize();

value->set(Literal(11));
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point, we want to change this to 0 (or some invalid value??), I had put 11 just as a place holder if the code to accumulate the count was added to apply_context.

@asiniscalchi
Copy link
Contributor

asiniscalchi commented Feb 23, 2018

Looking to the FindBinaryen you coded it seems you are trying to add the targets generated by Binaryen build using a Find*.cmake syntax.
Actually it is very confusing cause Find*.cmake paradigm is used to find installed packages (headers, binaries, and libs) not internal libraries into a building tree.
I suggest to add the Binaryen source into EOS source (submodule + add_subdirectory) or use cmake paradigm: ExternalProject

@spoonincode
Copy link
Contributor

@asiniscalchi yes that's exactly what we're going to do; binaryen source (from eosio fork) will be included as submodule

@spoonincode
Copy link
Contributor

spoonincode commented Feb 28, 2018

Be advised -- there is still much work to be done to clean this up. And WAVM is still the runtime used. But I think we can merge this now and clean up via smaller PRs based on master.

@spoonincode spoonincode removed their assignment Feb 28, 2018
@wanderingbort wanderingbort merged commit 8c08fda into EOSIO:master Mar 1, 2018
@wanderingbort wanderingbort deleted the feature/multiple-wasm-backends branch March 1, 2018 14:22
@SheldonHH SheldonHH mentioned this pull request Apr 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants