1030945 - Support asm.js frames in SavedStacks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Right now SavedStacks::insertFrames uses ScriptFrameIter, which means that it skips asm.js frames.

If we used a normal FrameIter, we would see them, but we would also have to start using FrameIter::computeLine instead of PCToLineNumber directly.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Some notes from IRC:

We don't want to use FrameIter::computeLine if we have a script, because that will call PCToLineNumber and miss out memoization bits in pcLocationMap.

So in the case when we have a script, we want to keep doing what we're doing.  In the case when we have no script, we'd need to grab the line/column/source directly from the iterator before we increment it.  Getting the line/column via computeLine is fast in this case.  Getting the script filename is too, but then we'd have to atomize it...

If we care about that part, we could push down the pcLocationMap bits into FrameIter and have it memoize stuff based on the info it has (so script+pc in the non-asm case, and scriptSource+line+column in the asm.js case).  And then we could just use FrameIter::scriptFilename and FrameIter::computeLine directly.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase whose behavior would change today if we used SavedStacks for exception stacks

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: asm-frames.patch

Try push: https://tbpl.mozilla.org/?tree=Try&rev=43f88daef390

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8446822 [details] [diff] [review]
asm-frames.patch

Review of attachment 8446822 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  Let me take one more look at the patch with these addressed:

::: js/src/jit-test/tests/saved-stacks/asm-frames.js
@@ +26,5 @@
> +print(stack);
> +assertEq(stack.functionDisplayName, "tester");
> +
> +assertEq(stack.parent.line, 6);
> +assertEq(stack.parent.column, 4);

With the fix mentioned in the other comment, you should be able to add functionDisplayName assertions here and below.

::: js/src/vm/SavedStacks.cpp
@@ +472,5 @@
>      // here, rather than inside saveCurrentStack, because in some cases we will
>      // pass the check there, despite later failing the check here (for example,
>      // in js/src/jit-test/tests/saved-stacks/bug-1006876-too-much-recursion.js).
>      JS_CHECK_RECURSION_DONT_REPORT(cx, return false);
> +    JSPrincipals* principals = iter.compartment() ? iter.compartment()->principals : nullptr;

Maybe I'm missing something, but I think iter.compartment() is always no-null, so you could just have iter.compartment()->principals here.

@@ +478,5 @@
> +    LocationValue location;
> +
> +    if (iter.hasScript()) {
> +        RootedScript script(cx, iter.script());
> +        callee.set(iter.maybeCallee());

Instead of doing this, I think you can just use:
  iter.isNonEvalFunctionFrame() ? iter.functionDisplayName() : nullptr,
below when constructing AutoLookupRooter.  That way asm.js functions will have names.

@@ +479,5 @@
> +
> +    if (iter.hasScript()) {
> +        RootedScript script(cx, iter.script());
> +        callee.set(iter.maybeCallee());
> +        jsbytecode* pc = iter.pc();

jsbytecode *pc

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: asm-frames.patch

Updated based on review comments.

New try push: https://tbpl.mozilla.org/?tree=Try&rev=25efadefdd72

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, this is safe because getLocation() actually copies out of the hashtable instead of returning a reference into it, so it's OK if the recursive insertFrames call has to rehash the hashtable?

Might be worth adding a comment to that effect, because I can totally see someone like me trying to optimize away that copy that by returning references into the hashtable.  ;)

Now that you're not holding on to the JSScript* across the iterator increment, I don't think you need the RootedScript.  It wasn't there before bug 1030938.

As far as filenames, as a followup, I wonder how much people would hate it if we just atomized the filename on JSScript creation and stored the JSAtom* in addition to the const char*...

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8446865 [details] [diff] [review]
asm-frames.patch

Review of attachment 8446865 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!

::: js/src/vm/SavedStacks.cpp
@@ +476,5 @@
> +    JSPrincipals* principals = iter.compartment()->principals;
> +    RootedAtom name(cx, iter.isNonEvalFunctionFrame() ? iter.functionDisplayAtom() : nullptr);
> +    LocationValue location;
> +
> +    if (iter.hasScript()) {

Super nit: I think it'd read a bit better to have the \n before the 'location' decl, not after.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: asm-frames.patch

With requested update.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1437c48ae58d

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I botched resolving a conflict when I pushed the patch, so here's a followup to fix that: https://hg.mozilla.org/integration/mozilla-inbound/rev/d2e790e8bc8e

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And I had to back these both out for introducing a new hazard: https://tbpl.mozilla.org/php/getParsedLog.php?id=42665020&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/67cdb3cbea9f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3f4fe642a893

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Need bug 1031168 to fix the hazards, its the same issue.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: asm-frames.patch

Rebased now that bug 1031168 landed. Shouldn't have the hazard anymore.

Try: https://tbpl.mozilla.org/?tree=Try&rev=281321298314

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I think the one build failure is unrelated, I've been seeing it in a bunch of other try pushes as well.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f342e07b19

Comment 16

[Carsten Book [:Tomcat]]

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