Closed Bug 1130481 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

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 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?
Attachment #8560577 - Flags: review?(benj) → review+
https://hg.mozilla.org/mozilla-central/rev/e7e6e82492cb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.