Move features from passOptions to Module#2001
Conversation
This allowing us to emit a (potentially modified) target features section and conditionally emit other sections such as the DataCount section based on the presence of features.
|
This is essentially #1993 without the controversial parts. |
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
lgtm aside from the questions below, plus: why did the .map files change?
| assert os.path.exists('a.wast') | ||
| subprocess.check_call( | ||
| WASM_OPT + ['a.wast', '--print-minified'], | ||
| WASM_OPT + ['a.wast', '--print-minified', '-all'], |
There was a problem hiding this comment.
seems like this may be unnecessary in this PR
| bool wasmHasFeatures = | ||
| ModuleUtils::readFeaturesSection(module, wasmFeatures); | ||
|
|
||
| void calculateFeatures(Module& module) { |
There was a problem hiding this comment.
how about "applyFeatures"? I think that makes it clear it applies them to the module.
| std::vector<std::string> debugInfoFileNames; | ||
|
|
||
| // `features` are the features allowed to be used in this module and should be | ||
| // respected regardless of the value of `hasFeaturesSection`. |
There was a problem hiding this comment.
maybe also add that hasFeaturesSection means that we read one, and will write one (unless told not to)
| if (debug) std::cerr << "getInt8: " << (int)(uint8_t)input[pos] << " (at " << pos << ")" << std::endl; | ||
| return input[pos++]; | ||
| if (debug) std::cerr << "getInt8: " << (int)(uint8_t)input.at(pos) << " (at " << pos << ")" << std::endl; | ||
| return input.at(pos++); |
There was a problem hiding this comment.
were these changes intended? i don't think we need them.
|
.map files changed because I changed FeatureSet::iterFeatures to match the alphabetical order of feature names (using the better feature names used by LLVM, which we should eventually switch to in Binaryen as well). |
|
Sounds good, lgtm. |
|
@kripken I just want to make sure you don't have an issue with the last two commits before merging. Apparently one of the spec tests expects an exported mutable global to fail validation, so I had to disable mutable globals for the spect tests and that meant adding feature options to wasm-shell. I'm surprised the spec test does not expect mutable globals to be enabled. |
|
The spec test is probably just quite old. How about simply removing the silly spec test? |
|
(Eventually it would be nice if the spec test files specified which features they use, or something like that, maybe...) |
|
Somehow this broke at least the other.test_zeroinit test. Seems that zeros are no longer being compressed in memory segments? Does that make any sense? |
| } | ||
|
|
||
| // Conservatively refuse to change segments if there might be memory.init | ||
| // and data.drop instructions. |
|
Yes, --detect-features is not properly defaulting to MVP in the absence of a features section. I'm working on a fix now. |
3 participants
This allowing us to emit a (potentially modified) target features
section and conditionally emit other sections such as the DataCount
section based on the presence of features.