Stream: git-wasmtime

Topic: wasmtime / PR #1916 Fix running filetests with custom flags

Wasmtime GitHub notifications bot (Jun 24 2020 at 18:58):

abrown opened PR #1916 from fix-1891 to master:

This change fixes the issues described in #1891:

• it makes the test-running infrastructure aware of what filetests will not be able to run on the current host machine by means of new implementations of TargetIsa::is_compatible_with and Flags::is_compatible_with (generated)
• it fixes the issue @bnjbvr described where the TargetIsa created by Parser is unaware of what host it is running on: for test run filetests, this is fixed with a use of Triple::host()

I now feel like is_compatible_with (like matches) is the wrong wording for that function; it should express that one configuration subsumes another so that it is clear that the opposite may not be true. I'm open to suggestions on that. I also likely need to adjust the ordering of commits at some point.

Wasmtime GitHub notifications bot (Jun 24 2020 at 18:58):

abrown requested bnjbvr for a review on PR #1916.

Wasmtime GitHub notifications bot (Jun 24 2020 at 19:01):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Jun 24 2020 at 19:01):

bjorn3 created PR Review Comment:

                    fmtln!(fmt, "Detail::Preset => {},");

This should not short-circuit.

Wasmtime GitHub notifications bot (Jun 24 2020 at 19:04):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jun 24 2020 at 19:04):

abrown created PR Review Comment:

I ended up omitting the self.shared_flags.is_compatible_with(&other.shared_flags) here because here I'm saying "self.isa_flags is more specific than other.isa_flags" (e.g. the host machine has more ISA flags set than the filetest will) but that same logic doesn't apply to the shared flags (e.g. the default shared flags will almost always have less flags set than the filetest).

Wasmtime GitHub notifications bot (Jun 24 2020 at 19:05):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jun 24 2020 at 19:05):

abrown created PR Review Comment:

I was under the impression that once the descriptors move into presets then it will be presets from there on?

Wasmtime GitHub notifications bot (Jun 25 2020 at 01:25):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 01:25):

abrown created PR Review Comment:

And here's an example of the skipped tests on Windows: https://github.com/bytecodealliance/wasmtime/pull/1916/checks?check_run_id=804670568#step:12:652.

Wasmtime GitHub notifications bot (Jun 25 2020 at 09:50):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Jun 25 2020 at 09:50):

bjorn3 created PR Review Comment:

It seems so. It isn't guaranteed anywhere though.

Wasmtime GitHub notifications bot (Jun 25 2020 at 18:48):

alexcrichton edited PR #1916 from fix-1891 to main:

This change fixes the issues described in #1891:

• it makes the test-running infrastructure aware of what filetests will not be able to run on the current host machine by means of new implementations of TargetIsa::is_compatible_with and Flags::is_compatible_with (generated)
• it fixes the issue @bnjbvr described where the TargetIsa created by Parser is unaware of what host it is running on: for test run filetests, this is fixed with a use of Triple::host()

I now feel like is_compatible_with (like matches) is the wrong wording for that function; it should express that one configuration subsumes another so that it is clear that the opposite may not be true. I'm open to suggestions on that. I also likely need to adjust the ordering of commits at some point.

Wasmtime GitHub notifications bot (Jul 02 2020 at 14:17):

bnjbvr created PR Review Comment:

Generating Rust code from the meta crate has a cost, in particular it's harder to read, correct errors and maintain over time. For a simple function that doesn't seem to use much of the meta machinery anyways, would it make sense to just implement as a plain function in the codegen (non-meta) crate instead? (With the extra argument that really only x86 needs it, so only this ISA can implement right now)

Wasmtime GitHub notifications bot (Jul 02 2020 at 14:17):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jul 02 2020 at 14:17):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jul 02 2020 at 14:17):

bnjbvr created PR Review Comment:

This check is a bit tricky and might be incorrect: if the setting was explicitly set to the default value, then it's different from being unset and inheriting the default value. This suggests that we'd need a different data structure with Options instead of direct values, and using this to store what's requested by the CLI arguments / test annotations, and comparing this against the real machine capabilities.

Wasmtime GitHub notifications bot (Jul 02 2020 at 14:17):

bnjbvr created PR Review Comment:

Same comment as below; could this be implemented in Rust with the same effect but making it easier to maintain?

Wasmtime GitHub notifications bot (Jul 02 2020 at 14:17):

bnjbvr created PR Review Comment:

I'm not sure if the question makes sense, but is there a chance that since the x64 new backend uses a different TargetIsa, this downcast may fail when comparing on a native x86 host?

Wasmtime GitHub notifications bot (Jul 10 2020 at 15:13):

abrown closed without merge PR #1916.

Last updated: Nov 22 2024 at 16:03 UTC