Closed
Bug 1338002
Opened 8 years ago
Closed 8 years ago
Baldr: validation rule updates
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: luke, Assigned: luke)
References
Details
Attachments
(8 files)
3.95 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
32.79 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
153.94 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
39.34 KB,
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
211.52 KB,
patch
|
decoder
:
feedback+
|
Details | Diff | Splinter Review |
37.05 KB,
patch
|
luke
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
465 bytes,
application/octet-stream
|
Details | |
170.40 KB,
patch
|
luke
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
![]() |
Assignee | |
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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+
![]() |
Assignee | |
Updated•8 years ago
|
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)
Comment 4•8 years ago
|
||
bugherder |
![]() |
Assignee | |
Comment 5•8 years ago
|
||
This patch just backs out bug 1324032.
Attachment #8836838 -
Flags: review?(sunfish)
![]() |
Assignee | |
Comment 6•8 years ago
|
||
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)
![]() |
Assignee | |
Comment 7•8 years ago
|
||
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)
![]() |
Assignee | |
Comment 8•8 years ago
|
||
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•8 years ago
|
||
(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?
![]() |
Assignee | |
Comment 10•8 years ago
|
||
applies to fbe76e704b6b
Updated•8 years ago
|
Attachment #8836838 -
Flags: review?(sunfish) → review+
![]() |
Assignee | |
Comment 11•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8836848 -
Flags: review?(sunfish) → review+
Updated•8 years ago
|
Attachment #8836846 -
Flags: review?(sunfish) → review+
Comment 12•8 years ago
|
||
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d82c91046a3f
Baldr: backout bug 1324032 (r=sunfish)
![]() |
Assignee | |
Comment 13•8 years ago
|
||
(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.
![]() |
Assignee | |
Comment 14•8 years ago
|
||
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?
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8837368 -
Attachment description: branch.patch → beta-branch.patch
Comment 15•8 years ago
|
||
backout bugherder |
Comment 16•8 years ago
|
||
Comment on attachment 8837368 [details] [diff] [review]
beta-branch.patch
wasm update for beta52
Attachment #8837368 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 17•8 years ago
|
||
uplift |
Comment 19•8 years ago
|
||
status-firefox-esr52:
--- → fixed
Comment 20•8 years ago
|
||
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•8 years ago
|
||
Comment 22•8 years ago
|
||
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+
![]() |
Assignee | |
Comment 23•8 years ago
|
||
Thanks! That actually did find an interesting bug with in topWithType() (it must update an Any type with the Unify()ed type).
Comment 24•8 years ago
|
||
Marking as blocking bug 1332691 since one core test (unreached-invalid.wast) wants this.
Blocks: 1332691
Comment 25•8 years ago
|
||
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)
![]() |
Assignee | |
Updated•8 years ago
|
Whiteboard: [leave open]
![]() |
Assignee | |
Comment 26•8 years ago
|
||
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?
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/955c45972c5a
https://hg.mozilla.org/mozilla-central/rev/700a14ec1ffc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 28•8 years ago
|
||
Comment on attachment 8838743 [details] [diff] [review]
aurora-branch.patch
Wasm update. Aurora53+.
Attachment #8838743 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 29•8 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•