1338002 - Baldr: validation rule updates

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Luke Wagner [:luke]]

Attachment: binary-version

Before making the unreachability-validation changes, this patch will help people transition to 0x1 by accepting both for a short time.

Comment 2

[Dan Gohman [: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".

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe76e704b6b
Baldr: temporarily accept both 0xd and 0x1 (r=sunfish)

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fbe76e704b6b

Comment 5

[Luke Wagner [:luke]]

Attachment: backout-br-end

This patch just backs out bug 1324032.

Comment 6

[Luke Wagner [:luke]]

Attachment: polymorphic-type-checking

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)

Comment 7

[Luke Wagner [:luke]]

Attachment: import-tests

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).

Comment 8

[Luke Wagner [:luke]]

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 :) ?

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

(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?

Comment 10

[Luke Wagner [:luke]]

Attachment: combined patch

applies to fbe76e704b6b

Comment 11

[Luke Wagner [:luke]]

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?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d82c91046a3f
Baldr: backout bug 1324032 (r=sunfish)

Comment 13

[Luke Wagner [:luke]]

(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.

Comment 14

[Luke Wagner [:luke]]

Attachment: beta-branch.patch

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d82c91046a3f

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8837368 [details] [diff] [review]
beta-branch.patch

wasm update for beta52

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a1191022c63

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/9a1191022c63

Comment 20

[Christian Holler (:decoder)]

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 21

[Christian Holler (:decoder)]

Attachment: Testcase for comment 20

Comment 22

[Christian Holler (:decoder)]

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.

Comment 23

[Luke Wagner [:luke]]

Thanks!  That actually did find an interesting bug with in topWithType() (it must update an Any type with the Unify()ed type).

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Marking as blocking bug 1332691 since one core test (unreached-invalid.wast) wants this.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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)

Comment 26

[Luke Wagner [:luke]]

Attachment: aurora-branch.patch

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

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/955c45972c5a
https://hg.mozilla.org/mozilla-central/rev/700a14ec1ffc

Comment 28

[Gerry Chang [:gchang]]

Comment on attachment 8838743 [details] [diff] [review]
aurora-branch.patch

Wasm update. Aurora53+.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc472627593465885a69a4c1c7e2fd8ad3787c48