Closed Bug 593596 Opened 9 years ago Closed 9 years ago

TM: "Assertion failure: have_call == (fp->hasFunction() && fp->getFunction()->isHeavyweight()),"

Categories

(Core :: JavaScript Engine, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: gkw, Assigned: billm)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

(function() {
  for (let a = 0; a < 4; ++a) {
    M: for each(var e in <x>></x>) {
      break M
    }
  }
})()

asserts js debug shell on TM changeset 60af58b42567 with -j at Assertion failure: have_call == (fp->hasFunction() && fp->getFunction()->isHeavyweight()),
Summary: "Assertion failure: have_call == (fp->hasFunction() && fp->getFunction()->isHeavyweight())," → TM: "Assertion failure: have_call == (fp->hasFunction() && fp->getFunction()->isHeavyweight()),"
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   52830:391c5c261186
user:        Bill McCloskey
date:        Thu Sep 02 10:50:15 2010 -0700
summary:     Bug 590006 - escaping closures on trace don't get block objects in their scope chain (r=lw)
Blocks: 590006
Here's what's happening:
1. The XML fragment causes a call to js_GetScopeChain inside the let block (not sure why...), so that the scope chain has a call object and a block object on it.
2. The tracer starts recording on the last loop iteration. It records the break, the loop exit, and then JSOP_STOP.
3. When recording JSOP_STOP, putActivationObjects asserts that there is a call object iff the function is heavyweight (and this function is not heavyweight).

Before the changeset above, we required that traces never leave the lexical block in which they start, so this situation never could have happened.

I'm having difficulty telling in what situation putActivationObjects actually works. Naively it seems like everything should be okay and the assertion is just too strong. However, there are a lot of differences between js_PutCallObjectOnTrace (which it calls) and js_PutCallObject (the normal version) that I can't explain. In particular, a lot of stuff happens differently if the method JIT is enabled. I'll look at this more, but maybe someone more knowledgeable should also take a look.
(In reply to comment #2)
> Here's what's happening:
> 1. The XML fragment causes a call to js_GetScopeChain inside the let block (not
> sure why...),

Where is the call to js_GetScopeChain?

On its face the <x></x> should make the function heavyweight, if this call is truly required.

/be
Before a recent bugfix (bug 592199), we had

  have_call = (fp->hasFunction && fp->getFunction()->isHeavyweight())

I replaced that with (fp->hasCallObj), since that seemed more direct, and put the assert in because we seemed to have been operating under that assumption so I thought it had better be true.  But perhaps it was better the original way?  That is, what if we enter a non-heavyweight frame (so don't record creating a call object), then create a scope chain, then record leaving the frame (so record a js_PutCallObjectOnTrace), and then manage not to call js_GetScopeChain on trace (thereby not bailing).  That would be bad.  So I think the fix is to restore the original definition of have_call and add a phat comment explaining why.
OTOH, it seems like a call to js_GetScopeChain from a non-heavyweight function during recording is just flirting with danger and perhaps it should abort recording.
(In reply to comment #3)
> Where is the call to js_GetScopeChain?

It's called from js_GetDefaultXMLNamespace.

Luke, I don't really understand what goes wrong in the scenario you described. Is it the following? We decide whether to compile js_PutCallObjectOnTrace into the trace based on whether there is a call object at recording time. But it seems like the trace doesn't have a guard for this. So when we invoke the trace later, there might not be a call object. Or vice versa: we record with no call object and then execute with a call object. Is this the problem?
It would be great to turn a js_GetScopeChain call from native code triggered from a non-heavyweight function into an error, but debugger use-cases remain. Comment 5 is right on the money though: no tracing for you!

Separately, if the compiler sees a special form in function code that results inevitably in js_GetScopeChain being called, the compiler has a bug if it fails to flag the function as heavyweight.

So, two things to fix.

/be
(In reply to comment #7)
> It would be great to turn a js_GetScopeChain call from native code triggered
> from a non-heavyweight function into an error, but debugger use-cases remain.
> Comment 5 is right on the money though: no tracing for you!

I still don't understand what actually can go wrong here. Are you guys just concerned that there might be a bug in there somewhere because it's not well tested?

> Separately, if the compiler sees a special form in function code that results
> inevitably in js_GetScopeChain being called, the compiler has a bug if it fails
> to flag the function as heavyweight.

Creating a flat closure also leads to a js_GetScopeChain call. Should that also cause a function to be flagged as heavyweight? There's also a call inside WrapEscapingClosure that looks suspicious.
(In reply to comment #8)
> (In reply to comment #7)
> > It would be great to turn a js_GetScopeChain call from native code triggered
> > from a non-heavyweight function into an error, but debugger use-cases remain.
> > Comment 5 is right on the money though: no tracing for you!
> 
> I still don't understand what actually can go wrong here. Are you guys just
> concerned that there might be a bug in there somewhere because it's not well
> tested?

The problem is that the compiler optimizes away Call objects by emitting bytecode specialized to bypass the scope chain. So any runtime creation of a Call object that does not deoptimize (see WrapEscapingClosure in jsfun.cpp) is broken.

Tracing just ups the ante by optimizing harder.

> > Separately, if the compiler sees a special form in function code that results
> > inevitably in js_GetScopeChain being called, the compiler has a bug if it fails
> > to flag the function as heavyweight.
> 
> Creating a flat closure also leads to a js_GetScopeChain call.

Not for the activation of the flat closure, note well: rather for the closure's outer scope when the closure is computed.

> Should that also cause a function to be flagged as heavyweight?

The outer function(s) that hold upvars used by the (partially) flat closure are already flagged, see FlagHeavyweights in jsparse.cpp.

> There's also a call inside WrapEscapingClosure that looks suspicious.

See above -- those are for the closure when it might escape what the compiler thought was the extent in which it was confined, not when the closure is called. Since an escapee could be called without the needed upvars available at the expected skip/slot indexes along the scope chain, the VM must wrap with a deoptimized version of the flat closure that is willing to do name lookups.

/be
(In reply to comment #8)
The problem I was worried about was if we record a trace that does not create a call object for a frame but does call js_PutCallObjectOnTrace on the frame's scope chain.  As is, I think the definition of have_call makes that possible if we call js_GetScopeChain while recording a non-heavyweight function, since the only place where we record creating a call object is in record_EnterFrame for heavyweight functions.
Attached patch proposed fixSplinter Review
After talking this over with Luke, I think this fix should be sufficient. The concern was that we might not decide correctly whether to do PutCallObject if the call object is created lazily in a non-heavyweight function.

However, this is only a concern because we created the call object before recording began and then we tried to record a function return, which is when the PutCallObject is needed. However, any trace that starts in the middle of a function and then tries to return will be aborted, so the problem doesn't actually arise.
Attachment #472785 - Flags: review?(lw)
blocking2.0: ? → betaN+
Comment on attachment 472785 [details] [diff] [review]
proposed fix

Sorry for the delay.
Attachment #472785 - Flags: review?(lw) → review+
Assignee: general → wmccloskey
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.