OdinMonkey: add stack-walking support for asm.js

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Assignee

Description

5 years ago
Bug 980059 changed FrameIter to be able to walk over script-less (asm.js) frames, but it only exposes the entry frame of an asm.js activation.  This bug is for adding all the others by recording enough callsite metadata to walk the stack (without needing to push anything extra during normal calls).  This fixes the long-standing absence of asm.js frames from Error.stack.
Assignee

Updated

5 years ago
Depends on: 998507
Assignee

Comment 1

5 years ago
This patch simplifies the stack-overflow check by moving it before the stack decrement. This means that the stack depth at the overflow exit stub is fixed which is good for stack-walking that happens from inside js_ReportOverRecursed.
Attachment #8409152 - Flags: review?(sunfish)
Assignee

Comment 2

5 years ago
Tangentially related aesthetic improvement: instead of activations.activation()->whatever(), let's overload operator-> so we can write activations->whatever().
Attachment #8409154 - Flags: review?(jdemooij)
Assignee

Comment 3

5 years ago
There is some code duplication stemming from the lack of an AssemblerShared class that the Assembler* can share.  This patch adds an AssemblerShared and hoists some existing vectors into it.  Later patches will add a new Vector for CallSites.  Asking Jan for r? on just the high-level change of adding AssemblerShared and Benjamin for r? on all the asm.js changes.
Attachment #8409157 - Flags: review?(jdemooij)
Attachment #8409157 - Flags: review?(benj)
Assignee

Comment 4

5 years ago
Unrelated, but I don't think anyone is using this anymore, so it's nice to cut off the dead weight.
Attachment #8409158 - Flags: review?(benj)
Comment on attachment 8409152 [details] [diff] [review]
simplify-stack-overflow

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

Yay! This patch feels like an overall simplification too, which is nice.

::: js/src/jit/AsmJS.cpp
@@ +6736,5 @@
>      MacroAssembler &masm = m.masm();
>      masm.align(CodeAlignment);
>      masm.bind(&m.stackOverflowLabel());
>  
> +    // The overflow check always occur before the initial function-specific

occurs

::: js/src/jit/arm/CodeGenerator-arm.cpp
@@ +66,4 @@
>      }
>  
> +    // Note that this automatically sets MacroAssembler::framePushed().
> +    masm.reserveStack(frameDepth_);

The non-asm.js version does checkStackAlignment() here. Does asm.js not keep the stack aligned on ARM? If it doesn't, it'd be good to have a comment here noting the difference with non-asm.js.

::: js/src/jit/shared/CodeGenerator-x86-shared.cpp
@@ +40,1 @@
>  {

The ARM version of this routine does JS_ASSERT(!gen->compilingAsmJS()); here. That'd be appropriate for x86 too.

@@ +45,5 @@
>  }
>  
>  bool
> +CodeGeneratorX86Shared::generateAsmJSPrologue(Label *stackOverflowLabel)
> +{

As above, the ARM version does JS_ASSERT(gen->compilingAsmJS()); here.

@@ +58,5 @@
> +    }
> +
> +    // Note that this automatically sets MacroAssembler::framePushed().
> +    masm.reserveStack(frameSize());
> +

I wouldn't otherwise remark about a blank line, but ARM has its own copy of this code without a blank line here, and it'd be nice to avoid uninteresting differences.
Attachment #8409152 - Flags: review?(sunfish) → review+
Assignee

Comment 6

5 years ago
Posted patch stack-walkSplinter Review
This patch adds a CallSite struct which describes the callsite (line, column, caller name) and also contains the stack depth metadata to walk to the next frame (stack depth at the call, return address from the call).

For the most part this was pretty straightforward as there aren't many ways to call into, call inside, and call out of asm.js.  Most of the challenge was just difference between x86 and ARM and the fact that, when calling C++, we can't trust the callee will immediately push lr.
Attachment #8409179 - Flags: review?(dtc-moz)
Comment on attachment 8409154 [details] [diff] [review]
act-iter-op-arrow

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

Pretty!
Attachment #8409154 - Flags: review?(jdemooij) → review+
Comment on attachment 8409157 [details] [diff] [review]
hoist-asmjs-vectors

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

