Change default feature set to MVP by tlively · Pull Request #1993 · WebAssembly/binaryen

tlively · 2019-04-09T22:44:13Z

This greatly simplifies the feature handling logic and does not
change the user experience when a target features section is present.
Also refactors features to be stored on the Module after they are read
out of the target features section, allowing us to emit a (potentially
modified) target features section and conditionally emit sections
based on features, such as the DataCount section.

This greatly simplifies the feature handling logic and does not change the user experience when a target features section is present. Also refactors features to be stored on the Module after they are read out of the target features section, allowing us to emit a (potentially modified) target features section and conditionally emit sections based on features, such as the DataCount section.

tlively · 2019-04-09T22:46:09Z

@@ -1 +1 @@
-{"version":3,"sources":["tests/hello_world.c","tests/other_file.cpp","return.cpp","even-opted.cpp","fib.c","/tmp/emscripten_test_binaryen2_28hnAe/src.c","(unknown)"],"names":[],"mappings":"yLAIA,IACA,ICyylTA,aC7vlTA,OAkDA,0BCnGA,OACA,OACA,uBCAA,4BAKA,QAJA,OADA,8CAKA,0ICsi1DA,MCrvyDA"}


TBH I have no idea what this change means.

I think it means that things moved around - the location of code is not the same as before. That's surprising, I think?

Aha, this is because I changed the element section to only be emitted if it has any elements. That moved the code section up in the file.

tlively · 2019-04-09T22:49:44Z

    run_command(WASM_REDUCE + ['a.wasm', '--command=%s b.wasm --fuzz-exec' % WASM_OPT[0], '-t', 'b.wasm', '-w', 'c.wasm'])
    after = os.stat('c.wasm').st_size
-    assert after < 0.6 * before, [before, after]
+    assert after < 0.7 * before, [before, after]


I haven't really examined why this changed yet.

We should definitely look into that before landing, yeah, since this PR should be NFC wrt this test?

kripken

Nice, mostly lgtm, but some issues left for discussion first.

kripken · 2019-04-09T22:53:11Z

    run_command(WASM_REDUCE + ['a.wasm', '--command=%s b.wasm --fuzz-exec' % WASM_OPT[0], '-t', 'b.wasm', '-w', 'c.wasm'])
    after = os.stat('c.wasm').st_size
-    assert after < 0.6 * before, [before, after]
+    assert after < 0.7 * before, [before, after]


We should definitely look into that before landing, yeah, since this PR should be NFC wrt this test?

kripken · 2019-04-09T22:56:59Z

+};
+
+Pass *createStripTargetFeaturesPass() {
+  return new FeaturesStrip();


why is the pass StripTargetFeatures and the instance FeaturesStrip? I think the convention we have is to keep them the same (i.e. both are the former).

kripken · 2019-04-09T23:01:22Z

  std::vector<std::string> debugInfoFileNames;

+  FeatureSet features = FeatureSet::MVP;
+  bool emitFeaturesSection = false;


Perhaps we should handle the features section the same way we handle the names section - we read it if it's there, but we don't emit it unless the user specifically asks us to?

Hmm, it would make sense to unify what we do with the features section and the names section (and the producers section? I forget what we do there). OTOH, it is very useful to preserve the features section through long chains of wasm-opt calls such as when running the reducer. Otherwise all but the first call of wasm-opt would have a validation failure.

I believe we decided offline to leave this how it is for now but to leave this open for unification with how we treat other sections in the future.

(and rename to hasFeaturesSection)

Right, thank you.

kripken · 2019-04-09T23:05:40Z

 }

