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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: sfink, Assigned: jandem)

References

Details

Attachments

(1 file)

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.
Blocks: 1088831
(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.
Also: the debugger (by default) ignores self-hosted frames. This is the saved-stacks thing, it does not hide self-hosted scripts...
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?
Summary: Debugger seees extra frame on stack when resuming generator → SavedStacks sees extra frame on stack when resuming generator
Attached patch PatchSplinter Review
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8526759 - Flags: review?(nfitzgerald)
(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 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+
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.
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.