TM: "Assertion failure: pendingGlobalSlotToSet == -1"

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: jruderman, Assigned: luke)

Tracking

(Blocks 1 bug, {assertion, regression, testcase})

Trunk
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker][fixed-in-tracemonkey])

Attachments

(3 attachments)

Reporter

Description

9 years ago
./js -j c.js

Assertion failure: pendingGlobalSlotToSet == -1, at /Users/jruderman/tracemonkey/js/src/jstracer.cpp:3689

The first bad revision is:
changeset:   38f985b0522c
user:        Luke Wagner
date:        Thu Nov 18 10:49:45 2010 -0800
summary:     Bug 561954 - Abort recording earlier to avoid expensive later bails (r=jorendorff)
Reporter

Comment 1

9 years ago
Posted file stack trace
This test triggers the same assert:
---
var a, b;
function g(x) {
    var y = x++;
    return [x, y];
}
function f() {
    for(var i=0; i<20; i++) {
        [a,b] = g("10");
    }
}
f();
---
blocking2.0: --- → ?
Keywords: regression
blocking2.0: ? → .x
Assignee

Comment 3

9 years ago
Ah, it seems there can be more than one global write per recorded op; should've seen this coming.  *sigh* this tracer design is killing me.  I think the solution is to turn pendingGlobalSlotToSet into a Vector<int>.

Until then, this assert can be safely commented out; its not a correctness bug, it will just cause some unnecessarily-pessimistic trace aborts.
Assignee

Comment 4

9 years ago
While the potential for real exploits is low, I can see the assert hitting in the wild and wasting debug-build users' time, so I'm going mark this blocking.
blocking2.0: .x → final+
Assignee

Comment 5

9 years ago
Posted patch fixSplinter Review
Assignee: general → lw
Status: NEW → ASSIGNED
Attachment #500915 - Flags: review?(jorendorff)
Whiteboard: softblocker
Blah. The assertion happens under TR::closeLoop, in SlotMap::adjustType, right? If so, we do not expect the interpreter to write to these global slots. It won't even get a chance; we're done recording. It seems more direct to skip the pendingGlobalSlotToSet code (and this assert) if we're in closeLoop.
Assignee

Comment 7

9 years ago
Oh, hah, I hadn't even checked the backtrace; just that the testcase was fixed.  Other than through enumeration, I can't find any hard reason why there can't be multiple writes to a global in a single op, so I'd almost rather just put in the general form, unless you think that there is no remotely likely chance, now or ever of this occurring outside closeLoop.
Comment on attachment 500915 [details] [diff] [review]
fix

Hmm. Well... all things being equal, I'd rather remove the assertion or change it to an `if (assumption violated) RETURN_STOP();`, than have a bit of machinery we think can't be used at present, and have no particular reason to think will ever be useful.

However, we are in a hurry. I'll leave this to your judgment. The patch looks correct. r=me.
Attachment #500915 - Flags: review?(jorendorff) → review+
Assignee

Comment 9

9 years ago
(In reply to comment #8)
> Hmm. Well... all things being equal, I'd rather remove the assertion or change
> it to an `if (assumption violated) RETURN_STOP();`, than have a bit of
> machinery we think can't be used at present, and have no particular reason to
> think will ever be useful.

I would agree and initially considered this but: (1) there is no easy way to RETURN_STOP() from Tracker::setImpl; certainly we'd not want to force all callers of set() to check and if we only made the global-setting callers check, we'd be back to my old grosser patch that had to find every place we set a global in the tracer, (2) we'd still need to insert some special case "if we are closing the loop" predicate which would add another ounce of hack.

So, despite its lack of foreseeable use outside closeLoop, I think this patch is the least hacky route.
Assignee

Comment 10

9 years ago
http://hg.mozilla.org/tracemonkey/rev/18e064a4bb68
Whiteboard: softblocker → [softblocker][fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/18e064a4bb68
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.