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

machinst x64: investigate remaining test failures #2079

Closed
2 of 4 tasks
bnjbvr opened this issue Jul 29, 2020 · 6 comments
Closed
2 of 4 tasks

machinst x64: investigate remaining test failures #2079

bnjbvr opened this issue Jul 29, 2020 · 6 comments
Labels
cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator

Comments

@bnjbvr
Copy link
Member

bnjbvr commented Jul 29, 2020

Opening an issue as a placeholder for remaining issues observed during testing. Initial PR for landing excluded the following tests:

  • implement proper stack unwinding for the new backend: codegen/src/isa/x86/unwind/{systemv, winx64}.rs, tests/all/traps.rs (also: this will require implementing the win64 fastcall ABI in x64)
  • cranelift filetests. For those, we'd need to either make them dependent on the Cargo feature x64 being not present (for e.g. regalloc tests), or keep them enabled (e.g. run tests). It will require more triaging. One idea would be to create a new directory x64 in the filetests suite, and copy over the tests that can be ported there; then we'd need some kind of way to dispatch the x64 tests to the new backend only, and x86 to the old backend only.
  • tests/all/gc.rs: investigate the gc-during-gc test failure. Probably a miscompilation in new-backend x64, since aarch64 doens't seem to fail this test.
  • tests/all/iloop.rs, tests/all/stack_overflow.rs: probably require implementing probe stacks for both. The first file relates to interrupts, which might or might not be handled the same way as stack overflows in Wasmtime; I don't know.
@bnjbvr bnjbvr added the cranelift:area:x64 Issues related to x64 codegen label Jul 29, 2020
@jlb6740
Copy link
Contributor

jlb6740 commented Jul 30, 2020

@bnjbvr Hi .. How do you test to see all the failures? If you run:

cargo test --all --features=experimental_x64

or under the cranelift project:

RUST_BACKTRACE=1` RUST_LOG=debug cargo test --all --features=experimental_x64 --package cranelift-codegen -- --nocapture

you see failures I believe related to the first bullet on stack onwinding .. but those are the only ones I can see fail.

@bnjbvr
Copy link
Member Author

bnjbvr commented Jul 30, 2020

A commit has now landed in CI with the right command line to use for running all the test cases; it's a bit long and requires Rust nightly (because of the way Cargo feature unification works in stable vs nightly):

        cargo +nightly \
            -Zfeatures=all -Zpackage-features \
            test \
            --features test-programs/test_programs \
            --features experimental_x64 \
            --all \
            --exclude lightbeam \
            --exclude peepmatic \
            --exclude peepmatic-automata \
            --exclude peepmatic-fuzzing \
            --exclude peepmatic-macro \
            --exclude peepmatic-runtime \
            --exclude peepmatic-test

@jlb6740
Copy link
Contributor

jlb6740 commented Jul 30, 2020

Thanks ... see this now after updating the version of nightly. Did not see any errors but will take a closer look at the tests that are disabled.

@github-actions
Copy link

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift"

Thus the following users have been cc'd because of the following labels:

  • bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

cfallin added a commit to cfallin/wasmtime that referenced this issue Nov 13, 2020
Some of the test failures tracked by bytecodealliance#2079 are in unwind tests that are
specific to the old x86 backend: namely, these tests invoke the unwind
implementation that is paired with the old backend, rather than generic
over all backends. It thus doesn't make sense to try to run these tests
with the new backend. (The new backend's unwind code should have
analogous tests written/ported over eventually.)

It seems that we were actually building *both* x86 backends when the
`x64` feature was enabled, except that the old x86 backend would never
be instantiated by the usual ISA-lookup logic because a `x86-64` target
triple unconditionally resolves to the new one.

This PR resolves both of the issues by tweaking the feature-config
directives to exclude the `x86` backend when `x64` is enabled.
cfallin added a commit that referenced this issue Nov 30, 2020
Some of the test failures tracked by #2079 are in unwind tests that are
specific to the old x86 backend: namely, these tests invoke the unwind
implementation that is paired with the old backend, rather than generic
over all backends. It thus doesn't make sense to try to run these tests
with the new backend. (The new backend's unwind code should have
analogous tests written/ported over eventually.)

It seems that we were actually building *both* x86 backends when the
`x64` feature was enabled, except that the old x86 backend would never
be instantiated by the usual ISA-lookup logic because a `x86-64` target
triple unconditionally resolves to the new one.

This PR resolves both of the issues by tweaking the feature-config
directives to exclude the `x86` backend when `x64` is enabled.
@bjorn3
Copy link
Contributor

bjorn3 commented Mar 20, 2021

implement proper stack unwinding for the new backend: codegen/src/isa/x86/unwind/{systemv, winx64}.rs, tests/all/traps.rs (also: this will require implementing the win64 fastcall ABI in x64)

This is done.

tests/all/gc.rs: investigate the gc-during-gc test failure. Probably a miscompilation in new-backend x64, since aarch64 doens't seem to fail this test.

Is this done?

@cfallin
Copy link
Member

cfallin commented Mar 20, 2021

I think so; we don't have any excluded GC tests anymore.

@cfallin cfallin closed this as completed Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

No branches or pull requests

4 participants