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

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: bbouvier)

Tracking

(Blocks: 2 bugs, {assertion, regression, testcase})

Trunk
mozilla24
x86_64
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
(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.
(Assignee)

Comment 1

4 years ago
Created attachment 758080 [details] [diff] [review]
proposed fix

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)
(Assignee)

Updated

4 years ago
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)

Comment 4

4 years ago
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 5

4 years ago
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).
(Assignee)

Comment 7

4 years ago
Created attachment 759377 [details] [diff] [review]
proposed fix addressing comments
Assignee: general → bbouvier
Attachment #758080 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #759377 - Flags: review?(luke)

Comment 8

4 years ago
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+
(Assignee)

Comment 9

4 years ago
Created attachment 759430 [details] [diff] [review]
same patch witch fixed nit

Carrying r+.

I feel like I should read the Spidermonkey coding style every days.
Attachment #759377 - Attachment is obsolete: true
Attachment #759430 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e50dde140bc
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1e50dde140bc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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.