Skip to content
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

Add aarch64 build to releases #6311

Closed
LumiWasTaken opened this issue Feb 15, 2024 · 20 comments · Fixed by #6334
Closed

Add aarch64 build to releases #6311

LumiWasTaken opened this issue Feb 15, 2024 · 20 comments · Fixed by #6334

Comments

@LumiWasTaken
Copy link

Hiya,

we've been trying to deploy it on arm servers but ran into the issues of it looking for aarach64 versions... how much of a hassle would it be to add arm releases?

@kripken
Copy link
Member

kripken commented Feb 15, 2024

What type of aarch64 servers? We do build aarch64 for MacOS,

- name: build-arm64
run: cmake --build out-arm64 -v --config Release --target install
if: matrix.os == 'macos-latest'

If you mean Linux then I'm not sure if GitHub Actions supports that, but maybe there has been an update I missed?

@LumiWasTaken
Copy link
Author

Interesting, because i recall

- name: build-arm64 run: cmake --build out-arm64 -v --config Release --target install if: matrix.os == 'linux-latest'

Being a thing somewhere... but given that i have absolutely 0 idea about the actual workflows behind this maybe someone else knows more?

The issue is that it's trying to be smart and download
https://github.com/WebAssembly/binaryen/releases/download/version_116/binaryen-version_116-aarch64-linux.tar.gz

Which does not exist :/

@DazWorrall
Copy link
Contributor

DazWorrall commented Feb 20, 2024

If you mean Linux then I'm not sure if GitHub Actions supports that, but maybe there has been an update I missed?

There are arm runners in private beta, but it looks like they will not be available to open source for the foreseeable (first going to Team and Enterprise plans).

I took a stab at building 116 on aarch64 using qemu via GitHub actions, but a) it's of course very slow, not sure if that would be acceptable and b) there was a test failure which needs attention from someone with more context than me!

@kripken
Copy link
Member

kripken commented Feb 20, 2024

I don't think speed would be an issue as this would only be for releases and not PRs, correct? Slower releases sound ok to me.

Btw, another option here might be the Zig cross-compilation toolchain? Emulation would be needed to run any tests, but that toolchain should be able to build aarch64 binaries at full speed at least.

The CI error looks like node: bad option: --experimental-wasm-threads which suggests it might be a too-old (or too-new?) node version. Finding which version is used there could help.

@DazWorrall
Copy link
Contributor

I did also try building the tip of main a few weeks ago and got an actual build error fwiw, 116 built but failed the tests. Let me refresh my branch and try building main again, knowing that a slow release build would be acceptable means it's worth me spending a bit more time on this.

@DazWorrall
Copy link
Contributor

Ok, tip of main still failing to build: https://github.com/DazWorrall/binaryen/actions/runs/7979012200/job/21785404126#step:9:240

/src/src/support/small_vector.h:32:38: error: '<unnamed>.wasm::SmallVector<wasm::Expression*, 10>::fixed' may be used uninitialized [-Werror=maybe-uninitialized]

Build is ok on x86_64 (or at least, the x86_64 version of the node:lts-alpine image). I can reproduce on my M2 Mac with podman.

@kripken
Copy link
Member

kripken commented Feb 20, 2024

Compiler error doesn't look arch-specific, that's odd. I would guess it is compiler-specific. That log has gcc 13.2.1 but I can't reproduce with 13.2.0 locally, so maybe it is arch-specific somehow..? Anyhow, if you find a reasonable workaround in the source code, we are ok with landing such fixes in general.

@DazWorrall
Copy link
Contributor

The gcc version in both the aarch64 and x86_64 container is 13.2.1, and the x86_64 build works 🤷 Running ninja -v install, if I'm reading the logs right, there is no difference in the compiler flags:

# x86_64
[211/293]
/usr/bin/c++  -I/src/src -I/src/third_party/llvm-project/include -I/src -static -DBUILD_LLVM_DWARF -Wall -Werror -Wextra -Wno-unused-parameter -Wno-dangling-pointer -fno-omit-frame-pointer -fno-rtti -Wno-implicit-int-float-conversion -Wno-unknown-warning-option -Wswitch -Wimplicit-fallthrough -Wnon-virtual-dtor -fPIC -fdiagnostics-color=always -O3 -DNDEBUG -UNDEBUG -std=c++17 -MD -MT src/passes/CMakeFiles/passes.dir/Precompute.cpp.o -MF src/passes/CMakeFiles/passes.dir/Precompute.cpp.o.d -o src/passes/CMakeFiles/passes.dir/Precompute.cpp.o -c /src/src/passes/Precompute.cpp

