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

RESOLVED FIXED in Firefox 41

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: bhackett)

Tracking

({regression})

unspecified
mozilla42
regression
Points:
---

Firefox Tracking Flags

(firefox40 unaffected, firefox41+ fixed, firefox42+ fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

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

Comment 1

3 years ago
Created attachment 8640845 [details]
m1.json.gz
(Reporter)

Comment 2

3 years ago
Created attachment 8640846 [details]
m2.json.gz
(Reporter)

Comment 3

3 years ago
BTW, about:memory will accept those files in both gzipped and non-gzipped form, if that's any help.
Ugh, seems bad. I'll take a look.
Flags: needinfo?(jdemooij)
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();
Flags: needinfo?(jdemooij) → needinfo?(bhackett1024)
[Tracking Requested - why for this release]:
Blocks: 1162199
status-firefox40: --- → unaffected
status-firefox41: --- → affected
status-firefox42: --- → affected
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
Keywords: regression

Comment 7

3 years ago
Tracking for 42 because regression.
tracking-firefox42: ? → +
(Reporter)

Comment 8

3 years ago
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... :)
(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.
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?
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"
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?
(Assignee)

Comment 13

3 years ago
Created attachment 8642573 [details] [diff] [review]
patch

Thanks for tracking this down!  Yes, MStoreUnboxedScalar can't assume that it is always correct to truncate integer inputs.
Assignee: nobody → bhackett1024
Flags: needinfo?(bhackett1024)
Attachment #8642573 - Flags: review?(jdemooij)

Updated

3 years ago
Summary: Different results in about:memory behaviour with Ion enabled → Ion assumes unboxed-object slots are truncated, causing incorrect output on about:memory
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.
Attachment #8642573 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 16

3 years ago
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
Attachment #8642573 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/dd7436fa536c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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!
Flags: needinfo?(bhackett1024)
FF41+
tracking-firefox41: ? → +
(Assignee)

Comment 20

3 years ago
(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.
Flags: needinfo?(bhackett1024)
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 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.
Attachment #8642573 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.