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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: decoder, Unassigned)

Tracking

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

Trunk
x86_64
Linux
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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]();
}
(Reporter)

Comment 1

7 years ago
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.
I see this assert too - but my testcases are often unreproducible and unreliable.
Created attachment 549824 [details] [diff] [review]
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
Attachment #549824 - Flags: review?(luke)

Comment 4

6 years ago
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...
(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).
(Reporter)

Updated

6 years ago
Blocks: 676763

Comment 6

6 years ago
Comment on attachment 549824 [details] [diff] [review]
patch

Fair enough.
Attachment #549824 - Flags: review?(luke) → review+
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 7

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