# aarch64
ninja: job failed:
/usr/bin/c++  -I/src/src -I/src/third_party/llvm-project/include -I/src -static -DBUILD_LLVM_DWARF -Wall -Werror -Wextra -Wno-unused-parameter -Wno-dangling-pointer -fno-omit-frame-pointer -fno-rtti -Wno-implicit-int-float-conversion -Wno-unknown-warning-option -Wswitch -Wimplicit-fallthrough -Wnon-virtual-dtor -fPIC -fdiagnostics-color=always -O3 -DNDEBUG -UNDEBUG -std=c++17 -MD -MT src/passes/CMakeFiles/passes.dir/Precompute.cpp.o -MF src/passes/CMakeFiles/passes.dir/Precompute.cpp.o.d -o src/passes/CMakeFiles/passes.dir/Precompute.cpp.o -c /src/src/passes/Precompute.cpp

I am outside my wheelhouse here though. Maybe I've read the logs wrong.

The build works on 116, so something changed in here to upset the aarch64 builds: version_116...c0cdd26

I can help with the plumbing here but am not familiar at all with the source to suggest a fix.

@kripken
Copy link
Member

kripken commented Feb 20, 2024

If this used to work then bisecting to find the bad commit might help.

@DazWorrall
Copy link
Contributor

DazWorrall commented Feb 21, 2024

