Closed Bug 878520 Opened 7 years ago Closed 7 years ago

"Assertion failure: isInterpreted()" with asm module inside recursive function

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jruderman, Assigned: bbouvier)

References

Details

(Keywords: assertion, regression, testcase)

Attachments

(1 file, 2 obsolete files)

(function rec(depth) {
    if (depth) {
        function unusedFun() {
            var asmModule = function () { 'use asm'; return {}; };
        };
        rec(depth - 1);
    }
})(2);

Assertion failure: isInterpreted(), at js/src/jsfun.h:220

#0  0x000000010000eca2 in JSFunction::getOrCreateScript ()
#1  0x000000010022e47b in js::CloneScript ()
#2  0x000000010022f116 in js::CloneFunctionScript ()
#3  0x00000001001852fb in js::CloneFunctionObject ()
#4  0x00000001001d258b in js::CloneFunctionObjectIfNotSingleton ()
#5  0x00000001001ce32b in js::DefFunOperation ()
#6  0x00000001001c6076 in js::Interpret ()
#7  0x00000001001c19b3 in js::RunScript ()
#8  0x00000001001cad2a in js::ExecuteKernel ()
#9  0x00000001001caf6e in js::Execute ()
#10 0x000000010012eb24 in JS_ExecuteScript ()
#11 0x00000001000036bd in Process ()
#12 0x0000000100001bb7 in Shell ()
#13 0x000000010000289e in main ()

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/30b977b2b911
user:        Nicholas Nethercote
date:        Thu Mar 14 18:44:03 2013 -0700
summary:     Bug 851421 (part 2) - Don't emit bytecode for asm.js functions unless linking fails.  r=luke.
Attached patch proposed fix (obsolete) — Splinter Review
If I understand correctly, the problem is that when copying the script, functions are all considered to be interpreted, while they could actually be native.

As asm.js modules are autonomous until linkage and cannot embed closures or external variables, we can just reuse the module function in every clone containing it. What do you think of this?
Attachment #758080 - Flags: feedback?(n.nethercote)
Duplicate of this bug: 879132
Comment on attachment 758080 [details] [diff] [review]
proposed fix

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

Better ask luke -- I'm not very familiar with the cloning stuff.  Sorry.

(I can point out that opening braces in |if| statements are meant to go on the same line, though...)
Attachment #758080 - Flags: feedback?(n.nethercote) → review?(luke)
A related question is why this recursively-called function is being given a singleton type!  Investigated and filed bug 880085.  This was a recent change which is why this bug (which is also real and should be fixed) only showed up recently.
Comment on attachment 758080 [details] [diff] [review]
proposed fix

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

Btw, thanks for looking into this Benjamin and nice job digging in!  Normally, with every bug-fix, we add the regressing testcase.  In our case, the testcase in comment 0 will cease to be a testcase soon (once bug 880085 is fixed), so could you add this testcase instead:

function surprise(depth) {
    arguments.callee.caller(depth);
}
(function(depth) {
    function foo() { function asmModule() { 'use asm'; return {} } };
    if (depth)
        surprise(depth - 1);
})(2);

(Hitting the same case without using a named lambda.)  The normal file path would be js/src/jit-tests/tests/asm.js/testBug878520.js.

::: js/src/jsscript.cpp
@@ +2302,5 @@
>              } else if (obj->isFunction()) {
>                  RootedFunction innerFun(cx, obj->toFunction());
> +                if (innerFun->isNative())
> +                {
> +                    clone = innerFun;

Can you add
  JS_ASSERT(innerFun->compartment() == cx->compartment);
?
Attachment #758080 - Flags: review?(luke) → review+
> JS_ASSERT(innerFun->compartment() == cx->compartment);
Or maybe assertSameCompartment(cx, innerFun).
Attached patch proposed fix addressing comments (obsolete) — Splinter Review
Assignee: general → bbouvier
Attachment #758080 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #759377 - Flags: review?(luke)
Comment on attachment 759377 [details] [diff] [review]
proposed fix addressing comments

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

Thanks again!

::: js/src/jsscript.cpp
@@ +2301,5 @@
>                  clone = CloneStaticBlockObject(cx, enclosingScope, innerBlock);
>              } else if (obj->isFunction()) {
>                  RootedFunction innerFun(cx, obj->toFunction());
> +                if (innerFun->isNative())
> +                {

{ on same line as if
Attachment #759377 - Flags: review?(luke) → review+
Carrying r+.

I feel like I should read the Spidermonkey coding style every days.
Attachment #759377 - Attachment is obsolete: true
Attachment #759430 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1e50dde140bc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.