Repository navigation
Remove x64 exception handling code and support aarch64 - #562
syntactically wants to merge 6 commits into
Conversation
77d9f9c to
e836b62
Compare
f1bc9ae to
3975edd
Compare
5a2b052 to
a43136d
Compare
8c0eed1 to
f3e17b7
Compare
f3e17b7 to
8c7a8fe
Compare
47cfe5c to
76ce45d
Compare
This requires changing the wasi-sdk builder container definition/script a little bit to make sure it works on aarch64 as well as x86_64. Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
87e2264 to
8ebab69
Compare
8ebab69 to
0552d20
Compare
d2c6ee7 to
5213f3b
Compare
jsturtevant
left a comment
There was a problem hiding this comment.
looks like ci still needs some work but over all this LGTM
1952f4d to
fd74970
Compare
As of Hyperlight 0.18.0, all of the functionality we need is supported(ish) with architecture-independent interfaces in core Hyperlight. Remove the old architecture-specific code in favour of those new interfaces. Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
Not all guests are x64 anymore. Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
a3181ae to
8138734
Compare
b5a0428 to
b2f884a
Compare
Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
Signed-off-by: Lucy Menon <168595099+syntactically@users.noreply.github.com>
b2f884a to
e41caf1
Compare
ludfjig
left a comment
There was a problem hiding this comment.
LGTM but ai had some comments
| fi | ||
|
|
||
| if [ -f "/.dockerenv" ] || grep -q docker /proc/1/cgroup; then | ||
| if [[ "$CI" == "true" && "$RUNNER_ARCH" == "ARM64" && -z "${CI_BUILD_WASM_MODULES:-}" ]]; then |
There was a problem hiding this comment.
Could you use ${CI:-} and ${RUNNER_ARCH:-} here? With nounset, this exits when CI is unset, or when CI=true and RUNNER_ARCH is unset. GitHub Actions sets both, but local shells need not.
| build-wasm-examples: | ||
| merge-docker-image: | ||
| needs: build-docker-image | ||
| if: ${{ needs.build-docker-image.outputs.do_push }} |
There was a problem hiding this comment.
Could you emit true in the writable branch and compare this output with 'true'? Both branches currently emit the string false, which is truthy here. Fork and Dependabot runs would attempt a registry push.
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: guest-modules-${{ matrix.wasmtime_version.name }} | ||
| name: guest-modules-${{ matrix.arch.name }}-${{ matrix.wasmtime_version.name }} |
There was a problem hiding this comment.
Could you update the release workflow to download guest-modules-X64-lts into build/${{ env.CONFIG }}? It still requests guest-modules-lts, which this job does not produce.
| ptr: *const u8, | ||
| len: usize, | ||
| ) -> core::result::Result<(), wasmtime::Error> { | ||
| wasmtime_mprotect(ptr as *mut u8, len, WASMTIME_PROT_READ | WASMTIME_PROT_EXEC); |
There was a problem hiding this comment.
Could you add instruction-cache synchronization here for ARM processors that need it? Hyperlight's paging barriers only synchronize translations. Wasmtime assigns cache coherence to this hook.
| || matrix.hypervisor == 'hyperv' | ||
| && format('["self-hosted", "Windows", "ARM64", "1ES.Pool=hl-windows-arm", "JobId=build-{0}-{1}-{2}-{3}"]', | ||
| matrix.config, | ||
| matrix.run_id, |
There was a problem hiding this comment.
Could you use github.run_* in both ARM Windows labels? These fields are not defined in matrix and expand to empty strings. Including matrix.wasmtime in the build ID and using the benchmark prefix would also distinguish the jobs.
| && apt-get install -y g++-multilib \ | ||
| && apt-get install -y libgcc-${GCC_VERSION}-dev \ | ||
| && apt-get install -y lib32gcc-${GCC_VERSION}-dev | ||
| && ([[ $TARGETPLATFORM == "linux/amd64" ]] \ |
There was a problem hiding this comment.
Could you use a POSIX if here and restore the package name lib32gcc-${GCC_VERSION}-dev? Ubuntu's /bin/sh rejects [[, and || true hides the failure. If these packages are unused, could we remove the branch?
|
|
||
| clippy target=default-target: (check target) | ||
| cargo clippy --workspace --exclude hyperlight-wasm-runtime --exclude hyperlight-wasm-macro --profile={{ if target == "debug" {"dev"} else { target } }} --all-targets --all-features -- -D warnings | ||
| cargo clippy --workspace --exclude hyperlight-wasm-runtime --exclude hyperlight-wasm-macro --profile={{ if target == "debug" {"dev"} else { target } }} --all-targets --features "$(cargo metadata --format-version 1 --no-deps | jq -r '[.packages[]|.name as$pkg|.features|keys.[]|select(.!="default" and (IN($ARGS.positional[0]//" "|split(",")[])|not))|$pkg+"/"+.]|join(",")' --args {{clippy-excluded-features}})" -- -D warnings |
There was a problem hiding this comment.
Could you use keys[] here? jq 1.6 rejects keys.[], leaving the feature list empty.
No description provided.