MIPS: Fix return-value from Array.push stub when pushing non-SMI value

MIPS: Fix return-value from Array.push stub when pushing non-SMI value

Load and update the arrays length in v0 to make sure the length gets
returned correctly when leaving the function.

Add new testcase.

TEST=mjsunit/array-push-non-smi-value

BUG=130022

[fs, 2013-08-27 12:23:41]

Fix for a MIPS-specific Array.push issue.

[Jakob Kummerow, 2013-08-27 13:58:36]

+Paul and Akos for the MIPS change.

The test case doesn't make much sense to me, for the reasons below, and also
because it passes just fine on current bleeding_edge.

https://codereview.chromium.org/23589002/diff/1/test/mjsunit/array-push-non-s...
File test/mjsunit/array-push-non-smi-value.js (right):

https://codereview.chromium.org/23589002/diff/1/test/mjsunit/array-push-non-s...
test/mjsunit/array-push-non-smi-value.js:31: (function() {
Why the outer closure?

https://codereview.chromium.org/23589002/diff/1/test/mjsunit/array-push-non-s...
test/mjsunit/array-push-non-smi-value.js:34:
%OptimizeFunctionOnNextCall(a.push);
The patch only affects the stub. Why optimize the function?

[Paul Lind, 2013-08-27 15:08:33]

This looks-good-to-me, a very nice fix. This is (unfortunately) a common error
pattern for MIPS port, though I thought we had caught all these in the past.

However, it would be nice to get the unit-test showing the failure in current
(un-patched) code, as Jakob mentioned.

[fs, 2013-08-27 16:16:12]

Rolled back the TC a few steps. Hope it's less puzzling now...

https://codereview.chromium.org/23589002/diff/1/test/mjsunit/array-push-non-s...
File test/mjsunit/array-push-non-smi-value.js (right):

https://codereview.chromium.org/23589002/diff/1/test/mjsunit/array-push-non-s...
test/mjsunit/array-push-non-smi-value.js:31: (function() {
On 2013/08/27 13:58:36, Jakob wrote:
> Why the outer closure?

Why indeed. Dropped.

https://codereview.chromium.org/23589002/diff/1/test/mjsunit/array-push-non-s...
test/mjsunit/array-push-non-smi-value.js:34:
%OptimizeFunctionOnNextCall(a.push);
On 2013/08/27 13:58:36, Jakob wrote:
> The patch only affects the stub. Why optimize the function?

I rolled back the TC a few steps (I suspect I tried to minimize it a bit too
aggressively...) I hope it makes more sense in that form. (I'm can't claim to be
entirely familiar with all details around stubs, so guidance towards a better TC
would be most appreciated.)

[Jakob Kummerow, 2013-08-27 16:52:53]

Test case now LGTM with comment.

https://codereview.chromium.org/23589002/diff/7001/test/mjsunit/array-push-no...
File test/mjsunit/array-push-non-smi-value.js (right):

https://codereview.chromium.org/23589002/diff/7001/test/mjsunit/array-push-no...
test/mjsunit/array-push-non-smi-value.js:28: // Flags: --allow-natives-syntax
Now you don't need this anymore.

[Paul Lind, 2013-08-28 05:21:46]

LGTM, I'll land this for you, and will remove the --allow-natives-syntax flag.

Thanks a lot for the fix!

[Jakob Kummerow, 2013-08-28 07:55:05]

On 2013/08/27 16:16:12, fs wrote:
> (I'm can't claim to be
> entirely familiar with all details around stubs, so guidance towards a better
TC
> would be most appreciated.)

Well, you're fixing a bug, so the primary point of the test is that it fails
without your patch (proving that there is, in fact, a bug) and passes with your
patch (proving that the patch fixes the bug). So the way to build such a test is
to take the unpatched binary and write a test that fails with it; then if
necessary iterate on it until you like it (trim it down, clean it up, whatever
-- or extend it to also cover related cases, depending on what the issue is),
always making sure that it still fails as expected for the bug(s) you're about
to fix.

Runtime function calls like %OptimizeFunctionOnNextCall (which are only allowed
when the --allow-natives-syntax flag is present) are tools that help recreate
specific circumstances in tests, if those are required to (reliably) trigger a
bug. There's no reason to use them if you don't need them.