1189137 - Ion assumes unboxed-object slots are truncated, causing incorrect output on about:memory

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicholas Nethercote [inactive]]

I was puzzled by some unexpected results in about:memory's output. It turns out to be a bug in IonMonkey.

Steps to reproduce:

- Start Firefox.
- Open about:memory.
- Click on the "verbose" checkbox and then click on the "Load and diff..." button.
- Select m1.json.gz and then m2.json.gz.
- Output is displayed.

Without IonMonkey enabled, I get the expected output:

> -2,805,641,216 B (100.0%) -- address-space
> ├──-2,769,719,296 B (98.72%) ── free [520]
> └─────-35,921,920 B (01.28%) -- reserved
>       ├──-35,880,960 B (01.28%) ── private [908]
>       └──────-40,960 B (00.00%) ── mapped [5]

With IonMonkey enabled, I get incorrect output:

> 1,489,326,080 B (100.0%) -- address-space
> ├──1,525,248,000 B (102.41%) ── free [520]
> └────-35,921,920 B (-2.41%) -- reserved
>      ├──-35,880,960 B (-2.41%) ── private [908]
>      └──────-40,960 B (-0.00%) ── mapped [5]

Note that the difference between the correct and bogus values of "free" is 
exactly 2^32, which is what made terrence suggest that it might be a JIT bug in
the first place. (And likewise for "address-space", which is derived from the 
"free" value.)

I've already reduced the input files down as far as I easily could, but they still have 1000s of lines in them. And about:memory is implemented in just over 2,000 lines of JS (in toolkit/components/aboutmemory/content/aboutMemory.js). I could probably write a patch that strips out a lot of those 2,000 lines, but this code isn't easy to get into a form that can be used by the shell.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: m1.json.gz

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: m2.json.gz

Comment 3

[Nicholas Nethercote [inactive]]

BTW, about:memory will accept those files in both gzipped and non-gzipped form, if that's any help.

Comment 4

[Jan de Mooij [:jandem]]

Ugh, seems bad. I'll take a look.

Comment 5

[Jan de Mooij [:jandem]]

Minimal shell testcase below.

$ js --no-baseline test.js
$ js --no-threads test.js
test.js:12:2 Error: Assertion failed: got -319658654, expected 3975308642

We have an MAdd without an overflow check... I think the problem is in MStoreUnboxedScalar::operandTruncateKind, it assumes the value will be truncated anyway so we don't need the overflow check. That's fine for typed arrays, but not for unboxed object slots and maybe other places that use MStoreUnboxedScalar.

var arr = [];
for (var i=0; i<2000; i++)
    arr.push({amount: i > 1900 ? 1987654321 : 1});

function f() {
    for (var i=0; i<arr.length; i++) {
	arr[i].amount += 1987654321;
	assertEq(arr[i].amount, i > 1900 ? 3975308642 : 1987654322);
    }
}
f();

Comment 6

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]:

Comment 7

[Kate Glazko]

Tracking for 42 because regression.

Comment 8

[Nicholas Nethercote [inactive]]

Fantastic work, jandem! Thank you.

How did you come up with the reduced test case? My guess is that you squinted a bit and then wrote it by hand... :)

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> Fantastic work, jandem! Thank you.

No problem, thanks for the report. Glad we caught this one.

> How did you come up with the reduced test case? My guess is that you
> squinted a bit and then wrote it by hand... :)

I added some code to Ion's CheckScript function, something like:

  uint32_t min = atoi(getenv("ION_MIN"));
  uint32_t max = atoi(getenv("ION_MAX"));
  return min <= script->lineno() && script->lineno() <= max;

Then I could set these environment variables to bisect based on the script's line number. This lead me to line 767:

  merge: function(aJr) {
    this.assertCompatible(aJr.kind, aJr.units);
    this._amount += aJr.amount;
    this._nMerged++;
  },

