Closed Bug 980059 Opened 7 years ago Closed 7 years ago

Expose script-less frames via the stack frame iterator


(Core :: JavaScript Engine, defect)

Not set





(Reporter: luke, Assigned: luke)




(5 files, 1 obsolete file)

asm.js functions do not have JSScripts which is a problem when trying to expose asm.js functions via ScriptFrameIter.  This bug removes the restriction since most of the stack iterations that actually want to see asm.js (ComputeStackString et al) only need the (filename, lineno, principal).  To do this, ScriptFrameIter is renamed to FrameIter (which can view frames where !hasScript()) and ScriptFrameIter is re-added as a derived class that auto-skips frames with !hasScript()).  Having audited all the uses of ScriptFrameIter, most either aren't looking for asm.js or will never see it so they can keep using ScriptFrameIter without change.
Attached patch tidyingSplinter Review
This patch just does some trivial renaming and rearrangement in preparation for the next patches.
Attachment #8386366 - Flags: review?(jdemooij)
This patch makes AsmJSActivation be a real Activation.  ScriptFrameIter just automatically skips over AsmJSActivations for now.
Attachment #8386369 - Flags: review?(jdemooij)
Attached patch add-frame-iterSplinter Review
This patch just splits out FrameIter from ScriptFrameIter, but it still doesn't actually look at asm.js frames.
Attachment #8386370 - Flags: review?(jdemooij)
Attached patch add-asmjs-frame (obsolete) — Splinter Review
As a holdover until actual asm.js stack-walking is done, this patch just makes the first asm.js function called in an AsmJSActivation show up in the stack.  This is enough to test all the places where asm.js can show up and give the fuzzers a target.  In another bug I'll make a real AsmJSFrameIterator that behaves and is used like to IonFrameIterator.
Attachment #8386376 - Flags: review?(jdemooij)
Comment on attachment 8386366 [details] [diff] [review]

Review of attachment 8386366 [details] [diff] [review]:

::: js/src/vm/Stack.h
@@ +961,5 @@
>  }
>  /*****************************************************************************/
> +class InterpreterRegs

Nice, I also want to do s/StackFrame/InterpreterFrame soon.
Attachment #8386366 - Flags: review?(jdemooij) → review+
Attachment #8386369 - Flags: review?(jdemooij) → review+
Attached patch add-asmjs-frameSplinter Review
Fix thinkos in DescribeScriptedCallerForCompilation caught by debugger tests.
Attachment #8386376 - Attachment is obsolete: true
Attachment #8386376 - Flags: review?(jdemooij)
Attachment #8386814 - Flags: review?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #5)
> Nice, I also want to do s/StackFrame/InterpreterFrame soon.

That'll be good.
Comment on attachment 8386370 [details] [diff] [review]

Review of attachment 8386370 [details] [diff] [review]:

::: js/src/vm/Stack.h
@@ +1464,5 @@
> +//  - When provided, the optional JSPrincipal argument will cause FrameIter to
> +//    only show frames in globals whose JSPrincipals are subsumed (via
> +//    JSSecurityCallbacks::subsume) by the given JSPrincipal.
> +//
> +// Additionally, there are automatically skip certain types of frames:

Nit: fix this sentence (nice comment btw).
Attachment #8386370 - Flags: review?(jdemooij) → review+
Comment on attachment 8386814 [details] [diff] [review]

Review of attachment 8386814 [details] [diff] [review]:

Nice, these changes turned out to be simpler than I expected.

::: js/src/vm/OldDebugAPI.cpp
@@ +901,5 @@
>  js_CallContextDebugHandler(JSContext *cx)
>  {
> +    FrameIter iter(cx);
> +    for (; !iter.done(); ++iter) {
> +        // If there is no script to debug, then abort execution from he slow

Nit: s/he/the

::: js/src/vm/Stack.h
@@ +1431,5 @@
>      void *errorRejoinSP_;
>      SPSProfiler *profiler_;
>      void *resumePC_;
> +    // This bits are temporary and will be replaced with real stack-walking

Nit: these bits, or something else.
Attachment #8386814 - Flags: review?(jdemooij) → review+
Thanks for the fast review!  I just realized that the fix I needed in DescribeScriptedCallerForCompilation was needed in *all* the places where I changed a NonBuiltinScriptFrameIter to a FrameIter.  This makes the need clear for adding a NonBuiltinFrameIter which does the obvious thing.  So one more simple patch.
Attachment #8386917 - Flags: review?(jdemooij)
Comment on attachment 8386917 [details] [diff] [review]

Review of attachment 8386917 [details] [diff] [review]:

::: js/src/vm/OldDebugAPI.cpp
@@ +907,5 @@
>          if (!iter.hasScript())
>              return false;
>          // Only stop once we reach a non-self-hosted script.
>          if (!iter.script()->selfHosted())

This condition is always true now.
Attachment #8386917 - Flags: review?(jdemooij) → review+
Blocks: 998490
You need to log in before you can comment on or make changes to this bug.