Skip to content

builder: run binaryen after the linker when generating wasm #2035

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

Closed
wants to merge 7 commits into from

Conversation

niaow
Copy link
Member

@niaow niaow commented Aug 7, 2021

Right now it builds and links.
It needs a lot of cleanup still, and debug symbol support had to be disabled for it to build.
This can use the wasm-opt command when dynamically built and libbinaryen when built normally.

@niaow
Copy link
Member Author

niaow commented Aug 7, 2021

To build binaryen, you can use:

make lib/binaryen/lib/libbinaryen.a

@niaow
Copy link
Member Author

niaow commented Aug 13, 2021

So I now have CI working on all non-windows platforms. For some reason it looks like the build process is stalling in a smoketest with WASM on Windows.

@niaow
Copy link
Member Author

niaow commented Aug 30, 2021

This has now been rebased on the latest dev.

@niaow
Copy link
Member Author

niaow commented Aug 30, 2021

It looks like the windows problem is that it is doing. . . nothing:
image

@niaow
Copy link
Member Author

niaow commented Aug 30, 2021

The windows bug appears to be a memory access violation in the ExternalInt64AsPtr transform pass

@niaow
Copy link
Member Author

niaow commented Aug 30, 2021

There also appears to be an issue with CGo support of some sort, which I have not been able to untangle.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Interesting work! Yeah, getting things to build reliable in a cross-platform way can be very painful. I have had plenty of experience with this (especially when originally porting TinyGo to Windows, that took me a few days of debugging). It would be pretty awesome to have wasm-opt built into TinyGo.

Comment on lines +60 to +62
// wasmOptLock exists because the binaryen C API uses global state.
// In the future, we can hopefully avoid this by re-execing.
var wasmOptLock sync.Mutex
Copy link
Member

Choose a reason for hiding this comment

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

We already do this for Clang and LLD, so it wouldn't be too much trouble to add another for wasm-opt. Either way, that's easy to do at a later time (no need to do it from the beginning).

Comment on lines +98 to +101
ifneq ("$(wildcard lib/binaryen/lib/libbinaryen.a)","")
CGO_CPPFLAGS+=-I$(abspath lib/binaryen/src/)
CGO_LDFLAGS+=$(abspath lib/binaryen/lib/libbinaryen.a)
endif
Copy link
Member

Choose a reason for hiding this comment

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

I think these flags should always be added when linking statically against LLVM.

Comment on lines +179 to +180
lib/binaryen/lib/libbinaryen.a: lib/binaryen/build.ninja
ninja -C lib/binaryen lib/libbinaryen.a
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be a dependency of the tinygo: make target.

@aykevl
Copy link
Member

aykevl commented Oct 6, 2021

I tried compiling wasm-opt using LLVM, using the following line in the Makefile:

cd lib/binaryen; cmake -G Ninja -DBUILD_STATIC_LIB=ON -DBUILD_LLVM_DWARF=ON -DLLVM_SOURCE="$(abspath $(LLVM_PROJECTDIR)/llvm)" -DLLVM_BUILD="$(abspath $(LLVM_BUILDDIR))" .

And the following patch to binaryen:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 45b4a0282..35ebdb51e 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -120,11 +120,17 @@ if(NOT EMSCRIPTEN)
   endif()
 endif()
 
+set(LLVM_SOURCE "${CMAKE_CURRENT_SOURCE_DIR}/third_party/llvm-project" CACHE STRING "LLVM source directory (llvm-project/llvm)")
+set(LLVM_BUILD "" CACHE STRING "LLVM build directory")
+
 # Compiler setup.
 
 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/src)
 if(BUILD_LLVM_DWARF)
-  include_directories(${CMAKE_CURRENT_SOURCE_DIR}/third_party/llvm-project/include)
+  include_directories(${LLVM_SOURCE}/include)
+  if(LLVM_BUILD)
+    include_directories(${LLVM_BUILD}/include)
+  endif()
 endif()
 
 # Add output directory to include path so config.h can be found
@@ -293,7 +299,7 @@ add_subdirectory(src/emscripten-optimizer)
 add_subdirectory(src/passes)
 add_subdirectory(src/support)
 add_subdirectory(src/wasm)
-add_subdirectory(third_party)
+#add_subdirectory(third_party)
 
 # Configure lit tests
 add_subdirectory(test/lit)
@@ -308,9 +314,9 @@ set(binaryen_objs
     $<TARGET_OBJECTS:cfg>
     $<TARGET_OBJECTS:support>)
 
-if(BUILD_LLVM_DWARF)
-  SET(binaryen_objs ${binaryen_objs} $<TARGET_OBJECTS:llvm_dwarf>)
-endif()
+#if(BUILD_LLVM_DWARF)
+#  SET(binaryen_objs ${binaryen_objs} $<TARGET_OBJECTS:llvm_dwarf>)
+#endif()
 
 # Sources.
 