Great that we can share those now (I just skimmed the patch, Benjamin should take a closer look).
Attachment #8409157 - Flags: review?(jdemooij) → review+
Comment on attachment 8409158 [details] [diff] [review]
rm-function-counts

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

Nice!
Attachment #8409158 - Flags: review?(benj) → review+
Comment on attachment 8409157 [details] [diff] [review]
hoist-asmjs-vectors

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

Very nice refactoring!

::: js/src/jit/MIRGraph.cpp
@@ -30,5 @@
>      performsCall_(false),
>      performsAsmJSCall_(false),
> -    asmJSHeapAccesses_(*alloc),
> -    asmJSGlobalAccesses_(*alloc),
> -    minAsmJSHeapLength_(AsmJSAllocationGranularity),

nit: shouldn't the last line stay? otherwise minAsmJSHeapLength_ would be uninitialized

::: js/src/jit/shared/Assembler-shared.h
@@ +813,5 @@
>  
> +// The base class of all Assemblers for all archs.
> +class AssemblerShared
> +{
> +    Vector<AsmJSHeapAccess, 0, SystemAllocPolicy> asmJSHeapAccesses_;

style nit: You could reuse the typedef name defined above here, but it'd break symmetry.

@@ +827,5 @@
> +    AsmJSGlobalAccess asmJSGlobalAccess(size_t i) const { return asmJSGlobalAccesses_[i]; }
> +
> +    bool append(AsmJSAbsoluteLink link) { return asmJSAbsoluteLinks_.append(link); }
> +    size_t numAsmJSAbsoluteLinks() const { return asmJSAbsoluteLinks_.length(); }
> +    AsmJSAbsoluteLink asmJSAbsoluteLink(size_t i) const { return asmJSAbsoluteLinks_[i]; }

I am wondering if this function and asmJSGlobalAccess shouldn't return const& instead of copies, as they carry more than a word of information (on intel's x86, a word is 16 bits long according to intel's architecture manual).
Attachment #8409157 - Flags: review?(benj) → review+
Assignee

Comment 11

5 years ago
(In reply to Dan Gohman [:sunfish] from comment #5)
> The non-asm.js version does checkStackAlignment() here. Does asm.js not keep
> the stack aligned on ARM?

Oops, you're right, I'll add checkStackAlignment back.

(In reply to Benjamin Bouvier [:bbouvier] from comment #10)
> nit: shouldn't the last line stay? otherwise minAsmJSHeapLength_ would be
> uninitialized

Good catch, thanks!

> style nit: You could reuse the typedef name defined above here, but it'd
> break symmetry.

Yeah, I was mostly going for symmetry :)

> > +    AsmJSAbsoluteLink asmJSAbsoluteLink(size_t i) const { return asmJSAbsoluteLinks_[i]; }
> 
> I am wondering if this function and asmJSGlobalAccess shouldn't return
> const& instead of copies, as they carry more than a word of information (on
> intel's x86, a word is 16 bits long according to intel's architecture
> manual).

I guess I could have nuanced my answer on IRC a bit more :)  In general, I prefer by-value, even when we are talking about something bigger than a word, for not-super-hot code since it is semantically simpler (e.g., it avoids the possibility of invalidation if someone holds onto the const& while the underlying container is mutated) and lexically shorter.  Furthermore, the compiler will surely inline and decompose this value into its constituent scalars so value vs. const& shouldn't actually affect the generated code.
Comment on attachment 8409179 [details] [diff] [review]
stack-walk

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

Nice work.

* Would like to have seen a high level overview of the requirements and design.  Even a short paragraph could help orientate a reader and answer questions such as:

  Is a stack backtrace ever required for asynchronously interrupted asm.js code?

  Is the backtrace only for debug purposes, and could it bail out and skip the asm.js frames?

  The Ion and Asm.js conventions do not use a dedicated frame pointer.

  The design records the call sites and the stack depth at only these points, and records the stack at exits?

* Is there a test for backtracing though the FFI OOLConvert path. If not then is it possible.

::: js/src/jit/AsmJS.cpp
@@ +6510,5 @@
>  
>      // Reserve space for a call to AsmJSImm_CoerceInPlace_* and an array of values used by
>      // OOLConvert which reuses the same frame. This code needs to be kept in sync with the
>      // stack usage in GenerateOOLConvert.
>      MIRType typeArray[] = { MIRType_Pointer, MIRType_Pointer }; // cx, argv

An extra word has been added to this frame for the return address and it should be accounted for when calculating the stack frame size.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +3574,5 @@
> +    // Note: this function stores the return address to sp[0]. The caller must
> +    // anticipate this by pushing additional space on the stack. The ABI does
> +    // not provide space for a return address so this function may only be
> +    // called if no argument are passed.
> +    JS_ASSERT(stackArgBytes == 0);

Good to see this check, and lucky that at most four arguments are currently needed.

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +601,5 @@
> +        appendCallSite(CallSiteDesc::Exit());
> +
> +        // The Ion ABI has the caller pop the return address off the stack.
> +        // The asm.js caller assumes that the call leaves sp unchanged, so bump
> +        // the stack.

Is it the callee, not the caller, that pops the stack in ARM Ion calls?

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +693,5 @@
>  
> +    void appendCallSite(const CallSiteDesc &desc) {
> +        // Add an extra sizeof(void*) to include the return address that was
> +        // pushed by the call instruction (see CallSite::stackDepth).
> +        enoughMemory_ &= append(CallSite(desc, currentOffset(), framePushed_ + sizeof(void*)));

The framePushed_ might not be the stack depth back to the function entry.

For example the FFI exit stubs.  The framePushed would be the stack depth back to the start of the exit stub, but are the calls to the exit stubs also compatible with the stack walking.
Attachment #8409179 - Flags: review?(dtc-moz) → review+
Assignee

Comment 13

5 years ago
(In reply to Douglas Crosher [:dougc] from comment #12)
> * Would like to have seen a high level overview of the requirements and
> design.

Ah, you're right.  I'll add a comment answering these questions in Assembler-shared.h on CallSiteDesc.

> * Is there a test for backtracing though the FFI OOLConvert path. If not
> then is it possible.

There is; it's the one where an object is returned with a valueOf function (which triggers the OOL convert).

> > +        // The Ion ABI has the caller pop the return address off the stack.
> > +        // The asm.js caller assumes that the call leaves sp unchanged, so bump
> > +        // the stack.
> 
> Is it the callee, not the caller, that pops the stack in ARM Ion calls?

Oops, yes.

> > +    void appendCallSite(const CallSiteDesc &desc) {
> > +        // Add an extra sizeof(void*) to include the return address that was
> > +        // pushed by the call instruction (see CallSite::stackDepth).
> > +        enoughMemory_ &= append(CallSite(desc, currentOffset(), framePushed_ + sizeof(void*)));
> 
> The framePushed_ might not be the stack depth back to the function entry.
> 
> For example the FFI exit stubs.  The framePushed would be the stack depth
> back to the start of the exit stub, but are the calls to the exit stubs also
> compatible with the stack walking.

I don't quite follow: exit stubs are either called by a call instruction (for FFIs) or are jumped to with a known stack-depth (throw stub).  In either case, the framePushed_ has the same meaning as it does for normal asm.js functions: it takes you back to the return-pc of the caller.  In the case of exits, the CallSite is marked as skipped so the exit stub's frame doesn't show up in the stack trace.
(In reply to Luke Wagner [:luke] from comment #13)
... 
> > For example the FFI exit stubs.  The framePushed would be the stack depth
> > back to the start of the exit stub, but are the calls to the exit stubs also
> > compatible with the stack walking.
> 
> I don't quite follow: exit stubs are either called by a call instruction
> (for FFIs) or are jumped to with a known stack-depth (throw stub).  In
> either case, the framePushed_ has the same meaning as it does for normal
> asm.js functions: it takes you back to the return-pc of the caller.  In the
> case of exits, the CallSite is marked as skipped so the exit stub's frame
> doesn't show up in the stack trace.

Thank you for the explanation, this was not clear.  Stepped through an example to see how it works.  Great work.
Target Milestone: mozilla31 → ---
Target Milestone: --- → mozilla31
Depends on: 1018290
You need to log in before you can comment on or make changes to this bug.