Conversation
This enum describes which wasm features the IR is expected to include. The validator should reject operations which require excluded features, and passes should avoid producing IR which requires excluded features. This makes it easier to catch possible errors in Binaryen producers (e.g. emscripten). Asm2wasm has a flag to enable or disable atomics. Other tools currently just accept all features (as, dis and opt are just for inspecting or modifying existing modules, so it would be annoying to have to use flags with those tools and I expect the risk of accidentally introducing atomics to be low).
| .add("--emit-text", "-S", "Emit text instead of binary for the output file", | ||
| Options::Arguments::Zero, | ||
| [&](Options *o, const std::string &argument) { emitBinary = false; }) | ||
| .add("--enable-atomics", "-a", "Enable the Atomics wasm feature", |
There was a problem hiding this comment.
I called the flag --enable-threads in wabt, probably worthwhile to be consistent.
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
For wasm-as etc., why not default the features to All?
| @@ -1286,6 +1285,7 @@ void Asm2WasmBuilder::processAsm(Ref ast) { | |||
| }; | |||
|
|
|||
| PassRunner passRunner(&wasm); | |||
There was a problem hiding this comment.
Can use the second passRunner constructor, which receives passOptions as a second param. That would copy in the features.
There was a problem hiding this comment.
Wouldn't that also copy in OptLevel and ShrinkLevel, which we don't want?
| void setValidateGlobally(bool validate) { | ||
| options.validateGlobally = validate; | ||
| } | ||
| void setFeatures(FeatureSet features) { |
There was a problem hiding this comment.
we don't strictly need this (can use the constructor as mentioned above, or also do getPassOptions().features = ) but I suppose this could be a useful convenience
There was a problem hiding this comment.
I figured we'd want to be able to set this without optLevel (see above)
|
|
||
| namespace wasm { | ||
|
|
||
| enum Features : uint32_t { |
There was a problem hiding this comment.
32 bits should be enough for all the features? ;)
There was a problem hiding this comment.
Could just use a struct of bools... :-)
https://github.com/WebAssembly/wabt/blob/master/src/feature.h#L26
There was a problem hiding this comment.
Then we wouldn't be able to bitwise or them together like Real C Programmers should.... although if they are going to have names, then generating them from a def file like that might make sense. Maybe an improvement for the future.
|
|
||
| template <typename T, typename S> | ||
| std::ostream& fail(S text, T curr, Function* func) { | ||
| __builtin_trap(); |
There was a problem hiding this comment.
Debug break that I forgot to remove. Removed in
6f85f55
| } | ||
|
|
||
| void FunctionValidator::visitLoad(Load *curr) { | ||
| shouldBeTrue(!curr->isAtomic || info.features & Features::Atomics, curr, "Atomic operation (atomics are disabled)"); |
There was a problem hiding this comment.
i personally think it's clearer to write if (curr->isAtomic) shouldBeTrue(..atomics feature enabled..)
|
|
||
| namespace wasm { | ||
|
|
||
| enum Features : uint32_t { |
There was a problem hiding this comment.
Nit: Given we have Features and FeatureSet, would it make sense to call the enum of flags just Feature? Benefit would be that it would be clear that a variable named features is probably a FeatureSet
|
|
||
| std::vector<std::string> passes; | ||
| PassOptions passOptions; | ||
| Features features = Features::Atomics; |
There was a problem hiding this comment.
FeatureSet features maybe?
…/store validation for readability, validate shared mem
4 participants
This enum describes which wasm features the IR is expected to include. The
validator should reject operations which require excluded features, and passes
should avoid producing IR which requires excluded features.
This makes it easier to catch possible errors in Binaryen producers (e.g.
emscripten). Asm2wasm has a flag to enable or disable atomics. Other
tools currently just accept all features (as, dis and opt are just for
inspecting or modifying existing modules, so it would be annoying to have to use
flags with those tools and I expect the risk of accidentally introducing
atomics to be low).