Skip to content

Apply features from the commandline first #3960

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jul 2, 2021
Merged

Apply features from the commandline first #3960

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
93bbc28
https://github.com/WebAssembly/binaryen/pull/3955#issuecomment-871016647
kripken Jul 1, 2021
7096fe2
fix
kripken Jul 1, 2021
5dd5d3e
format
kripken Jul 1, 2021
66fba1c
update tests
kripken Jul 1, 2021
7776de2
comment
kripken Jul 1, 2021
29d1d2c
fix lint
kripken Jul 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/binaryen-c.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3974,7 +3974,8 @@ BinaryenModuleRef BinaryenModuleRead(char* input, size_t inputSize) {
buffer.resize(inputSize);
std::copy_n(input, inputSize, buffer.begin());
try {
WasmBinaryBuilder parser(*wasm, buffer);
// TODO: allow providing features in the C API
WasmBinaryBuilder parser(*wasm, FeatureSet::MVP, buffer);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WasmBinaryBuilder parser(*wasm, FeatureSet::MVP, buffer);
WasmBinaryBuilder parser(*wasm, FeatureSet::All, buffer);

This is closer to the previous behavior. I would be surprised if it weren't also required by our tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow you. Wouldn't this only be needed if we used to enable all the features in this code path? I don't see that. This should have no effect, that is, it sets the features to MVP, which is the default of a module anyhow.

But maybe I'm missing something... the tests on CI all failed due to lint, which is fixed now, so we can see what happens. Locally though they seem to pass for me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, never mind about the tests. I was thinking that the feature set here would control what feature would be able to be parsed, but that's not right. Why does this code have to change at all? FeatureSet::MVP is already the default feature set on a Module.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line changes only because I changed the API. I made the wasm binary builder class take a FeatureSet as a parameter. That feels nicer than the user setting the features on the module before in a separate operation. I could make this new parameter to the constructor default to MVP, and then this line could not change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this capability isn't used anywhere except to set the features to MVP, which they already are, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RoundTrip for example does apply the known correct features:

-    WasmBinaryBuilder parser(*module, input);
+    WasmBinaryBuilder parser(*module, features, input);

(And in wasm-io we set the features as well, although there we set them from the module, because that is all it has - a larger refactoring might also add an API there to get the features.)

parser.read();
} catch (ParseException& p) {
p.dump(std::cerr);
Expand Down
7 changes: 3 additions & 4 deletions src/passes/RoundTrip.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,14 @@ struct RoundTrip : public Pass {
void run(PassRunner* runner, Module* module) override {
BufferWithRandomAccess buffer;
// Save features, which would not otherwise make it through a round trip if
// the target features section has been stripped.
// the target features section has been stripped. We also need them in order
// to tell the builder which features to build with.
auto features = module->features;
// Write, clear, and read the module
WasmBinaryWriter(module, buffer).write();
ModuleUtils::clearModule(*module);
auto input = buffer.getAsChars();
WasmBinaryBuilder parser(*module, input);
WasmBinaryBuilder parser(*module, features, input);
parser.setDWARF(runner->options.debugInfo);
try {
parser.read();
Expand All @@ -48,8 +49,6 @@ struct RoundTrip : public Pass {
std::cerr << '\n';
Fatal() << "error in parsing wasm binary";
}
// Reapply features
module->features = features;
}
};

Expand Down
30 changes: 4 additions & 26 deletions src/tools/tool-options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ struct ToolOptions : public Options {
"Disable all non-MVP features",
Arguments::Zero,
[this](Options*, const std::string&) {
hasFeatureOptions = true;
enabledFeatures.setMVP();
disabledFeatures.setAll();
})
Expand All @@ -50,20 +49,14 @@ struct ToolOptions : public Options {
"Enable all features",
Arguments::Zero,
[this](Options*, const std::string&) {
hasFeatureOptions = true;
enabledFeatures.setAll();
disabledFeatures.setMVP();
})
.add("--detect-features",
"",
"Use features from the target features section, or MVP (default)",
"(deprecated - this flag does nothing)",
Arguments::Zero,
[this](Options*, const std::string&) {
hasFeatureOptions = true;
detectFeatures = true;
enabledFeatures.setMVP();
disabledFeatures.setMVP();
})
[](Options*, const std::string&) {})
.add("--quiet",
"-q",
"Emit less verbose output and hide trivial warnings.",
Expand Down Expand Up @@ -134,7 +127,6 @@ struct ToolOptions : public Options {
std::string("Enable ") + description,
Arguments::Zero,
[=](Options*, const std::string&) {
hasFeatureOptions = true;
enabledFeatures.set(feature, true);
disabledFeatures.set(feature, false);
})
Expand All @@ -144,32 +136,18 @@ struct ToolOptions : public Options {
std::string("Disable ") + description,
Arguments::Zero,
[=](Options*, const std::string&) {
hasFeatureOptions = true;
enabledFeatures.set(feature, false);
disabledFeatures.set(feature, true);
});
return *this;
}

void applyFeatures(Module& module) const {
if (hasFeatureOptions) {
if (!detectFeatures && module.hasFeaturesSection) {
FeatureSet optionsFeatures = FeatureSet::MVP;
optionsFeatures.enable(enabledFeatures);
optionsFeatures.disable(disabledFeatures);
if (!(module.features <= optionsFeatures)) {
Fatal() << "features section is not a subset of specified features. "
<< "Use --detect-features to resolve.";
}
}
module.features.enable(enabledFeatures);
module.features.disable(disabledFeatures);
}
module.features.enable(enabledFeatures);
module.features.disable(disabledFeatures);
}

private:
bool hasFeatureOptions = false;
bool detectFeatures = false;
FeatureSet enabledFeatures = FeatureSet::MVP;
FeatureSet disabledFeatures = FeatureSet::MVP;
};
Expand Down
3 changes: 1 addition & 2 deletions src/tools/wasm-as.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ int main(int argc, const char* argv[]) {
auto input(read_file<std::string>(options.extra["infile"], Flags::Text));

Module wasm;
options.applyFeatures(wasm);

try {
if (options.debug) {
Expand All @@ -115,8 +116,6 @@ int main(int argc, const char* argv[]) {
Fatal() << "error in parsing input";
}

options.applyFeatures(wasm);

if (options.extra["validate"] != "none") {
if (options.debug) {
std::cerr << "Validating..." << std::endl;
Expand Down
3 changes: 1 addition & 2 deletions src/tools/wasm-ctor-eval.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,7 @@ int main(int argc, const char* argv[]) {
auto input(read_file<std::string>(options.extra["infile"], Flags::Text));

Module wasm;
options.applyFeatures(wasm);

{
if (options.debug) {
Expand All @@ -563,8 +564,6 @@ int main(int argc, const char* argv[]) {
}
}

options.applyFeatures(wasm);

if (!WasmValidator().validate(wasm)) {
std::cout << wasm << '\n';
Fatal() << "error in validating input";
Expand Down
3 changes: 1 addition & 2 deletions src/tools/wasm-dis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ int main(int argc, const char* argv[]) {
std::cerr << "parsing binary..." << std::endl;
}
Module wasm;
options.applyFeatures(wasm);
try {
ModuleReader().readBinary(options.extra["infile"], wasm, sourceMapFilename);
} catch (ParseException& p) {
Expand All @@ -72,8 +73,6 @@ int main(int argc, const char* argv[]) {
Fatal() << "error in parsing wasm source mapping";
}

options.applyFeatures(wasm);

// TODO: Validation. However, validating would mean that users are forced to
// run with wasm-dis -all or such, to enable the features (unless the
// features section is present, but that's rare in general). It would be
Expand Down
3 changes: 1 addition & 2 deletions src/tools/wasm-emscripten-finalize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ int main(int argc, const char* argv[]) {
auto writeOutput = outfile.size() > 0 || !emitBinary;

Module wasm;
options.applyFeatures(wasm);
ModuleReader reader;
// If we are not writing output then we definitely don't need to read debug
// info, as it does not affect the metadata we will emit. (However, if we
Expand Down Expand Up @@ -239,8 +240,6 @@ int main(int argc, const char* argv[]) {
Fatal() << "error in parsing wasm source map";
}

options.applyFeatures(wasm);

BYN_TRACE_WITH_TYPE("emscripten-dump", "Module before:\n");
BYN_DEBUG_WITH_TYPE("emscripten-dump", std::cerr << &wasm);

Expand Down
3 changes: 1 addition & 2 deletions src/tools/wasm-metadce.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,7 @@ int main(int argc, const char* argv[]) {
auto input(read_file<std::string>(options.extra["infile"], Flags::Text));

Module wasm;
options.applyFeatures(wasm);

{
if (options.debug) {
Expand All @@ -519,8 +520,6 @@ int main(int argc, const char* argv[]) {
}
}

options.applyFeatures(wasm);

if (options.passOptions.validate) {
if (!WasmValidator().validate(wasm)) {
std::cout << wasm << '\n';
Expand Down
4 changes: 1 addition & 3 deletions src/tools/wasm-opt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ int main(int argc, const char* argv[]) {
options.parse(argc, argv);

Module wasm;
options.applyFeatures(wasm);

BYN_TRACE("reading...\n");

Expand Down Expand Up @@ -259,16 +260,13 @@ int main(int argc, const char* argv[]) {
"request for silly amounts of memory)";
}

options.applyFeatures(wasm);

if (options.passOptions.validate) {
if (!WasmValidator().validate(wasm)) {
exitOnInvalidWasm("error validating input");
}
}
}
if (translateToFuzz) {
options.applyFeatures(wasm);
TranslateToFuzzReader reader(wasm, options.extra["infile"]);
if (fuzzPasses) {
reader.pickPasses(options);
Expand Down
2 changes: 1 addition & 1 deletion src/tools/wasm-split.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,7 @@ void WasmSplitOptions::parse(int argc, const char* argv[]) {
}

void parseInput(Module& wasm, const WasmSplitOptions& options) {
options.applyFeatures(wasm);
ModuleReader reader;
reader.setProfile(options.profile);
try {
Expand All @@ -408,7 +409,6 @@ void parseInput(Module& wasm, const WasmSplitOptions& options) {
Fatal() << "error building module, std::bad_alloc (possibly invalid "
"request for silly amounts of memory)";
}
options.applyFeatures(wasm);

if (options.passOptions.validate && !WasmValidator().validate(wasm)) {
Fatal() << "error validating input";
Expand Down
6 changes: 3 additions & 3 deletions src/wasm-binary.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1338,9 +1338,9 @@ class WasmBinaryBuilder {
std::vector<HeapType> types;

public:
WasmBinaryBuilder(Module& wasm, const std::vector<char>& input)
: wasm(wasm), allocator(wasm.allocator), input(input), sourceMap(nullptr),
nextDebugLocation(0, {0, 0, 0}), debugLocation() {}
WasmBinaryBuilder(Module& wasm,
FeatureSet features,
const std::vector<char>& input);

void setDebugInfo(bool value) { debugInfo = value; }
void setDWARF(bool value) { DWARF = value; }
Expand Down
98 changes: 58 additions & 40 deletions src/wasm/wasm-binary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1319,6 +1319,14 @@ void WasmBinaryWriter::writeField(const Field& field) {

// reader

WasmBinaryBuilder::WasmBinaryBuilder(Module& wasm,
FeatureSet features,
const std::vector<char>& input)
: wasm(wasm), allocator(wasm.allocator), input(input), sourceMap(nullptr),
nextDebugLocation(0, {0, 0, 0}), debugLocation() {
wasm.features = features;
}

bool WasmBinaryBuilder::hasDWARFSections() {
assert(pos == 0);
getInt32(); // magic
Expand Down Expand Up @@ -3186,58 +3194,68 @@ void WasmBinaryBuilder::readNames(size_t payloadLen) {

void WasmBinaryBuilder::readFeatures(size_t payloadLen) {
wasm.hasFeaturesSection = true;
wasm.features = FeatureSet::MVP;

auto sectionPos = pos;
size_t numFeatures = getU32LEB();
for (size_t i = 0; i < numFeatures; ++i) {
uint8_t prefix = getInt8();
if (prefix != BinaryConsts::FeatureUsed) {
if (prefix == BinaryConsts::FeatureRequired) {
std::cerr
<< "warning: required features in feature section are ignored";
} else if (prefix == BinaryConsts::FeatureDisallowed) {
std::cerr
<< "warning: disallowed features in feature section are ignored";
} else {
throwError("Unrecognized feature policy prefix");
}

bool disallowed = prefix == BinaryConsts::FeatureDisallowed;
bool required = prefix == BinaryConsts::FeatureRequired;
bool used = prefix == BinaryConsts::FeatureUsed;

if (!disallowed && !required && !used) {
throwError("Unrecognized feature policy prefix");
}
if (required) {
std::cerr << "warning: required features in feature section are ignored";
}

Name name = getInlineString();
if (pos > sectionPos + payloadLen) {
throwError("ill-formed string extends beyond section");
}

if (prefix != BinaryConsts::FeatureDisallowed) {
if (name == BinaryConsts::UserSections::AtomicsFeature) {
wasm.features.setAtomics();
} else if (name == BinaryConsts::UserSections::BulkMemoryFeature) {
wasm.features.setBulkMemory();
} else if (name == BinaryConsts::UserSections::ExceptionHandlingFeature) {
wasm.features.setExceptionHandling();
} else if (name == BinaryConsts::UserSections::MutableGlobalsFeature) {
wasm.features.setMutableGlobals();
} else if (name == BinaryConsts::UserSections::TruncSatFeature) {
wasm.features.setTruncSat();
} else if (name == BinaryConsts::UserSections::SignExtFeature) {
wasm.features.setSignExt();
} else if (name == BinaryConsts::UserSections::SIMD128Feature) {
wasm.features.setSIMD();
} else if (name == BinaryConsts::UserSections::TailCallFeature) {
wasm.features.setTailCall();
} else if (name == BinaryConsts::UserSections::ReferenceTypesFeature) {
wasm.features.setReferenceTypes();
} else if (name == BinaryConsts::UserSections::MultivalueFeature) {
wasm.features.setMultivalue();
} else if (name == BinaryConsts::UserSections::GCFeature) {
wasm.features.setGC();
} else if (name == BinaryConsts::UserSections::Memory64Feature) {
wasm.features.setMemory64();
} else if (name ==
BinaryConsts::UserSections::TypedFunctionReferencesFeature) {
wasm.features.setTypedFunctionReferences();
}
FeatureSet feature;
if (name == BinaryConsts::UserSections::AtomicsFeature) {
feature = FeatureSet::Atomics;
} else if (name == BinaryConsts::UserSections::BulkMemoryFeature) {
feature = FeatureSet::BulkMemory;
} else if (name == BinaryConsts::UserSections::ExceptionHandlingFeature) {
feature = FeatureSet::ExceptionHandling;
} else if (name == BinaryConsts::UserSections::MutableGlobalsFeature) {
feature = FeatureSet::MutableGlobals;
} else if (name == BinaryConsts::UserSections::TruncSatFeature) {
feature = FeatureSet::TruncSat;
} else if (name == BinaryConsts::UserSections::SignExtFeature) {
feature = FeatureSet::SignExt;
} else if (name == BinaryConsts::UserSections::SIMD128Feature) {
feature = FeatureSet::SIMD;
} else if (name == BinaryConsts::UserSections::TailCallFeature) {
feature = FeatureSet::TailCall;
} else if (name == BinaryConsts::UserSections::ReferenceTypesFeature) {
feature = FeatureSet::ReferenceTypes;
} else if (name == BinaryConsts::UserSections::MultivalueFeature) {
feature = FeatureSet::Multivalue;
} else if (name == BinaryConsts::UserSections::GCFeature) {
feature = FeatureSet::GC;
} else if (name == BinaryConsts::UserSections::Memory64Feature) {
feature = FeatureSet::Memory64;
} else if (name ==
BinaryConsts::UserSections::TypedFunctionReferencesFeature) {
feature = FeatureSet::TypedFunctionReferences;
} else {
// Silently ignore unknown features (this may be and old binaryen running
// on a new wasm).
}

if (disallowed && wasm.features.has(feature)) {
std::cerr
<< "warning: feature " << feature.toString()
<< " was enabled by the user, but disallowed in the features section.";
}
if (required || used) {
wasm.features.enable(feature);
}
}
if (pos != sectionPos + payloadLen) {
Expand Down
4 changes: 3 additions & 1 deletion src/wasm/wasm-io.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@ void ModuleReader::readBinaryData(std::vector<char>& input,
Module& wasm,
std::string sourceMapFilename) {
std::unique_ptr<std::ifstream> sourceMapStream;
WasmBinaryBuilder parser(wasm, input);
// Assume that the wasm has had its initial features applied, and use those
// while parsing.
WasmBinaryBuilder parser(wasm, wasm.features, input);
parser.setDebugInfo(debugInfo);
parser.setDWARF(DWARF);
parser.setSkipFunctionBodies(skipFunctionBodies);
Expand Down
3 changes: 2 additions & 1 deletion src/wasm/wasm-s-parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,8 @@ SExpressionWasmBuilder::SExpressionWasmBuilder(Module& wasm,
stringToBinary(str, size, data);
}
}
WasmBinaryBuilder binaryBuilder(wasm, data);
// TODO: support applying features here
WasmBinaryBuilder binaryBuilder(wasm, FeatureSet::MVP, data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WasmBinaryBuilder binaryBuilder(wasm, FeatureSet::MVP, data);
WasmBinaryBuilder binaryBuilder(wasm, FeatureSet::All, data);

We should leave disallowed features to be caught by validation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand you here either, sorry... How would enabling all features get us more validation in the text parser? (the text parser has no feature section parsing, but even if it did..?)

(tests passed)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that we would want to be able to parse all features by default, but the feature set passed here won't actually control what features can be parsed, so I was confused. Disregard this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, why does this need to change to apply a new feature set? Can't it just keep the default one that wasm already had?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same issue as above, I think)

binaryBuilder.read();
return;
}
Expand Down
Loading