Closed
Bug 1102498
Opened 10 years ago
Closed 9 years ago
SavedStacks sees extra frame on stack when resuming generator
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: sfink, Assigned: jandem)
References
Details
Attachments
(1 file)
1.11 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
I have a jit-test assertion that has been bedeviling me when trying to land a statistics patch. I finally managed to reproduce it without my patch: 1. Build a debug JS shell with --enable-gccompacting (or run |devtools/automation/autospider.sh generational| to build it.) 2. cd js/src/jit-test 3. for n in $(seq 100); do JS_GC_ZEAL=14,$n ./jit_test.py ~/src/MI-GC/obj-spider/dist/bin/js saved-stacks/generators.js; done The point of the loop is that the bug will only show up when you trigger a compacting pass at just the wrong time. If you add this code to generators.js: var f = frame; do { print(f.functionDisplayName); f = f.parent; } while (f); you'll see that in a successful run you will get iife2 generator next iife1 null and in a failed run you will see iife2 generator InterpretGeneratorResume next iife1 null So it is seeing an extra self-hosted InterpretGeneratorResume entry on the stack.
Assignee | ||
Comment 1•10 years ago
|
||
(In reply to Steve Fink [:sfink, :s:] from comment #0) > So it is seeing an extra self-hosted InterpretGeneratorResume entry on the > stack. I don't think this is a bug. If we throw out the generator's JIT code (and CGC purges JIT code I think), there can be an extra InterpretGeneratorResume frame on the stack. The test should probably allow both cases.
Assignee | ||
Comment 2•10 years ago
|
||
Also: the debugger (by default) ignores self-hosted frames. This is the saved-stacks thing, it does not hide self-hosted scripts...
Comment 3•10 years ago
|
||
According to fitzgen, saved stacks save self-hosted frames depending on the principal on purpose. So some possibilities: - Change the test to accept both cases like Jan suggested. - Add a saveNonBuiltinStack testing function. - Expose a .builtin getter on the saved frame JS object. - Change the saved stack default?
Updated•10 years ago
|
Summary: Debugger seees extra frame on stack when resuming generator → SavedStacks sees extra frame on stack when resuming generator
Assignee | ||
Comment 4•10 years ago
|
||
I changed the test to use frame.toString(). It hides self-hosted frames so this avoids the bug and this way we don't rely on an implementation detail in the self-hosted code.
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #4) > Created attachment 8526759 [details] [diff] [review] > Patch > > I changed the test to use frame.toString(). It hides self-hosted frames so > this avoids the bug and this way we don't rely on an implementation detail > in the self-hosted code. That's probably an adequate fix for this. But as a separate issue, it still leaves in the possibility of differential fuzz testing seeing a mismatch between the interpreter and the JIT. (I was only getting the test failure when running --ion-eager.) With this test gone, that will *probably* never happen for this particular case, but it seems we should at least file a bug to resolve the general problem. (Even if it means whitelisting SavedStack somehow in the fuzzers.) Are there any other known cases where jit/non-jit can validly produce different output? For that reason, I think a better fix might be to have a way of explicitly marking certain functions to be *always* skipped (well, unless perhaps you set a fuzzing-unsafe flag or something.)
Comment 6•10 years ago
|
||
Comment on attachment 8526759 [details] [diff] [review] Patch Review of attachment 8526759 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/saved-stacks/generators.js @@ +7,5 @@ > }()); > }()).next(); > }()); > > +// toString does not include self-hosted frames. Maybe link back to this bug, so future readers understand *why* we don't want to include self-hosted frames here.
Attachment #8526759 -
Flags: review?(nfitzgerald) → review+
Reporter | ||
Comment 7•10 years ago
|
||
Filed bug 1103155 for the differential fuzzbug problem. I will land this for now, with bug numbers added as requested, since it's trigger from a different push I made earlier.
Reporter | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/24ab00570a33
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/24ab00570a33
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•