Closed Bug 1338002 Opened 6 years ago Closed 6 years ago

Baldr: validation rule updates

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(8 files)

      No description provided.
Attached patch binary-versionSplinter Review
Before making the unreachability-validation changes, this patch will help people transition to 0x1 by accepting both for a short time.
Attachment #8835213 - Flags: review?(sunfish)
Comment on attachment 8835213 [details] [diff] [review]
binary-version

Review of attachment 8835213 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/lib/wasm-binary.js
@@ +4,5 @@
>  const magic2 = 0x73;  // 's'
>  const magic3 = 0x6d;  // 'm'
>  
> +// EncodingVersion
> +const experimentalVersion = 0x1;

We can also rename this from "experimentalVersion" to "encodingVersion".
Attachment #8835213 - Flags: review?(sunfish) → review+
Whiteboard: [leave open]
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe76e704b6b
Baldr: temporarily accept both 0xd and 0x1 (r=sunfish)
Attached patch backout-br-endSplinter Review
This patch just backs out bug 1324032.
Attachment #8836838 - Flags: review?(sunfish)
This patch switches to polymorphic type checking.  As part of this patch:
 - a new enum class StackType is introduced which is ValType + Any; StackType is almost exclusively used by the impl of OpIter keeping the interface mostly in terms of ValType/ExprType
 - the remaining bits of validation that were done by WasmValidate.cpp (br_table, call) are moved inside OpIter; to support this, OpIter is given a ModuleEnviornment.  As a nice bonus, this should allow a simple follow-up patch to validate while compiling instead of having two passes as we currently do.
 - OpIter::reachable_ is removed and instead the only state is ControlStackEntry::polymorphicBase which is exclusively read by the pop*/top* functions.
 - infalliblePush had to become fallible because now a push preceded by a pop doesn't imply that the valueStack_ has memory (the popped value could've come from the polymorphicBase case)
Attachment #8836846 - Flags: review?(sunfish)
Attached patch import-testsSplinter Review
This patch updates to the latest test suite which we now completely pass (w/ the exception of a few unrelated nan tests that are new and a too-big memory allocation).
Attachment #8836848 - Flags: review?(sunfish)
Lars/Benjamin: feel free to have a look and make any comments.  The plan is to uplift this patch to aurora/beta.  Benjamin: perhaps you could feed this patch to your fuzzer :) ?
(In reply to Luke Wagner [:luke] from comment #8)
> Lars/Benjamin: feel free to have a look and make any comments.  The plan is
> to uplift this patch to aurora/beta.  Benjamin: perhaps you could feed this
> patch to your fuzzer :) ?

Sure, can you make a rolled up patch and let me know on which revision it does apply, please?
Attached patch combined patchSplinter Review
applies to fbe76e704b6b
Attachment #8836838 - Flags: review?(sunfish) → review+
Comment on attachment 8836863 [details] [diff] [review]
combined patch

It looks like Benjamin's wasm fuzzer isn't able to hit these interesting corner cases yet; do you suppose you could feed this patch (based on fbe76e704b6b) to AFL?
Attachment #8836863 - Flags: feedback?(choller)
Attachment #8836848 - Flags: review?(sunfish) → review+
Attachment #8836846 - Flags: review?(sunfish) → review+
(Waiting for Binaryen/fuzzing before landing the other patches.)

In the meantime, I'm thinking it's least risky to *not* land the polymorphic-checking patch on beta because beta validation code is significantly different than aurora/trunk and thus would require non-trivial (and thus risky) rebasing.  Instead, on beta I'd just like to land the version-change and backout patches.  Thus, FF52 would accept a slight super-set of v.1 and FF53 would be the first with the polymorphic checking restrictions.
Approval Request Comment
[Feature/Bug causing the regression]: spec change
[User impact if declined]: wrong version accepted
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: the two patches folded into this branch patch (1) tweak a version number, (2) backout a patch spec-change patch that was itself recently landed on beta
Attachment #8837368 - Flags: review+
Attachment #8837368 - Flags: approval-mozilla-beta?
Attachment #8837368 - Attachment description: branch.patch → beta-branch.patch
Comment on attachment 8837368 [details] [diff] [review]
beta-branch.patch

