Add array bound checks to code generated for SwapElements. This fixes a bug t...

Add array bound checks to code generated for SwapElements. This fixes a bug that lead to a segfault when an array was modified while it was sorted.


Committed: http://code.google.com/p/v8/source/detail?r=6020

[Lasse Reichstein, 2010-12-15 08:34:55]

LGTM with comments.

http://codereview.chromium.org/5686006/diff/6001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/5686006/diff/6001/src/arm/codegen-arm.cc#newco...
src/arm/codegen-arm.cc:5599: __ cmp(tmp2, index2);
You might consider making the second compare conditional and only having one
branch. I.e.:
 __ cmp(tmp2, index1);
 __ cmp(tmp2, index2, gt);
 deferred->Branch(le);
I doubt it's faster, but it's a little smaller.

http://codereview.chromium.org/5686006/diff/6001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/5686006/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3116: __ j(less_equal, &slow_case);
I just noticed that there is no test for negative indices, which ofcourse
shouldn't be able to happen ... but trust is overrated.

Use below_equal here to do unsigned comparison (and trust that an array length
can never be negative :).
(You could also change the test above to test against (kSmiTagMask | <signbit
constant>), but that would probably use more bytes whereas changing the
condition is free).

Goes for all platforms.

[Karl Klose, 2010-12-15 09:11:45]

http://codereview.chromium.org/5686006/diff/6001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/5686006/diff/6001/src/arm/codegen-arm.cc#newco...
src/arm/codegen-arm.cc:5599: __ cmp(tmp2, index2);
On 2010/12/15 08:34:55, Lasse Reichstein wrote:
> You might consider making the second compare conditional and only having one
> branch. I.e.:
>  __ cmp(tmp2, index1);
>  __ cmp(tmp2, index2, gt);
>  deferred->Branch(le);
> I doubt it's faster, but it's a little smaller.

Done.

http://codereview.chromium.org/5686006/diff/6001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/5686006/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3116: __ j(less_equal, &slow_case);
On 2010/12/15 08:34:55, Lasse Reichstein wrote:
> I just noticed that there is no test for negative indices, which ofcourse
> shouldn't be able to happen ... but trust is overrated.
> 
> Use below_equal here to do unsigned comparison (and trust that an array length
> can never be negative :).
> (You could also change the test above to test against (kSmiTagMask | <signbit
> constant>), but that would probably use more bytes whereas changing the
> condition is free).
> 
> Goes for all platforms.

Done.