Fortunately a tiny function and the problem was pretty obvious when I looked at the generated code.

Comment 10

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

Using the testcase in comment 5:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/322487136b28
user:        Brian Hackett
date:        Sun May 17 20:12:14 2015 -0600
summary:     Bug 1162199 - Use unboxed objects by default, r=jandem.

I guess bug 1162199 probably exposed the bug?

Comment 11

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

For comment 10, I used the following command from the funfuzz GitHub repository:

funfuzz/autobisect-js/autoBisect.py -p "--no-threads --ion-eager 1189137.js" -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -o "Assertion failed"

Comment 12

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

funfuzz/autobisect-js/autoBisect.py -s 7820fd141998 -e 'parents(322487136b28)' -p "--no-threads --ion-eager --unboxed-objects 1189137.js" -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -o "Assertion failed"

Thanks to Jesse, who suggested going back further than that changeset by specifying conditions for --unboxed-objects, with this flag being introduced in m-c rev 7820fd141998 and removed in m-c rev 322487136b28.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/6470d649e1bb
user:        Brian Hackett
date:        Sun Mar 01 16:31:41 2015 -0600
summary:     Bug 1135423 - Use unboxed objects for object literals where possible, clean up object literal creation and property initialization code, r=jandem.

Perhaps bug 1135423 is a more likely regressor?

Comment 13

[Brian Hackett [Laid off!]]

Attachment: patch

Thanks for tracking this down!  Yes, MStoreUnboxedScalar can't assume that it is always correct to truncate integer inputs.

Comment 14

[Jan de Mooij [:jandem]]

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

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

::: js/src/jit/MIR.h
@@ +9662,5 @@
>  
>      static MStoreUnboxedScalar* New(TempAllocator& alloc,
>                                      MDefinition* elements, MDefinition* index,
>                                      MDefinition* value, Scalar::Type storageType,
> +                                    bool truncateInput = true,

Hm I'd prefer making this a non-default argument, to avoid a potential footgun, and it seems most/all callers already pass it explicitly.

::: js/src/jit/RangeAnalysis.cpp
@@ +2683,5 @@
>  MDefinition::TruncateKind
>  MStoreUnboxedScalar::operandTruncateKind(size_t index) const
>  {
> +    // Some receiver objects, such as typed arrays, will truncate out of range integer inputs.
> +    return truncateInput() && index == 2 && isIntegerWrite() ? Truncate : NoTruncate;

Pre-existing, but can you parenthesize the ?: condition? It's hard to read like this and I had to look at a precedence table.

Comment 15

[Pulsebot]

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

Comment 16

[Brian Hackett [Laid off!]]

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

Approval Request Comment
[Feature/regressing bug #]: bug 1162199
[User impact if declined]: potential incorrect behavior
[Describe test coverage new/current, TreeHerder]: new test
[Risks and why]: low

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Brian, aside from the test that was added, did you get a chance to do any verification locally using IonMonkey add-on to ensure there were no unexpected issues? That would give me more confidence uplifting the patch to Aurora. Thanks!

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

FF41+

Comment 20

[Brian Hackett [Laid off!]]

(In reply to Ritu Kothari (:ritu) from comment #18)
> Brian, aside from the test that was added, did you get a chance to do any
> verification locally using IonMonkey add-on to ensure there were no
> unexpected issues? That would give me more confidence uplifting the patch to
> Aurora. Thanks!

I don't know what you're asking.  IonMonkey isn't an addon, it's used throughout the browser so is being extensively tested by nightly users.  This is a simple patch that adds a new restriction to an existing optimization.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Sorry about the addon typo bit. What I was trying to ask was whether any additional testing is needed here or will the test case added suffice. In any case, I will let this one bake another day or two on m-c before uplifting to Aurora to be sure.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

[Triage Comment]

Fix has been in Nightly for a few days. Should be safe to uplift to Beta41.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5329b78bfdb3