diff --git a/src/wasm/wasm-debug.cpp b/src/wasm/wasm-debug.cpp
index 60e97a27e..d38af1c36 100644
--- a/src/wasm/wasm-debug.cpp
+++ b/src/wasm/wasm-debug.cpp
@@ -20,7 +20,7 @@
 #ifdef BUILD_LLVM_DWARF
 #include "llvm/ObjectYAML/DWARFEmitter.h"
 #include "llvm/ObjectYAML/DWARFYAML.h"
-#include "llvm/include/llvm/DebugInfo/DWARFContext.h"
+#include "llvm/DebugInfo/DWARF/DWARFContext.h"
 
 std::error_code dwarf2yaml(llvm::DWARFContext& DCtx, llvm::DWARFYAML::Data& Y);
 #endif

but it's failing because Binaryen is built for an older version of LLVM. So it doesn't look like static linking while preserving DWARF debuginfo would be possible at all (and preserving DWARF is IMHO a necessity, certainly if you ask @dkegel-fastly).

@@ -621,6 +621,30 @@ func Build(pkgName, outpath string, config *compileopts.Config, action func(Buil
}
}

// Run wasm-opt if necessary.
if config.GOARCH() == "wasm" {
Copy link
Member

Choose a reason for hiding this comment

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

This should be strings.HasPrefix(config.Triple(), "wasm") because WASI uses GOOS=linux GOARCH=arm because of reasons (see #1964 for details).

@aykevl
Copy link
Member

aykevl commented Oct 7, 2021

@niaow is this something you intend to continue working on? Otherwise I can take over this PR if needed.

Right now it seems to me that it is not possible to link against libbinaryen while libbinaryen is built with DWARF support. Because we do want DWARF support, I do not think linking against libbinaryen is a reasonable option. Therefore, we always need to call the external wasm-opt binary.
We can either bundle wasm-opt (~12MB original, ~8MB stripped, or ~3MB compressed) or expect the user to install this manually. A manual install is less of a problem than it may seem: binaryen is already packaged for MacOS (Homebrew) and Windows (Chocolatey). And it is only necessary for WebAssembly. But bundling will definitely be less of a hassle for new users.

@ghost
Copy link

ghost commented Oct 7, 2021

@aykevl Recommended way to install tinygo on Windows - scoop manager, but unfortunately binaryen doesn't supported there, but any way, it's not a very promise feature to be able to abfuscate Wasm-code to run it after that locally in Browser, more important to be able to optimize or shrink code for microcontrollers (here we already have LLVM).

@dkegel-fastly
Copy link
Contributor

I think the motivation for allowing binaryen as a postprocessing pass is to try fixing #2101 using asyncify, see #2011
so this is about correctness more than optimization...

@aykevl
Copy link
Member

aykevl commented Oct 7, 2021

Recommended way to install tinygo on Windows - scoop manager, but unfortunately binaryen doesn't supported there,

Binaryen can easily be added to Scoop. No problem (but see WebAssembly/binaryen#4148).

it's not a very promise feature to be able to abfuscate Wasm-code to run it after that locally in Browser, more important to be able to optimize or shrink code for microcontrollers (here we already have LLVM).

Maybe not for you, but some people really want to use TinyGo for WebAssembly.

Also, @dkegel-fastly is correct. The goal is not to shrink binary size. The goal is to be able to use Asyncify which is part of binaryen.

@ghost
Copy link

ghost commented Oct 7, 2021

@aykevl @dkegel-fastly thanks, now I see it, Binaryen's Asyncify really looks interesting.

@ghost
Copy link

ghost commented Oct 7, 2021

@aykevl I am a big fan of WebAssembly, some time ago I even thought about using it directly on Microcontrollers, like now wasm3 do, but after all I think that your approarch with tinygo looks more progmatic.

@aykevl
Copy link
Member

aykevl commented Oct 7, 2021

I have created a PR to add Binaryen to Scoop: ScoopInstaller/Main#2740

@aykevl I am a big fan of WebAssembly, some time ago I even thought about using it directly on Microcontrollers, like now wasm3 do

I actually made something very similar! Please take a look here: https://play.tinygo.org/. It implements the same machine package but runs in the browser using WebAssembly.

@niaow
Copy link
Member Author

niaow commented Oct 12, 2021

@niaow is this something you intend to continue working on? Otherwise I can take over this PR if needed.

Right now it seems to me that it is not possible to link against libbinaryen while libbinaryen is built with DWARF support. Because we do want DWARF support, I do not think linking against libbinaryen is a reasonable option. Therefore, we always need to call the external wasm-opt binary. We can either bundle wasm-opt (~12MB original, ~8MB stripped, or ~3MB compressed) or expect the user to install this manually. A manual install is less of a problem than it may seem: binaryen is already packaged for MacOS (Homebrew) and Windows (Chocolatey). And it is only necessary for WebAssembly. But bundling will definitely be less of a hassle for new users.

I think i can get back to the asyncify work a bit now, but I updated #2011 before to shell out to binaryen for now, which I think is the most reasonable option until binaryen can be built with tinygo's copy of LLVM. I think it is probably more appropriate just to close this for now.

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.

3 participants