1125770 - Backtracking started crashing on kraken-stanford-crypto-aes

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

http://www.arewefastyet.com/#machine=28&view=single&suite=kraken&start=1421927078&end=1422097812

Arewefastyet doesn't report numbers on "backtracking" anymore since a subtest started to fail/crash. Tested manually this is test "kraken-stanford-crypto-aes" which segfaults.

On the given graph we have two times it started crashing:

First regression range:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7cea69231f77&tochange=920ae6ac243b
6d56dfa4e845	Brian Hackett -- Bug 934502 - Remove unnecessary pushedArgumentSlots, track argument slots explicitly in safepoints, r=jandem.

Range that fixed it again:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5cd5233505fc&tochange=bd9169b924a1
a92e6bed098d	Ryan VanderMeulen -- Backed out changeset 6d56dfa4e845 (bug 934502) for SM(ggc) failures.

Second regression range:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ef538bd60b1c&tochange=92582779bc79
629c8aac3ece	Brian Hackett — Bug 934502 - Remove unnecessary pushedArgumentSlots, track argument slots explicitly in safepoints, r=jandem.
4d669523c7dd	Brian Hackett -- Bug 1123011 - Box 'this' values when eval'ing strict scripts from a non-strict Ion script, r=jandem.

The revision are just educated guesses. I didn't bisect, but it seems to make sense.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

This is a calling conventions issue.  When an Ion frame calls another one and is constructing, after the callee finishes executing the caller will reload the value it pushed for |this| if the callee returned a primitive.  Bug 934502 changed JIT frame tracing so that the |this| slot is no longer required to hold a value, and might not be marked by a GC within the callee if it is not live there.

The attached patch restores the earlier convention of always marking the |this| value as part of the frame, so that the caller can always assume it is live (and holds a valid |this| value for the frame).  I added some logic to the backtracking allocator so that it can't group vregs spilling to the |this| slot with other vregs that don't; this will prevent the allocator from spilling those other vregs to the |this| slot.

Comment 2

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0767ae5882f9

Comment 3

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=5959773&repo=mozilla-inbound

Comment 4

[Brian Hackett [Laid off!]]

Attachment: patch v2

The first patch had a bug where |this| values for non-function Ion frames were not being marked.  Fixing this also fixes bug 1125840.

Comment 6

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05f7505159eb

Comment 7

[Hannes Verschore [:h4writer]]

Thanks. Arewefastyet reports numbers again for kraken-standford-crypto-aes (and shumway, which was also affected).

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/05f7505159eb