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.

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.

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();

[Tracking Requested - why for this release]:

Tracking for 42 because regression.

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?

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

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 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

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!

FF41+

(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.

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.