675251 - TI: Assertion failure: (uint8*)ic.funGuard.executableAddress() + ic.joinPointOffset == returnAddress, at methodjit/MethodJIT.cpp:1264

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on TI revision d43c6dddeb2b (run with -j -m -n -a), tested on 64 bit:


function printBugNumber (num) {
  BUGNUMBER = num;
  print ('BUGNUMBER: ' + num);
}
var actual = '';
test();
function test() {
  printBugNumber(test);
  function f(N) {
    for (var i = 0; i != N; ++i) {
      for (var repeat = 0;repeat != 2; ++repeat) {
        var count = BUGNUMBER(repeat);
      }
    }
  }
  var array = [function() { f(10); }, ];
  for (var i = 0; i != array.length; ++i)
    array[i]();
}

Comment 1

[Christian Holler (:decoder)]

Further note on this: I wasn't able to remove the "print" call here but later found out that you can replace "print" by another function like "unescape" which will speed up the test a bit and not spill junk on your terminal.

Comment 2

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

I see this assert too - but my testcases are often unreproducible and unreliable.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

The problem here was that after expanding only the topmost inline frame when making an Invoke() call, the prevPC values were not filled in for the outer frame or previous frames on the stack.  After reentering the interpreter on the outer frame, it went back into the mjit, overwriting the entry frame's ncode without its prevPC being set and leading to a later crash when that information was needed.

I think that with the new method for moving between the interpreter and jitcode (jitcode can finish anywhere under its initial frame, finishing those in the interpreter), allowing inline frame expansion without expanding *all* inline frames in the compartment (which will fill in prevPC values for the stack as well) is basically broken.  A topmost inline jitframe could be expanded, which will throw everything associated with the VMFrame into the interpreter, which could then proceed to pop other inline frames that were never actually expanded.

This fix changes things so that if we expand any inline frames in a compartment, we expand all of them.  With this bigger penalty it also makes the change described in bug 673763 of marking functions uninlineable when they make Invoke() or similar calls that go through ensureOnTop, so that inline frames for them will not be expanded over and over again.

http://hg.mozilla.org/projects/jaegermonkey/rev/d763fda00eb9

Comment 4

[Luke Wagner [:luke]]

Can you do the MarkTypeObjectFlags(UNINLINEABLE) inside ExpandInlineFrames instead of in ensureOnTop?  ensureOnTop is certainly a place where we can get pathological inlining, but it seems like most if not all ExpandInlineFrames callers could be as well.  Is there any benchmark that loses valid inlining if ExpandInlineFrames marks everything as UNINLINEABLE?  If so, perhaps you could give a 1 or 2 bit counter before marking permanently UNINLINEABLE...

Comment 5

[Brian Hackett [Laid off!]]

(In reply to comment #4)
> Can you do the MarkTypeObjectFlags(UNINLINEABLE) inside ExpandInlineFrames
> instead of in ensureOnTop?  ensureOnTop is certainly a place where we can
> get pathological inlining, but it seems like most if not all
> ExpandInlineFrames callers could be as well.  Is there any benchmark that
> loses valid inlining if ExpandInlineFrames marks everything as UNINLINEABLE?
> If so, perhaps you could give a 1 or 2 bit counter before marking
> permanently UNINLINEABLE...

A problem here is that there are legitimate reasons to expand inline frames, mainly when we need to recompile a script.  I am worried about using a small counter as in some cases it would be relatively easy to trip accidentally and cause chaotic perf effects.

I think most of the remaining places where we could get pathological behavior are related to exceptions (script decompiling, etc.), where we need to expand all inline frames on the stack anyways.  Is it OK to wait and see if we actually do see bad behavior?  (if/when we do, the state of the world may have changed and we may have new ways of looking at this problem).

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 549824 [details] [diff] [review]
patch

Fair enough.

Comment 7

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929