Ok, I grabbed some native hardware for this (a ~90 minute emulated build time would have taken quite a while to bisect!): 141f7ca (#6212) is the first commit which fails to build on aarch64:

ninja: job failed: /usr/bin/c++  -I/root/binaryen/src -I/root/binaryen/third_party/llvm-project/include -I/root/binaryen -static -DBUILD_LLVM_DWARF -Wall -Werror -Wextra -Wno-unused-parameter -Wno-dangling-pointer -fno-omit-frame-pointer -fno-rtti -Wno-implicit-int-float-conversion -Wno-unknown-warning-option -Wswitch-Wimplicit-fallthrough -Wnon-virtual-dtor -fPIC -fdiagnostics-color=always -O3 -DNDEBUG -UNDEBUG -std=c++17 -MD -MT src/passes/CMakeFiles/passes.dir/Precompute.cpp.o -MF src/passes/CMakeFiles/passes.dir/Precompute.cpp.o.d -o src/passes/CMakeFiles/passes.dir/Precompute.cpp.o -c /root/binaryen/src/passes/Precompute.cpp
In file included from /root/binaryen/src/wasm-traversal.h:30,
                 from /root/binaryen/src/pass.h:24,
                 from /root/binaryen/src/ir/intrinsics.h:20,
                 from /root/binaryen/src/ir/effects.h:20,
                 from /root/binaryen/src/passes/Precompute.cpp:30:
In copy constructor 'wasm::SmallVector<wasm::Expression*, 10>::SmallVector(const wasm::SmallVector<wasm::Expression*, 10>&)',
    inlined from 'constexpr std::pair<_T1, _T2>::pair(const _T1&, const _T2&) [with _U1 = wasm::Select* const; _U2 = wasm::SmallVector<wasm::Expression*, 10>; typename std::enable_if<(std::_PCC<true, _T1, _T2>::_ConstructiblePair<_U1, _U2>() && std::_PCC<true, _T1, _T2>::_ImplicitlyConvertiblePair<_U1, _U2>()), bool>::type <anonymous> = true; _T1 = wasm::Select* const; _T2 = wasm::SmallVector<wasm::Expression*, 10>]' at /usr/include/c++/13.2.1/bits/stl_pair.h:559:21,
    inlined from 'T& wasm::InsertOrderedMap<Key, T>::operator[](const Key&) [with Key = wasm::Select*; T = wasm::SmallVector<wasm::Expression*, 10>]' at /root/binaryen/src/support/insert_ordered.h:112:29,
    inlined from 'void wasm::Precompute::partiallyPrecompute(wasm::Function*)::StackFinder::visitSelect(wasm::Select*)' at /root/binaryen/src/passes/Precompute.cpp:472:24,
    inlined from 'static void wasm::Walker<SubType, VisitorType>::doVisitSelect(SubType*, wasm::Expression**) [with SubType = wasm::Precompute::partiallyPrecompute(wasm::Function*)::StackFinder; VisitorType = wasm::Visitor<wasm::Precompute::partiallyPrecompute(wasm::Function*)::StackFinder, void>]' at /root/binaryen/src/wasm-delegations.def:50:1:
/root/binaryen/src/support/small_vector.h:32:38: error: '<unnamed>.wasm::SmallVector<wasm::Expression*, 10>::fixed' may be used uninitialized [-Werror=maybe-uninitialized]
   32 | template<typename T, size_t N> class SmallVector {
      |                                      ^~~~~~~~~~~
In file included from /root/binaryen/src/passes/Precompute.cpp:38:
/root/binaryen/src/support/insert_ordered.h: In static member function 'static void wasm::Walker<SubType, VisitorType>::doVisitSelect(SubType*, wasm::Expression**) [with SubType = wasm::Precompute::partiallyPrecompute(wasm::Function*)::StackFinder; VisitorType = wasm::Visitor<wasm::Precompute::partiallyPrecompute(wasm::Function*)::StackFinder, void>]':
/root/binaryen/src/support/insert_ordered.h:112:29: note: '<anonymous>' declared here
  112 |     std::pair<const Key, T> kv = {k, {}};
      |                             ^~
At global scope:
cc1plus: note: unrecognized command-line option '-Wno-unknown-warning-option' may have been intended to silence earlier diagnostics
cc1plus: note: unrecognized command-line option '-Wno-implicit-int-float-conversion' may have been intended to silence earlier diagnostics
cc1plus: all warnings being treated as errors
141f7cad3aa516db3828e619b31fe681e46a151b is the first bad commit

@DazWorrall
Copy link
Contributor

I should add that the build above was ran in the same docker container used in CI (node:lts-alpine) for consistency, but I was also able to replicate on debian bullseye (gcc 10.2.1), reducing the chances that the issue is environmental.

@kripken
Copy link
Member

kripken commented Feb 21, 2024

Thanks, interesting... I see nothing in that commit that is special. But somehow that warning appears on that SmallVector usage...

To me this looks like a spurious warning. But it's beyond my C++ knowledge to guess as to why it complains here specifically, or to know how to work around it, sorry. Maybe you just need to disable warnings in general for this build?

@DazWorrall
Copy link
Contributor

If you're happy with that I will try to do so conservatively.

@DazWorrall
Copy link
Contributor

Proposing #6330

@DazWorrall
Copy link
Contributor

PR to add aarch64 builds on release: #6334

@sbc100
Copy link
Member

sbc100 commented Feb 22, 2024

@LumiWasTaken what distro are you using? Does it not contain a packaged version of binaryen that you can install?

@LumiWasTaken
Copy link
Author

LumiWasTaken commented Feb 22, 2024

@LumiWasTaken what distro are you using? Does it not contain a packaged version of binaryen that you can install?

Problem is that it's during the docker deployment. The Dockerfile looks like this:

FROM rust:1.76.0-slim-bookworm as builder
WORKDIR /app

RUN apt-get update
RUN apt-get install -y libpq-dev

RUN cargo install --locked trunk
RUN rustup target add wasm32-unknown-unknown

COPY ./ .
RUN cd front && trunk build --release
RUN cargo build --release

FROM debian:bookworm-slim
WORKDIR /app

RUN apt-get update
RUN apt-get install -y libssl-dev libpq-dev ca-certificates

COPY --from=builder /app/target/release/example/app/target/release/example
COPY --from=builder /app/front/dist/ /app/front/dist/

CMD ["./target/release/example"]

ENV PORT=8080

And the rust package automatically downloads the binairy which tries to do something like ${arch} which on normal systems would download the x86_64 release but since we do not release a aarch64 (and it looks for aarch64) it fails to find.

@sbc100
Copy link
Member

sbc100 commented Feb 22, 2024

Ah! That is interesting. So its rust that is trying to download these binaries and failing. I guess this is effecting all rust + wasm users of arm64 linux.. I wonder why we haven't heard more folks compaining about it. Perhaps there is some workaround on the rust side (e.g. using x86_64 binaries in emulation mode). @alexcrichton are you aware of this issue?

@LumiWasTaken
Copy link
Author

I just feel like nobody was insane enough to actually attempt and compile it on a arm64 server.

Since: The binairy is only required during compilation. So Technically i'd be able to build the image locally on my x84_64 PC, push the image to the server and run it there (The Binary is required to build the Webassembly which in the final image will be already generated)

This whole thing would in the end allow us to build the Image on efficient ARM servers rather than having to mess around with it.

(In the future this would also make it more accessible to users that dare to compile on a Raspberry pi for example)

@alexcrichton
Copy link
Contributor

I've not seen this issue myself yet, no, but this looks like it's part of the trunk tool which constructs a binaryen download url here. I'm not familiar with trunk myself, but in my experience folks mostly run on aarch64 macos machines and aarch64 linux isn't the most common (not to say it's not important of course, I run on aarch64 linux regularly myself!)

kripken pushed a commit that referenced this issue Feb 23, 2024
radekdoulik pushed a commit to dotnet/binaryen that referenced this issue Jul 12, 2024
kripken added a commit that referenced this issue Oct 28, 2024
Similar to

* #6330
* #6311
* #6912
* #5946

This extends the region we ignore the gcc warning in.

The warning:

ninja: job failed: /usr/bin/c++  -I/src/src -I/src/third_party/FP16/include -I/src/third_party/llvm-project/include -I/src -static -DBUILD_LLVM_DWARF -Wall -Werror -Wextra -Wno-unused-parameter -Wno-dangling-pointer -fno-omit-frame-pointer -fno-rtti -Wno-implicit-int-float-conversion -Wno-unknown-warning-option -Wswitch -Wimplicit-fallthrough -Wnon-virtual-dtor -fPIC -fdiagnostics-color=always -O3 -DNDEBUG -UNDEBUG -std=c++17 -MD -MT src/passes/CMakeFiles/passes.dir/Precompute.cpp.o -MF src/passes/CMakeFiles/passes.dir/Precompute.cpp.o.d -o src/passes/CMakeFiles/passes.dir/Precompute.cpp.o -c /src/src/passes/Precompute.cpp
In file included from /src/src/literal.h:27,
                 from /src/src/wasm.h:36,
                 from /src/src/ir/boolean.h:20,
                 from /src/src/ir/bits.h:20,
                 from /src/src/ir/properties.h:20,
                 from /src/src/ir/iteration.h:20,
                 from /src/src/passes/Precompute.cpp:30:
In copy constructor 'wasm::SmallVector<T, N>::SmallVector(const wasm::SmallVector<T, N>&) [with T = wasm::Expression*; long unsigned int N = 10]',
    inlined from 'constexpr std::pair<_T1, _T2>::pair(const _T1&, const _T2&) [with _U1 = wasm::Select* const; _U2 = wasm::SmallVector<wasm::Expression*, 10>; typename std::enable_if<(std::_PCC<true, _T1, _T2>::_ConstructiblePair<_U1, _U2>() && std::_PCC<true, _T1, _T2>::_ImplicitlyConvertiblePair<_U1, _U2>()), bool>::type <anonymous> = true; _T1 = wasm::Select* const; _T2 = wasm::SmallVector<wasm::Expression*, 10>]' at /usr/include/c++/13.2.1/bits/stl_pair.h:559:21,
    inlined from 'T& wasm::InsertOrderedMap<Key, T>::operator[](const Key&) [with Key = wasm::Select*; T = wasm::SmallVector<wasm::Expression*, 10>]' at /src/src/support/insert_ordered.h:112:29,
    inlined from 'void wasm::Precompute::partiallyPrecompute(wasm::Function*)::StackFinder::visitSelect(wasm::Select*)' at /src/src/passes/Precompute.cpp:571:24,
    inlined from 'static void wasm::Walker<SubType, VisitorType>::doVisitSelect(SubType*, wasm::Expression**) [with SubType = wasm::Precompute::partiallyPrecompute(wasm::Function*)::StackFinder; VisitorType = wasm::Visitor<wasm::Precompute::partiallyPrecompute(wasm::Function*)::StackFinder, void>]' at /src/src/wasm-delegations.def:50:1:
/src/src/support/small_vector.h:69:35: error: '<unnamed>.wasm::SmallVector<wasm::Expression*, 10>::fixed' may be used uninitialized [-Werror=maybe-uninitialized]
   69 |     : usedFixed(other.usedFixed), fixed(other.fixed), flexible(other.flexible) {
      |                                   ^~~~~~~~~~~~~~~~~~
In file included from /src/src/passes/Precompute.cpp:37:
/src/src/support/insert_ordered.h: In static member function 'static void wasm::Walker<SubType, VisitorType>::doVisitSelect(SubType*, wasm::Expression**) [with SubType = wasm::Precompute::partiallyPrecompute(wasm::Function*)::StackFinder; VisitorType = wasm::Visitor<wasm::Precompute::partiallyPrecompute(wasm::Function*)::StackFinder, void>]':
/src/src/support/insert_ordered.h:112:29: note: '<anonymous>' declared here
  112 |     std::pair<const Key, T> kv = {k, {}};
      |                             ^~
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 a pull request may close this issue.

5 participants