1231024 - Assertion failure: !minimalBundle(bundle) at jit/BacktrackingAllocator.cpp:1297

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: minidump crash report

1. https://communities.intel.com/community/itpeernetwork/datastack/blog/2013/08/05/how-to-maximise-cpu-performance-for-the-oracle-database-on-linux

2. Assertion failure: !minimalBundle(bundle), at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/js/src/jit/BacktrackingAllocator.cpp:1297

Windows, Linux Nightly/45 at least

reproduced with https://hg.mozilla.org/mozilla-central/rev/cc9c6cd756cb744596ba039dcc5ad3065a7cc3ea though that is not the regressor.

I can reproduce with a saved version of the page. I'll try to reduce.

Comment 1

[Jan de Mooij [:jandem]]

Brian do you have some cycles to look at this? Thanks!

Comment 2

[Brian Hackett [Laid off!]]

If this can be reduced and is reproducible then a blame cset would be good.  This assertion is often a bug in the lowering phase.

Comment 3

[Bob Clary [:bc] (inactive)]

It is reproducible and I am working on reducing it but it will take time. mozregression doesn't support debug builds for a long enough range and I don't know when I will be able to manually bisect it.

Comment 4

[Jan de Mooij [:jandem]]

Alice0775, CC'ing you in case you're bored and looking for a bug to bisect :)

Comment 5

[Alice0775 White]

(In reply to Jan de Mooij [:jandem] from comment #4)
> Alice0775, CC'ing you in case you're bored and looking for a bug to bisect :)

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=21730adb78b62b34cd4ed45eb08b9841bb1524ef&tochange=cd35e12d8f91f410f71b6f5d3023a40e9071e24b

Suspect: Bug 1108290

Comment 6

[Lars T Hansen [:lth]]

32-bit or 64-bit?

Comment 7

[Bob Clary [:bc] (inactive)]

Looks like 32bit only. Windows 7 and RHEL6.

Crashes opt too:

bp-85b1bd8a-4cb0-4c51-a15a-1c2c62151208
bp-ff17cd4a-f147-4c8a-b78f-0a0bf2151208

[@ js::jit::BacktrackingAllocator::splitAt ]

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: minidump crash report nightly opt win61i32

Bughunter did see an additional opt signature on Windows

js::jit::TempAllocator::allocateInfallible(unsigned int) with an opt assertion?

Assertion failure: result ([OOM] Is it really infallible?), at c:\builds\moz2_slave\m-cen-w32-00000000000000000000\build\src\js\src\ds/LifoAlloc.h:281

Comment 9

[Lars T Hansen [:lth]]

Disregarding the possible OOM issue:  The register management for that optimization on 32-bit x86 is particularly unpleasant, because there are too few registers to go around and some do double duty.  It's possible there's a reg assignment bug or (more likely) that there's a regalloc invariant I wasn't aware of.

Comment 10

[Brian Hackett [Laid off!]]

(In reply to Lars T Hansen [:lth] from comment #9)
> Disregarding the possible OOM issue:  The register management for that
> optimization on 32-bit x86 is particularly unpleasant, because there are too
> few registers to go around and some do double duty.  It's possible there's a
> reg assignment bug or (more likely) that there's a regalloc invariant I
> wasn't aware of.

If someone can attach the output from setting IONFLAGS=regalloc for the compilation where it's crashing, that should help with diagnosing the problem.

Comment 11

[Bob Clary [:bc] (inactive)]

Attachment: 1231024.log

With a debug build from today with IONFLAGS=regalloc  I get

Assertion failure: int32_t(outlen) > 0, at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/js/src/jsprf.cpp:999

on startup without even attempting to load the url.

Comment 12

[Lars T Hansen [:lth]]

(In reply to Bob Clary [:bc:] from comment #11)
> Created attachment 8697020 [details]
> 1231024.log
> 
> With a debug build from today with IONFLAGS=regalloc  I get
> 
> Assertion failure: int32_t(outlen) > 0, at
> c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/js/src/jsprf.
> cpp:999
> 
> on startup without even attempting to load the url.

That sounds like bug 1231581, making that a blocker for the time being.

Comment 13

[Wei Wu [:wuwei UTC+8]]

(In reply to Lars T Hansen [:lth] from comment #12)
> (In reply to Bob Clary [:bc:] from comment #11)
> > Created attachment 8697020 [details]
> > 1231024.log
> > 
> > With a debug build from today with IONFLAGS=regalloc  I get
> > 
> > Assertion failure: int32_t(outlen) > 0, at
> > c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/js/src/jsprf.
> > cpp:999
> > 
> > on startup without even attempting to load the url.
> 
> That sounds like bug 1231581, making that a blocker for the time being.

Drive-by comment:

liveRange dump has a relatively small buffer for JitSpew, which has only 2000 bytes currently.
I tried to enlarge it to 4096, and crashed again. turns out the liveBundle and liveRange could be
huge, and this is the largest result I profiled agains kraken benchmark:

LiveRange usesCount: 760
LiveRange usesCount: 762
LiveRange usesCount: 762
LiveRange usesCount: 762
LiveRange usesCount: 770
LiveRange usesCount: 770
LiveRange usesCount: 770
LiveBundle usesCount: 7
LiveBundle usesCount: 7
LiveBundle usesCount: 7

the easiest way is to try enlarge the buffer size (both LiveRange::toString() and LiveBundle::toString()), or you can replace the MOZ_CRASH() with 'break', if it is urgent.

Comment 14

[Wei Wu [:wuwei UTC+8]]

Attachment: bug1231024.workarounds.patch

Hi,

so the simplest workarounds it to add the buffer size up to 20000.
(the buffers are in static var area, not on the stack, so 20k is ok.)
Tested on kraken benchmark. Hope it helps :)

Comment 15

[Bob Clary [:bc] (inactive)]

Wei Wu: I did a Windows 32bit try build with your patch. You can find it at https://treeherder.mozilla.org/#/jobs?repo=try&revision=408fd6ef12a3&selectedJob=14557749

Running the debug build on Windows 7 64bit still asserts: Assertion failure: !minimalBundle(bundle), at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/js/src/jit/BacktrackingAllocator.cpp:1297

Running the build on Windows 7 32bit also still asserts: Assertion failure: !minimalBundle(bundle), at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/js/src/jit/BacktrackingAllocator.cpp:1297

Comment 16

[Wei Wu [:wuwei UTC+8]]

Attachment: bug1231024.truncates.patch

(In reply to Bob Clary [:bc:] from comment #15)
> Wei Wu: I did a Windows 32bit try build with your patch. You can find it at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=408fd6ef12a3&selectedJob=14557749
> 
> Running the debug build on Windows 7 64bit still asserts: Assertion failure:
> !minimalBundle(bundle), at
> c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/js/src/jit/
> BacktrackingAllocator.cpp:1297
> 
> Running the build on Windows 7 32bit also still asserts: Assertion failure:
> !minimalBundle(bundle), at
> c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/js/src/jit/
> BacktrackingAllocator.cpp:1297

ok seems like the dump is realy big.. this patch outputs "truncates" when the buffer is full instead of crash. Hope this patch will work :)

Comment 17

[Bob Clary [:bc] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd6dda17a654 build failure. Please needinfo me when you have a good try debug build I can download.

Comment 18

[Wei Wu [:wuwei UTC+8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58dfb54e7b91

Comment 19

[Wei Wu [:wuwei UTC+8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9845ea11946f
looks green now.

FYI it was built on top of mozilla-central. if you want to build in mozilla-inbound,
try apply https://hg.mozilla.org/try/rev/9845ea11946f.

Comment 20

[Wei Wu [:wuwei UTC+8]]

Comment on attachment 8697954 [details] [diff] [review]
bug1231024.truncates.patch

obsolete old patches.

Comment 21

[Wei Wu [:wuwei UTC+8]]

Comment on attachment 8697589 [details] [diff] [review]
bug1231024.workarounds.patch

obsolete old patches.

Comment 22

[Bob Clary [:bc] (inactive)]

Same assertion, sorry.

Comment 23

[Wei Wu [:wuwei UTC+8]]

(In reply to Bob Clary [:bc:] from comment #22)
> Same assertion, sorry.
so can we get IONFLAGS=regalloc jitspew logs?

Comment 24

[Bob Clary [:bc] (inactive)]

Attachment: ionflags.txt.gz

Worked this time. :-)

Comment 25

[Wei Wu [:wuwei UTC+8]]

(In reply to Bob Clary [:bc:] from comment #24)
> Created attachment 8698062 [details]
> ionflags.txt.gz
> 
> Worked this time. :-)

Thank you :)

Comment 26

[Bob Clary [:bc] (inactive)]

Attachment: testcase.html

Comment 27

[Jan de Mooij [:jandem]]

Attachment: Shell testcase

Bob, thanks a lot for the reduced testcase! Here's a version that runs in the shell.

It asserts with --no-threads on x86.

We also have a crash in BacktrackingAllocator::splitAt on crash-stats, might be the same issue.

Comment 28

[Jan de Mooij [:jandem]]

Brian, Lars: could one of you please take a look?

[Tracking Requested - why for this release]:

Comment 29

[Lars T Hansen [:lth]]

At issue is this snippet on lines 14-16:

  e = "function" == typeof w ? w.apply(void 0, e) : w, a.a(function(s, p) {
      p && s && (b[s] = p, h(s))
  }, o, e)

more clearly written as 

  if (typeof w == "function")
     e = w.apply(void 0, e);
  else
     e = w;
  a.a(function(s, p) { p && s && (b[s] = p, h(s)) }, o, e)

where only the second line matters.  Without looking more deeply yet it could appear that the output of the call requires a register while at the same time the implementation of apply uses all the available registers for its own purposes.  I'll try to construct a smaller test case to demonstrate this.

Comment 30

[Lars T Hansen [:lth]]

I actually think that this additionally is the result of some inlining which ends up creating a function with two inlined apply instances, which then seem to conflict in their allocations, but I'm not yet adept at reading the regalloc spew to the point where I understand it fully (though I have a much smaller log than the one previously offered).

Comment 31

[Lars T Hansen [:lth]]

The problem is the following deobfuscated function:

    function define_() {
        var args = [].slice.call(arguments);
        var key = "string" == typeof args[0] ? args.shift() : void 0;
        var keys = args.length > 1 ? args.shift() : [];
	let w = args[0];	
	for (let x=0;;) {
	    let t = function ()                       // *A*
	    {
		let values = [];
                for (let k=0; k < keys.length ; k++)  // *B*
		    values.push(definitions[keys[k]]) // *C*
                if (typeof w == "function") {
		    values = w.apply(void 0, values);
		}
		else
		    values = w;
		if (values && key)
		    definitions[key] = values;
	    }
	    t();                                      // *D*
	    if (x >= keys.length)
		break;
	    x++;
	}
    }

This fails with the same assertion (within the context of the program, I'll attach an updated TC).  It can be made to work in two ways: either by commenting out the lines at *A* and *D* so that the function is in-line, or by moving the two lines at *B* and *C* into the "then" arm of the following "if", where they belong.

Comment 32

[Lars T Hansen [:lth]]

Attachment: Updated shell test case

Run this with --no-threads --ion-warmup-threshold=100.

Comment 33

[Lars T Hansen [:lth]]

Attachment: Updated shell test case (much smaller)

Comment 34

[Lars T Hansen [:lth]]

Attachment: Updated shell test case (much smaller still)

Last go-around, this is the smallest I've been able to make it.  It appears that some amount of polymorphism at the call site is necessary; if there is not a sufficiently large number of functions then the assertion is not triggered.  And there needs to be some diversity in control paths through the function.

However, inlining is not necessary.  This still fails in a 32-bit Mac OS X build with --no-threads --ion-warmup-threshold=100 --ion-inlining=off.

Comment 35

[Lars T Hansen [:lth]]

Attachment: reglog

regalloc spew for a run of the final test case with the flags from my last comment

Comment 36

[Lars T Hansen [:lth]]

Based on my inspection of the spew last week it could look like this fails on x86 because some values are Values and need more than one register, but I'm still not familiar enough with regalloc to say.  It's unfortunately likely that I won't have time to figure this out before the train leaves.  I'll offer a patch to disable the optimization on X86 for now.

Comment 37

[Lars T Hansen [:lth]]

Attachment: disable fn.apply(...,array) optimization on x86

Comment 38

[Nicolas B. Pierron [:nbp]]

(In reply to Lars T Hansen [:lth] from comment #37)
> Created attachment 8706245 [details] [diff] [review]
> disable fn.apply(...,array) optimization on x86

If we do that, I will recommend to do it based on the value representation instead of the architecture.  Such that we don't get similar issues on ARM.

Comment 39

[Lars T Hansen [:lth]]

(In reply to Nicolas B. Pierron [:nbp] from comment #38)
> (In reply to Lars T Hansen [:lth] from comment #37)
> > Created attachment 8706245 [details] [diff] [review]
> > disable fn.apply(...,array) optimization on x86
> 
> If we do that, I will recommend to do it based on the value representation
> instead of the architecture.  Such that we don't get similar issues on ARM.

That seems reasonable, even if I think ARM should have enough registers to be OK here.

The problem is that I have very limited time right before the FF46 train leaves, and what I have is not a very deep diagnosis of the problem - I could be wrong about what's going on.

(It might be safer just to disable the optimization on all platforms until we have a diagnosis.)

Comment 40

[Brian Hackett [Laid off!]]

Attachment: fix spew

Unfortunately, the regalloc spew is still messed up.

Comment 41

[Brian Hackett [Laid off!]]

With the above patch, it's clearer what the problem is from the regalloc spew.  Looking at the assertion failure:

[RegAlloc] Allocating  v125 [298,300) v125:%ecx@298 [priority 2] [weight 2000000]
[RegAlloc]   Requirement %ecx, due to use at 298
[RegAlloc]   %ecx collides with  v139 [299,302) %ecx (def) v139:*@301 [weight 2000000]
Assertion failure: !minimalBundle(bundle)

The [298,300) is the instruction range being allocated, and looking at the instructions from the beginning of the register allocation:

[298,299 ApplyArrayGeneric] [def v139<t>:%ecx] [def v140<p>:%edx] [temp v135<g>:%eax] [temp v136<g>:%ebx] [use v125:%ecx] [use v134:%edi] [use v137:%esi] [use v138:%edx] [use v84:*] [use v84:*] [use v85:*] [use v86:*] [use v87:*] [use v91:*] [use v84:*] [use v99:*] [use v101:*] [use v125:*] [use v130:*] [use v131:*]

v139 is a fixed definition in %ecx, and v125 is a fixed use in %ecx.  This would be OK if v125 was usedAtStart by the instruction, in which case its live range covers only the input of the instruction and not the output (this permits the register allocator to reuse the input's register for an output).  Unfortunately you can't tell from the 'use' above whether the use is AtStart or not, but looking at the source for LIRGenerator::visitApplyArray shows the two uses are useFixed instead of useFixedAtStart, and changing the first one to useFixedAtStart fixes this assertion.  I don't know if doing that will require other changes to the ApplyArray codegen though, the latter looks pretty complicated.

Comment 42

[Lars T Hansen [:lth]]

Thanks, that's very helpful.  That code is hairy so I'll need to look into it, but at least there's no indication of a regalloc bug, ie, we can disable the opt on x86 if we want to, cf pending patch, if there's not enough time to fix this before the train leaves.

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a79e9b6b0da5

Comment 44

[Carsten Book [:Tomcat]]

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

Comment 45

[Lars T Hansen [:lth]]

That patch was not a fix, just a correction to regalloc spew.

Comment 46

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9565e4602dbc

Comment 47

[Lars T Hansen [:lth]]

I believe both uses could be marked AtStart, trying that now.

Comment 48

[Lars T Hansen [:lth]]

Attachment: narrow the live range for values

Comment 49

[Sylvestre Ledru [:Sylvestre]]

assert crash, taking it.

Comment 50

[Jan de Mooij [:jandem]]

Comment on attachment 8709040 [details] [diff] [review]
narrow the live range for values

Review of attachment 8709040 [details] [diff] [review]:
-----------------------------------------------------------------

Since all registers are fixed, do you know why this didn't fail all the time?

Comment 51

[Lars T Hansen [:lth]]

(In reply to Jan de Mooij [:jandem] from comment #50)
> Comment on attachment 8709040 [details] [diff] [review]
> narrow the live range for values
> 
> Review of attachment 8709040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since all registers are fixed, do you know why this didn't fail all the time?

I don't.  Given my experiments with the test case (comment 34) it seems that some amount of polymorhism is required to have been detected at the time the code is being jitted, for example, there needs to be a certain number of distinct functions that are applied at the call site; my guess is that this increases register pressure or further constrains register assignment and thus triggers the failure.  (Not much of an analysis.)

Comment 52

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8c14902800d80d248d8adf451c2f635fd611d2
Bug 1231024 - narrow the live range for values.  r=jandem

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c8c14902800

Comment 54

[Sylvestre Ledru [:Sylvestre]]

Lars, could you request the uplift to beta? Thanks

Comment 55

[Sylvestre Ledru [:Sylvestre]]

Jan, this is a top crash of 45. Could you fill the uplift request for Lars? Thanks

Comment 56

[Lars T Hansen [:lth]]

Comment on attachment 8709040 [details] [diff] [review]
narrow the live range for values

Approval Request Comment
[Feature/regressing bug #]: Corrects bug in bug 1108290
[User impact if declined]: Crashes
[Describe test coverage new/current, TreeHerder]: None in patch
[Risks and why]: Zero, to the best of my knowledge, corrects regalloc bug
[String/UUID change made/needed]: None

Comment 57

[Lars T Hansen [:lth]]

Jan, I took care of this.

Comment 58

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8709040 [details] [diff] [review]
narrow the live range for values

thanks
Should be in 45 beta 3

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/785489d45d2d

Comment 60

[Paul Silaghi, QA [:pauly]]

Reproduced the issue on Nightly 2015-12-08 debug build, Win 7 x64.
Verified fixed 45b7, 46.0a2 (2016-02-19) debug builds.

Comment 61

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'js::jit::BacktrackingAllocator::splitAt':
 - nightly (version 50): 1 crash from 2016-06-06.
 - aurora  (version 49): 16 crashes from 2016-06-07.
 - beta    (version 48): 357 crashes from 2016-06-06.
 - release (version 47): 805 crashes from 2016-05-31.
 - esr     (version 45): 283 crashes from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          1          0          0          0          0          0
 - aurora           0          3          1          4          3          4          0
 - beta            65         49         57         56         51         45         17
 - release        105        102        121        124        124        130         50
 - esr             19         30         35         38         25         29         28

Affected platforms: Windows, Mac OS X, Linux

Comment 62

[Lars T Hansen [:lth]]

Re the crash reports above, those are likely something else than this bug.  (This bug was x86-only, for one thing; I think we don't build 32-bit for Mac OS X any longer.)

Comment 63

[Jan de Mooij [:jandem]]

Filed bug 1293592 to track this.