wasm update for beta52
Attachment #8837368 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This is an automated crash issue comment:

Summary: Assertion failure: k == Stk::RegisterI32 || k == Stk::ConstI32 || k == Stk::MemI32 || k == Stk::LocalI32, at js/src/wasm/WasmBaselineCompile.cpp:1770
Build version: mozilla-inbound revision fbe76e704b6b+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug
Runtime options: 

Testcase:

See attachment.

Backtrace:

==25063==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000003d1380e bp 0x7ffdf26a4050 sp 0x7ffdf26a4030 T0)
    #0 0x3d1380d in js::wasm::BaseCompiler::popJoinRegUnlessVoid(js::wasm::ExprType) js/src/wasm/WasmBaselineCompile.cpp:1775:13
    #1 0x3cd89e6 in js::wasm::BaseCompiler::emitBr() js/src/wasm/WasmBaselineCompile.cpp:5434:16
    #2 0x3cf7d5b in js::wasm::BaseCompiler::emitBody() js/src/wasm/WasmBaselineCompile.cpp:6731:13
    #3 0x3d00066 in js::wasm::BaseCompiler::emitFunction() js/src/wasm/WasmBaselineCompile.cpp:7306:10
    #4 0x3d05381 in js::wasm::BaselineCompileFunction(js::wasm::CompileTask*, js::wasm::FuncCompileUnit*, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmBaselineCompile.cpp:7567:10
    #5 0xc94fc5 in js::wasm::CompileFunction(js::wasm::CompileTask*, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmGenerator.cpp:1214:18
    #6 0xc94780 in js::wasm::ModuleGenerator::launchBatchCompile() js/src/wasm/WasmGenerator.cpp:946:14
    #7 0xc95e9e in js::wasm::ModuleGenerator::finishFuncDefs() js/src/wasm/WasmGenerator.cpp:993:26
    #8 0xc52499 in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/wasm/WasmCompile.cpp:90:12
    #9 0xc52499 in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char [], JS::FreePolicy>*) js/src/wasm/WasmCompile.cpp:126
    #10 0xdd3135 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/wasm/WasmJS.cpp:399:27
    #11 0x5eeebb in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5851:14

I can't really tell if this is caused by the patch because we only have one wasm build on our build server right now. But I haven't seen this one before. If it's unrelated to the patch here, please let me know and I will file a separate bug.
Comment on attachment 8836863 [details] [diff] [review]
combined patch

Apart from the issue I reported in the last comment, I haven't seen any failures for this patch. There are some problems with OOM handling that I will investigate on Monday, but I suspect they are unrelated to this bug.
Attachment #8836863 - Flags: feedback?(choller) → feedback+
Thanks!  That actually did find an interesting bug with in topWithType() (it must update an Any type with the Unify()ed type).
Marking as blocking bug 1332691 since one core test (unreached-invalid.wast) wants this.
Blocks: 1332691
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/955c45972c5a
Baldr: validate unreachable code with polymorphic type checking (r=sunfish)
https://hg.mozilla.org/integration/mozilla-inbound/rev/700a14ec1ffc
Baldr: update spec tests (r=sunfish)
Whiteboard: [leave open]
Approval Request Comment
[Feature/Bug causing the regression]: spec change
[User impact if declined]: incorrect behavior according to spec
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: lots of good testing: AFL tends to be super-effective, lots of unit tests, and we have million-line workloads
Attachment #8838743 - Flags: review+
Attachment #8838743 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/955c45972c5a
https://hg.mozilla.org/mozilla-central/rev/700a14ec1ffc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8838743 [details] [diff] [review]
aurora-branch.patch

Wasm update. Aurora53+.
Attachment #8838743 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.