849398 - BaselineCompiler: "Assertion failure: has(reg)," with any function

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

dis()

asserts js debug shell on ionmonkey changeset 73009fa09525 without any CLI arguments at Assertion failure: has(reg),

This is a fuzzblocker for Win64 fuzzing for the baseline compiler due to its simplicity, and this might be Win64-only.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Actually this does not necessarily have to be dis(), it asserts just the same with print(), help() or even (function(){})().

Comment 2

[Kannan Vijayan [:djvj]]

What revision is this happening with?

Comment 3

[Kannan Vijayan [:djvj]]

Learns me to read things :)  Scratch that question.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Due to skipped revisions, the first bad revision could be any of:
changeset:   123133:be125cabea26
user:        Jan de Mooij
date:        Tue Feb 26 10:43:57 2013 +0100
summary:     Bug 843596 - Run scripts in the interpreter before baseline-compili
ng them. r=djvj

changeset:   123134:c856bc366e53
user:        Jan de Mooij
date:        Tue Feb 26 13:35:39 2013 +0100
summary:     Fix bug 843596 fallout. r=bhackett on IRC

I'm guessing the regressor is bug 843596.

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: Patch

Tentative untested (but probably correct) fix.  I can't build/test this since I don't have immediate access to an x64 box.  I've asked gkw to run it through when he has time.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 723552 [details] [diff] [review]
Patch

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

Thanks!

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +148,5 @@
>          regs.take(reg_code);
>  
> +        // Ensure that |scratch| does not end up being JSReturnOperand.
> +        // takeUnchecked because on Win64/x64, reg_code (IntArgReg0) and JSReturnOperand are
> +        // the same (rcx).  See https://bugzilla.mozilla.org/show_bug.cgi?id=849398

Nit: no need for the bug url.

@@ +209,5 @@
>          // OOM: load error value, discard return address and previous frame
>          // pointer and return.
> +        // Take JSReturnOperand here because it may have been added back by reg_code
> +        // (happens on Win64/x64).  See https://bugzilla.mozilla.org/show_bug.cgi?id=849398
> +        regs.takeUnchecked(JSReturnOperand);

Nit: I don't think we need the regs.add(reg_code) + takeUnchecked here, there's no register pressure so it's simpler to just "reserve" them the whole time.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 723552 [details] [diff] [review]
Patch

Does not compile on Win64. Compile error will be attached soon.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: compile error

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: compile error take 2

Comment 10

[Kannan Vijayan [:djvj]]

Per confirmation from gkw on IRC, comments addressed and pushed (with fixes to address compile issues above):
https://hg.mozilla.org/projects/ionmonkey/rev/ef0e134ef78f