1130481 - IonMonkey: Assert that no Ion frame is miss-aligned.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

While investigating the remaining alignment issues, I noticed that it was quite useful to add an assertion at the entry point of the Ion frame.  Doing so simplifies the debugging as the caller just pushed its return address.

Note: Unfortunately, it is hard to identify the caller without doing a similar trick as [1], but this is something I intend to address later.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips#Finding_the_script_of_Ion_generated_assembly_%28from_gdb%29

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey: Assert that Ion frames are properly aligned.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8560577 [details] [diff] [review]
IonMonkey: Assert that Ion frames are properly aligned.

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

Asserts, asserts everywhere.

::: js/src/jit/CodeGenerator.cpp
@@ +1996,5 @@
>      // to 0, before reserving the stack.
>      MOZ_ASSERT(masm.framePushed() == frameSize());
>      masm.setFramePushed(0);
> +
> +    // Ensure that the Ion frames is properly aligned.

nit: either s/is/are, or s/frames/frame :) (comment applies to this file and platform specific codegen functions)

@@ +1997,5 @@
>      MOZ_ASSERT(masm.framePushed() == frameSize());
>      masm.setFramePushed(0);
> +
> +    // Ensure that the Ion frames is properly aligned.
> +    masm.assertStackAlignment(JitStackAlignment, 0);

nit: offset is 0 by default (comment applies to this file and platform specific codegen functions)

::: js/src/jit/JitFrames.cpp
@@ +2761,5 @@
>                  // everything can properly be aligned before adding complexity.
>                  MOZ_RELEASE_ASSERT(frames.ionScript()->frameSize() % JitStackAlignment == 0,
>                    "Ensure that if the Ion frame is aligned, then the spill base is also aligned");
>  
> +                if (isScriptedCallee) {

I found the definition of isScriptedCallee in bug 1112160.  However, it seems that isScriptedCallee, unless I missed a patch, is set for the next loop turn.  So, in the next loop turn, this doesn't apply to the callee anymore, but to the caller.  How about renaming it to wasScriptedCallee, or isScriptedCaller?

Comment 3

[Nicolas B. Pierron [:nbp]]

(Try) https://treeherder.mozilla.org/#/jobs?repo=try&revision=220ed7bf0305
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7e6e82492cb

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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