Closed
Bug 1001372
Opened 10 years ago
Closed 10 years ago
[jsdbg2] Clone optimized ICStubs when doing debug mode OSR
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
VERIFIED
FIXED
mozilla32
Tracking | Status | |
---|---|---|
firefox31 | --- | wontfix |
firefox-esr24 | --- | unaffected |
firefox-esr31 | - | wontfix |
People
(Reporter: decoder, Assigned: shu)
References
Details
(Keywords: crash, sec-low, testcase, Whiteboard: [fuzzblocker] [jsbugmon:update])
Crash Data
Attachments
(3 files, 1 obsolete file)
40.90 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
27.67 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
902 bytes,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 5ecd532a167e (run with --fuzzing-safe): var lfcode = new Array(); lfcode.push = loadFile; lfcode.push(""); lfcode.push(""); lfcode.push("3"); lfcode.push(""); lfcode.push(""); lfcode.push(""); lfcode.push(""); lfcode.push("4"); lfcode.push(""); lfcode.push("0"); lfcode.push("gczeal(2);"); lfcode.push("\ var g = newGlobal();\ g.parent = this;\ g.eval('new Debugger(parent).onExceptionUnwind = f;');\ "); function loadFile(lfVarx) { evaluate(lfVarx); }
Reporter | ||
Comment 1•10 years ago
|
||
Marking as a fuzzblocker, as this crashes on the heap with no reasonable signatures that could be used to distinguish it from other failures. Also it seems highly frequent.
status-firefox31:
--- → affected
Whiteboard: [jsbugmon:update,bisect][fuzzblocker]
Reporter | ||
Comment 3•10 years ago
|
||
Here's another test (requested by shu), that also crashes on the heap: var lfcode = new Array(); lfcode.push(""); lfcode.push("0"); lfcode.push("newGlobal(\"1\").eval(\"(new Debugger).addAllGlobalsAsDebuggees();\");\n"); while (true) { var file = lfcode.shift(); if (file == undefined) { break; } loadFile(file) } function loadFile(lfVarx) { try { if (lfVarx.substr(-3) != ".js" && lfVarx.length != 1) { evaluate(lfVarx); } else if (!isNaN(lfVarx)) {} } catch (lfVare) { } }
Reporter | ||
Updated•10 years ago
|
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update]
Reporter | ||
Comment 4•10 years ago
|
||
JSBugMon: Bisection requested, result: === Tinderbox Build Bisection Results by autoBisect === The "good" changeset has the timestamp "20140424014842" and the hash "25dd3d17a19a". The "bad" changeset has the timestamp "20140424020245" and the hash "079f4f0ed6a6". Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=25dd3d17a19a&tochange=079f4f0ed6a6
Assignee | ||
Comment 5•10 years ago
|
||
This is caused by me completely missing handling of fake exit frames, as I thought Baseline didn't generate them. The current debug mode OSR code depends on the exit frame always being a different ABI frame (i.e. pushed ip) from the stub frame. For fake exit frames generated by ICCall_Native, this is not true, so we are getting unpredictable crashes there. Closing bug since this is technically s-s, though a low rating because there is currently no frontend way to in the browser to attach a Debugger with scripts live on the stack -- and should be that way until the fuzzers calm down. A fix here is going to be tricky. For normal callVMs, the wrapper persists outside of a particular BaselineScript, so VM calls always have a place to return to. It is the exit frame that the wrapper pushes that debug mode OSR patches. For ICCall_Native's optimized stubs, the fake exit frame is a per-BaselineScript jitcode, so if we destroy the original BaselineScript at debug mode OSR time, there will be no code for the VM call to return to. I believe the cleanest way, at the cost of a little perf, is to have a real exit frame for ICCall_Native stubs. ICCall_Fallback would compile a little stub that expects a callee already loaded in a particular register (R1 for 32bit, ExtractTemp0 for 64bit), pushes a real exit frame, and does the callABI. When debug mode OSR happens, we then have persistent piece of jitcode to return to and a real exit frame to patch. This comes at the cost of an extra call, and an extra real ABI frame. My hope is that the perf hti from this will be negligible, since this is in Baseline.
Group: javascript-core-security
Flags: needinfo?(shu)
Assignee | ||
Updated•10 years ago
|
Summary: [jsdbg2] Crash on Heap near [@ js::jit::DoCallNativeSetterInfo] → [jsdbg2] Push an actual frame in ICCall_Native stubs for debug mode OSR
Assignee | ||
Comment 6•10 years ago
|
||
Push a real frame in ICCall_Native. This incidentally lets us save a little memory by reusing a generic native wrapper.
Attachment #8413161 -
Flags: review?(jdemooij)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → shu
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•10 years ago
|
||
Good news is this patch doesn't really affect perf. SunSpider, WITH patch: ============================================ RESULTS (means and 95% confidence intervals) -------------------------------------------- Total: 149.0ms +/- 4.7% -------------------------------------------- 3d: 25.2ms +/- 1.6% cube: 9.2ms +/- 2.7% morph: 4.1ms +/- 3.9% raytrace: 12.0ms +/- 3.5% access: 14.0ms +/- 39.3% binary-trees: 2.0ms +/- 2.1% fannkuch: 7.4ms +/- 73.5% nbody: 2.3ms +/- 1.5% nsieve: 2.3ms +/- 2.2% bitops: 7.2ms +/- 1.0% 3bit-bits-in-byte: 0.9ms +/- 2.2% bits-in-byte: 1.8ms +/- 3.7% bitwise-and: 1.6ms +/- 1.9% nsieve-bits: 2.9ms +/- 1.1% controlflow: 1.9ms +/- 1.7% recursive: 1.9ms +/- 1.7% crypto: 12.5ms +/- 2.8% aes: 6.2ms +/- 4.7% md5: 3.5ms +/- 1.8% sha1: 2.9ms +/- 0.9% date: 18.9ms +/- 1.3% format-tofte: 9.7ms +/- 1.7% format-xparb: 9.2ms +/- 1.8% math: 9.8ms +/- 1.4% cordic: 2.1ms +/- 1.8% partial-sums: 6.2ms +/- 1.5% spectral-norm: 1.5ms +/- 1.9% regexp: 10.7ms +/- 1.5% dna: 10.7ms +/- 1.5% string: 48.8ms +/- 1.7% base64: 4.7ms +/- 0.9% fasta: 5.8ms +/- 1.6% tagcloud: 14.4ms +/- 1.9% unpack-code: 16.5ms +/- 3.3% validate-input: 7.4ms +/- 0.9% SunSpider, WITHOUT patch: ============================================ RESULTS (means and 95% confidence intervals) -------------------------------------------- Total: 149.7ms +/- 4.1% -------------------------------------------- 3d: 25.6ms +/- 2.3% cube: 9.4ms +/- 3.4% morph: 4.0ms +/- 1.9% raytrace: 12.2ms +/- 4.3% access: 14.1ms +/- 36.8% binary-trees: 2.0ms +/- 1.7% fannkuch: 7.5ms +/- 69.1% nbody: 2.3ms +/- 1.5% nsieve: 2.3ms +/- 2.0% bitops: 7.2ms +/- 1.3% 3bit-bits-in-byte: 0.9ms +/- 2.2% bits-in-byte: 1.8ms +/- 3.7% bitwise-and: 1.7ms +/- 2.6% nsieve-bits: 2.9ms +/- 1.0% controlflow: 1.9ms +/- 2.0% recursive: 1.9ms +/- 2.0% crypto: 12.6ms +/- 2.2% aes: 6.2ms +/- 4.4% md5: 3.5ms +/- 1.9% sha1: 2.9ms +/- 2.0% date: 19.3ms +/- 1.1% format-tofte: 9.8ms +/- 1.5% format-xparb: 9.5ms +/- 1.4% math: 9.7ms +/- 1.3% cordic: 2.1ms +/- 3.1% partial-sums: 6.1ms +/- 1.3% spectral-norm: 1.4ms +/- 2.8% regexp: 10.7ms +/- 1.6% dna: 10.7ms +/- 1.6% string: 48.7ms +/- 1.8% base64: 4.8ms +/- 1.7% fasta: 5.8ms +/- 1.0% tagcloud: 14.5ms +/- 2.7% unpack-code: 16.4ms +/- 3.9% validate-input: 7.3ms +/- 1.1% Octane, WITH patch: Richards: 21866 DeltaBlue: 30467 Crypto: 26340 RayTrace: 52021 EarleyBoyer: 32509 RegExp: 2855 Splay: 15687 SplayLatency: 10531 NavierStokes: 31398 PdfJS: 15762 Mandreel: 30692 MandreelLatency: 38348 Gameboy: 48635 CodeLoad: 19178 Box2D: 32532 zlib: 81501 Typescript: 21180 ---- Score (version 9): 24549 Octane, WITHOUT patch: Richards: 21887 DeltaBlue: 29575 Crypto: 26542 RayTrace: 51281 EarleyBoyer: 32580 RegExp: 2793 Splay: 16323 SplayLatency: 11431 NavierStokes: 31335 PdfJS: 15851 Mandreel: 30177 MandreelLatency: 37483 Gameboy: 45719 CodeLoad: 19183 Box2D: 32877 zlib: 81112 Typescript: 20746 ---- Score (version 9): 24482
Assignee | ||
Comment 8•10 years ago
|
||
I'll also add assertions that, while patching return addresses for debug mode OSR, each frame's return address is different.
Comment 9•10 years ago
|
||
I don't really understand the patch. AFAICS, the problem is this: (1) There's a Call_Native stub for the call to evaluate(..) (2) Debug mode OSR patches the stub pointer in the stub frame to the Call_Fallback stub. (3) The callWithABI returns to the Call_Native stub, where it crashes in the code emitted for EmitEnterTypeMonitorIC(masm), because the stub pointer is not a Call_Native stub but a Call_Fallback stub. What happens with this patch instead of (3)? Will we return to the returnFromStubOffset_ code in Call_Fallback? The code there looks wrong for native calls. Could the debug OSR code attach a Call_Native stub in this case, based on the old one? Then (3) wouldn't crash and it's zero overhead in the no-debug-mode case. (In reply to Shu-yu Guo [:shu] from comment #6) > Push a real frame in ICCall_Native. This incidentally lets us save a little > memory by reusing a generic native wrapper. There's only a single copy of the Call_Native stub code so this doesn't matter.
Updated•10 years ago
|
Attachment #8413161 -
Flags: review?(jdemooij)
Assignee | ||
Comment 10•10 years ago
|
||
This patch reimplements how stub frames are patched. Instead of going to some special fixer like the Ion bailout fixer, this patch clones optimized stubs on the stack and uses the old stub's JitCode, thus keeping it alive. Fallback stubs don't need to be cloned, since they always exist. This should be cleaner than the old approach and easier to reason about, since we will always return to the same stub code.
Attachment #8414085 -
Flags: review?(jdemooij)
Assignee | ||
Comment 11•10 years ago
|
||
Part 1 obviates the need for a special return-from-stub fixer originally added to patch stub frames, so I reverted the change.
Attachment #8414086 -
Flags: review?(jdemooij)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8414087 -
Flags: review?(jdemooij)
Assignee | ||
Updated•10 years ago
|
Attachment #8413161 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Summary: [jsdbg2] Push an actual frame in ICCall_Native stubs for debug mode OSR → [jsdbg2] Clone optimized ICStubs when doing debug mode OSR
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8414085 [details] [diff] [review] Part 1: Clone on-stack optimized stubs when doing debug mode OSR Review of attachment 8414085 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/BaselineJIT.cpp @@ +638,5 @@ > > void > +BaselineScript::adoptFallbackStubsForDebugModeOSR(BaselineScript *other) > +{ > +} Oops, forgot to remove this. ::: js/src/jit/BaselineJIT.h @@ +304,5 @@ > } > > void copyICEntries(JSScript *script, const ICEntry *entries, MacroAssembler &masm); > void adoptFallbackStubs(FallbackICStubSpace *stubSpace); > + void adoptFallbackStubsForDebugModeOSR(BaselineScript *other); And this. Ignore these please.
Comment 14•10 years ago
|
||
Comment on attachment 8414085 [details] [diff] [review] Part 1: Clone on-stack optimized stubs when doing debug mode OSR Review of attachment 8414085 [details] [diff] [review]: ----------------------------------------------------------------- Great, r=me with nits addressed and changes to BaselineJIT.* reverted. ::: js/src/jit/BaselineDebugModeOSR.cpp @@ +3,5 @@ > * This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +#include <malloc.h> Is this include necessary? @@ +581,5 @@ > for (size_t i = 0; i < entries.length(); i++) { > JSScript *script = entries[i].script; > + > + if (!RecompileBaselineScriptForDebugMode(cx, script) || > + !CloneOldBaselineStub(cx, entries[i])) What happens if a single stub is on the stack multiple times, will we clone it multiple times? It's harmless but would be better to avoid it. ::: js/src/jit/BaselineIC.h @@ +3166,5 @@ > } > + > + static ICGetElem_NativePrototypeCallNative *Clone( > + JSContext *cx, ICStubSpace *space, ICStub *firstMonitorStub, > + ICGetElem_NativePrototypeCallNative &other) The indentation here looks unusual. Could you also move the definition to the cpp file? There's too much code in the header file (same for MIR.h), I've been trying to add more to the cpp file. This also doesn't need inlining etc. Same for the methods below.
Attachment #8414085 -
Flags: review?(jdemooij) → review+
Comment 15•10 years ago
|
||
Comment on attachment 8414086 [details] [diff] [review] Part 2: Revert bailout return offset changes in Baseline can-call fallbacks Review of attachment 8414086 [details] [diff] [review]: ----------------------------------------------------------------- \o/
Attachment #8414086 -
Flags: review?(jdemooij) → review+
Updated•10 years ago
|
Attachment #8414087 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 16•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e3480e87428d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/caad9ce31941 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c3c7a4b24fbe
Comment 17•10 years ago
|
||
landed on m-c https://hg.mozilla.org/mozilla-central/rev/e3480e87428d https://hg.mozilla.org/mozilla-central/rev/caad9ce31941 https://hg.mozilla.org/mozilla-central/rev/c3c7a4b24fbe
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Reporter | ||
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 18•10 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•10 years ago
|
status-firefox-esr24:
--- → unaffected
Updated•10 years ago
|
Group: javascript-core-security → core-security
Updated•10 years ago
|
status-firefox-esr31:
--- → affected
tracking-firefox-esr31:
--- → -
Updated•9 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•