+void WasmBinaryWriter::writeFeaturesSection() {
+  if (!wasm->emitFeaturesSection || wasm->features <= FeatureSet{}) {


does the right side of the if mean "if MVP, don't emit the features section"?

Yes, I forgot about the isMVP() method which I should definitely use here.

kripken · 2019-04-09T23:17:52Z

@@ -1 +1 @@
-{"version":3,"sources":["tests/hello_world.c","tests/other_file.cpp","return.cpp","even-opted.cpp","fib.c","/tmp/emscripten_test_binaryen2_28hnAe/src.c","(unknown)"],"names":[],"mappings":"yLAIA,IACA,ICyylTA,aC7vlTA,OAkDA,0BCnGA,OACA,OACA,uBCAA,4BAKA,QAJA,OADA,8CAKA,0ICsi1DA,MCrvyDA"}


I think it means that things moved around - the location of code is not the same as before. That's surprising, I think?

The change in debug location is due to the newly-elided element section.

kripken · 2019-04-11T17:33:52Z

Having to sprinkle -all everywhere (to tell it "all features are ok") worries me that it will be annoying in the day to day. How do other tools do this, like wabt? cc @binji

tlively · 2019-04-11T17:41:05Z

I believe WABT simply enables all features all the time (AFAIK it doesn't have any feature options). This makes sense for tools that read but do not modify modules and is what we do for wasm-as and wasm-dis. Tools that can modify modules need to know what features they are allowed to use, and it seems best to be conservative about that.

That being said, we probably do have more -all sprinkled around than would be optimal. In check.py there is one instance where we pass -all to wasm-shell. wasm-shell should be changed to assume -all. But all other instances of -all are passed to wasm-opt, which I think makes sense.

Also: - Error when target features do not exactly match feature options - Add --print-features pass and use it in testing

tlively · 2019-04-16T22:34:50Z

@kripken PTAL. This PR move towards implementing the plan described in #2004. I also expect this to fix current waterfall breakage, though I have not finished testing locally.

MVP is the default feature set.
When there is a target features section and no command line feature flags, the features section is used
When there is a target features section and there are command line feature flags, error if they do not exactly match, unless --detect-features was one of the feature flags.
--detect-features sets the current feature set equal to the contents of the target features section

There is not yet a way to crawl a binary and detect what features it uses.

kripken

lgtm with those comments fixed

kripken · 2019-04-16T22:42:56Z

    Module wasm;
    ModuleReader reader;
    reader.read(working, *module);
+    wasm.features = FeatureSet::All;


I'm not sure this is optimal - it might lead the optimizer to attempt odd things that hurt reduction. Please add a comment that we should likely autodetect features from the binary here instead, once we can do that.

kripken · 2019-04-16T22:43:14Z

  {
    // read and write it
-    auto cmd = Path::getBinaryenBinaryTool("wasm-opt") + " " + input + " -o " + test;
+    auto cmd = Path::getBinaryenBinaryTool("wasm-opt") + " " + input + " -all -o " + test;


kripken · 2019-04-16T22:44:23Z

-  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++);


why is this change here? seems offtopic for the PR; also we already have a check right above it that throws an error.

Right, sorry. My reverting of this change in #2001 was not backported to this branch. Fixing.

sbc100 · 2019-04-16T22:49:05Z

 - Add `segmentPassive` argument to `BinaryenSetMemory` for marking segments
  passive.
 - Make `-o -` print to stdout instead of a file named "-".
+- Change default feature set in the absence of a target features section from


This should go up above in Current Trunk

sbc100 · 2019-04-16T22:53:59Z

    testcase.assertEqual(str(p.stdout), str(f.read()))
+
+
+def disassemble(self, filename):


Surely these should be left as members? They take self as arg0 after all.

Yes, but I want to share them across multiple unittest.TestCase subclasses. I suppose the proper way to do that would be to make all my test cases inherit from a new class that contains these methods.

binji · 2019-04-17T07:13:18Z

Having to sprinkle -all everywhere (to tell it "all features are ok") worries me that it will be annoying in the day to day. How do other tools do this, like wabt? cc @binji

I believe WABT simply enables all features all the time (AFAIK it doesn't have any feature options).

Sorry, missed this. Wabt does have feature flags for all tools. But wasm-objdump enables all features by default.

This reverts commit cb2d635. The issues with feature validation were mostly resolved in WebAssembly#1993, and this PR finishes the job by adding feature flags to wasm-as to avoid emitting the DataCount section when bulk-memory is not enabled.

This reverts commit cb2d635. The issues with feature validation were mostly resolved in #1993, and this PR finishes the job by adding feature flags to wasm-as to avoid emitting the DataCount section when bulk-memory is not enabled.

tlively requested a review from kripken April 9, 2019 22:44

tlively commented Apr 9, 2019

View reviewed changes

kripken reviewed Apr 9, 2019

View reviewed changes

tlively added 8 commits April 10, 2019 17:05

Address aesthetic comments

6472678

Restore fishy test changes

4438353

Restore updated debug info tests.

6b4e499

The change in debug location is due to the newly-elided element section.

Attempt to fix wasm-reduce

c35f3e5

Fix wasm-reduce

62f3a92

Merge branch 'master' into features-refactor

4f46cb1

Fix test

b6f0469

Merge branch 'master' into features-refactor

55c294a

Remove feature opts from wasm-shell

ae86883

tlively mentioned this pull request Apr 11, 2019

Move features from passOptions to Module #2001

Merged

tlively added 3 commits April 16, 2019 14:52

Merge branch 'master' into features-refactor

0442539

Also: - Error when target features do not exactly match feature options - Add --print-features pass and use it in testing

Flake8 fixes

7b1d0b1

Remove --mvp-features from emscripten metadata

67812f5

kripken approved these changes Apr 16, 2019

View reviewed changes

sbc100 reviewed Apr 16, 2019

View reviewed changes

Address comments

5079322

sbc100 reviewed Apr 16, 2019

View reviewed changes

Move util functions to BinaryenTestCase class

4f65b10

tlively merged commit 4d81752 into WebAssembly:master Apr 16, 2019

tlively mentioned this pull request Apr 18, 2019

Re-land emitting of DataCount section #2027

Merged

tlively deleted the features-refactor branch April 24, 2020 23:15

		@@ -1 +1 @@
		{"version":3,"sources":["tests/hello_world.c","tests/other_file.cpp","return.cpp","even-opted.cpp","fib.c","/tmp/emscripten_test_binaryen2_28hnAe/src.c","(unknown)"],"names":[],"mappings":"yLAIA,IACA,ICyylTA,aC7vlTA,OAkDA,0BCnGA,OACA,OACA,uBCAA,4BAKA,QAJA,OADA,8CAKA,0ICsi1DA,MCrvyDA"} No newline at end of file

		testcase.assertEqual(str(p.stdout), str(f.read()))


		def disassemble(self, filename):

Conversation

tlively commented Apr 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Apr 11, 2019

Uh oh!

tlively commented Apr 11, 2019

Uh oh!

tlively commented Apr 16, 2019

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

binji commented Apr 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants