1659093 - [warp] Assertion failure: vreg(ins->getTemp(i)).type() != vreg(use).type() || (use->isFixedRegister() && ins->getTemp(i)->isFixed()), at jit/BacktrackingAllocator.cpp:709

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200813-f46205a42fae (debug build, run with --fuzzing-safe --ion-offthread-compile=off --warp --baseline-eager --ion-full-warmup-threshold=10):

(function(global) {
    global.assertDeepEq = (function() {})();
})(this);
function LoggingProxy(target) {
    var h55 = {};
    return new Proxy(target || [], h55);
}
LoggingProxy.from = Array.from;
LoggingProxy.from([3, 4, 5]);
var arr = [5, 6, 7];
assertDeepEq(Array.from(new LoggingProxy(arr)), [5, 6, 7]);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x585c54e1 in ?? ()
[...]
#25 0x57d54542 in ?? ()
#26 0x57a67add in _start ()
eax	0x5676782d	1450604589
ebx	0x592e14ec	1496192236
ecx	0x592e1f5c	1496194908
edx	0x0	0
esi	0xecdd4618	-321042920
edi	0x592e14ec	1496192236
ebp	0xffffa0e8	4294942952
esp	0xffff9f80	4294942592
eip	0x585c54e1	1482446049
=> 0x585c54e1:	movl   $0x2c5,0x0
   0x585c54eb:	call   0x57a8de97 <abort>

Reproduces on 32-bit only.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[André Bargull [:anba]]

Slightly minified:

function ArrayFrom(C, items) {
  var A = new C();
  var k = 0;
  for (var i = 0; i < items.length; ++i) {}
  A.length = k;
}

function LoggingProxy() {
  return new Proxy([], {});
}

ArrayFrom(LoggingProxy, [3, 4, 5]);
ArrayFrom(Array, [5, 6, 7]);

Comment 3

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200814094524-605c404fbd80.
The bug appears to have been introduced in the following build range:
> Start: 06e4f8547d78187f934074b2b4ff4d4d4d12efd5 (20200813154618)
> End: f46205a42faecda0e5a73bdcde0a8e1caa0126d9 (20200813213942)
> Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=06e4f8547d78187f934074b2b4ff4d4d4d12efd5&tochange=f46205a42faecda0e5a73bdcde0a8e1caa0126d9

Comment 4

[André Bargull [:anba]]

LProxySet has non-fixed, "atStart" registers, but also temporary registers.

Comment 5

[Jan de Mooij [:jandem]]

The assertion also affects jit-tests on 32-bit with Warp enabled so we're just missing test coverage there for now.

The assertion is unnecessary nowadays, with the backtracking allocator temps never alias inputs (even at-start uses). I'll add an assertion for that and clean this up a bit.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1659093 part 1 - Assert temps don't alias inputs, fix some comments. r?anba!

Temps cover both the input and output part of the instruction, so their ranges
always overlap with at-start uses (covering the input) and non-at-start uses
(covering both input and output). This means the allocated register must be
different from any uses.

Defs cover only the output part of the instruction so there at-start makes a
difference.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1659093 part 2 - Remove a now-unnecessary assertion. r?anba!

Given that temps never alias inputs as established in the previous patch, this
assertion isn't necessary. It's possible the LSRA allocator did things differently.

The assertion was also weird/wrong because it depends on LDefinition::Type, but
there are multiple Types that could still map to the same Register. It looks
like bug 1067610 broke this; before that refactoring we compared isFloatReg().

Depends on D87263

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1659093 part 3 - Require call instruction temps to be fixed registers. r?anba!

This prevents the potential issue described in the comment and most instructions
already did this anyway.

Depends on D87264

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1659093 part 4 - Remove fixed register uses that are no longer necessary. r?anba!

Depends on D87265

Comment 10

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

Set release status flags based on info from the regressing bug 1658882

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9e192acbc34
part 1 - Assert temps don't alias inputs, fix some comments. r=anba
https://hg.mozilla.org/integration/autoland/rev/bc3a6719c0f4
part 2 - Remove a now-unnecessary assertion. r=anba
https://hg.mozilla.org/integration/autoland/rev/ed4528417b1b
part 3 - Require call instruction temps to be fixed registers. r=anba
https://hg.mozilla.org/integration/autoland/rev/b0b25fca6075
part 4 - Remove fixed register uses that are no longer necessary. r=anba

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b9e192acbc34
https://hg.mozilla.org/mozilla-central/rev/bc3a6719c0f4
https://hg.mozilla.org/mozilla-central/rev/ed4528417b1b
https://hg.mozilla.org/mozilla-central/rev/b0b25fca6075

Comment 13

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200817214602-508a0cc2f6d4.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.