[regexp] Fix two more possible shape changes on fast path

[regexp] Fix two more possible shape changes on fast path

This CL fixes two more cases in which a regexp could unintentionally transition
to slow mode while on the fast path, leading to possible OOB accesses of
lastIndex.

In both cases, the fix is to re-check the shape and possibly bail to runtime.

BUG=chromium:708247, v8:6210
Review-Url: https://codereview.chromium.org/2803603005
Cr-Commit-Position: refs/heads/master@{#44451}
Committed: https://chromium.googlesource.com/v8/v8/+/1ccf6c0943e328183cb670e14d718b7461cbcb93

[jgruber, 2017-04-06 09:50:32]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 09:50:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2017-04-06 09:54:38]

Description was changed from

==========
[regexp] Fix two more possible shape changes on fast path

This CL fixes two more cases in which a regexp could unintentionally transition
to slow mode while on the fast path, leading to possible OOB accesses of
lastIndex.

In both cases, the fix is to re-check the shape and possibly bail to runtime.

BUG=chromium:708247
==========

to

==========
[regexp] Fix two more possible shape changes on fast path

This CL fixes two more cases in which a regexp could unintentionally transition
to slow mode while on the fast path, leading to possible OOB accesses of
lastIndex.

In both cases, the fix is to re-check the shape and possibly bail to runtime.

BUG=chromium:708247,v8:6210
==========

[commit-bot: I haz the power, 2017-04-06 10:47:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 10:47:05]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-04-06 14:24:54]

jgruber@chromium.org changed reviewers:
+ cbruni@chromium.org

[Camillo Bruni, 2017-04-06 14:54:15]

LGTM with one nit.

https://codereview.chromium.org/2803603005/diff/1/src/builtins/builtins-regex...
File src/builtins/builtins-regexp-gen.cc (right):

https://codereview.chromium.org/2803603005/diff/1/src/builtins/builtins-regex...
src/builtins/builtins-regexp-gen.cc:2310: Node* const limit = ToUint32(context,
maybe_limit);
future-nit: something like ToUint32(..., &was_fast) would avoid the subsequent
more expensive BranchIfFastRegExp checks. Basically you can skip if you get a
Number as input.

https://codereview.chromium.org/2803603005/diff/1/src/builtins/builtins-regex...
src/builtins/builtins-regexp-gen.cc:2317: BranchIfFastRegExp(context, regexp,
LoadMap(regexp), &next, &runtime);
site-note: You'd probably be better off with a GotoIfSlowRegexp(..., &runtime)
since the fast-path code is mostly inline.

https://codereview.chromium.org/2803603005/diff/1/src/builtins/builtins-regex...
src/builtins/builtins-regexp-gen.cc:2345: CSA_ASSERT(this,
IsHeapNumberMap(LoadReceiverMap(limit)));
nit: CSA_ASSERT(this, IsNumber(limit));

[jgruber, 2017-04-06 15:18:35]

The CQ bit was checked by jgruber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 15:18:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jgruber, 2017-04-06 15:21:12]

https://codereview.chromium.org/2803603005/diff/1/src/builtins/builtins-regex...
File src/builtins/builtins-regexp-gen.cc (right):

https://codereview.chromium.org/2803603005/diff/1/src/builtins/builtins-regex...
src/builtins/builtins-regexp-gen.cc:2310: Node* const limit = ToUint32(context,
maybe_limit);
On 2017/04/06 14:54:15, Camillo Bruni wrote:
> future-nit: something like ToUint32(..., &was_fast) would avoid the subsequent
> more expensive BranchIfFastRegExp checks. Basically you can skip if you get a
> Number as input.

Sounds reasonable. I want to keep this CL small and noninvasive though since it
will need to be backmerged. I added a fast path for non-negative smis above.

https://codereview.chromium.org/2803603005/diff/1/src/builtins/builtins-regex...
src/builtins/builtins-regexp-gen.cc:2317: BranchIfFastRegExp(context, regexp,
LoadMap(regexp), &next, &runtime);
On 2017/04/06 14:54:15, Camillo Bruni wrote:
> site-note: You'd probably be better off with a GotoIfSlowRegexp(..., &runtime)
> since the fast-path code is mostly inline.

Acknowledged.

https://codereview.chromium.org/2803603005/diff/1/src/builtins/builtins-regex...
src/builtins/builtins-regexp-gen.cc:2345: CSA_ASSERT(this,
IsHeapNumberMap(LoadReceiverMap(limit)));
On 2017/04/06 14:54:15, Camillo Bruni wrote:
> nit: CSA_ASSERT(this, IsNumber(limit));

Can do in a follow-up (don't want to touch c-s-a.{h,cc} in this one).

[commit-bot: I haz the power, 2017-04-06 15:45:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-06 15:45:39]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-04-06 15:50:01]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2017-04-06 15:50:01]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/2803603005/#ps20001
(title: "Add fast-path before ToUint32")

[commit-bot: I haz the power, 2017-04-06 15:50:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 15:52:22]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491493801011010,
"parent_rev": "9d7354f9f3061b2094a1dfe085e340d93e51477e", "commit_rev":
"1ccf6c0943e328183cb670e14d718b7461cbcb93"}

[commit-bot: I haz the power, 2017-04-06 15:52:26]

Description was changed from

==========
[regexp] Fix two more possible shape changes on fast path

This CL fixes two more cases in which a regexp could unintentionally transition
to slow mode while on the fast path, leading to possible OOB accesses of
lastIndex.

In both cases, the fix is to re-check the shape and possibly bail to runtime.

BUG=chromium:708247,v8:6210
==========

to

==========
[regexp] Fix two more possible shape changes on fast path

This CL fixes two more cases in which a regexp could unintentionally transition
to slow mode while on the fast path, leading to possible OOB accesses of
lastIndex.

In both cases, the fix is to re-check the shape and possibly bail to runtime.

BUG=chromium:708247,v8:6210

Review-Url: https://codereview.chromium.org/2803603005
Cr-Commit-Position: refs/heads/master@{#44451}
Committed:
https://chromium.googlesource.com/v8/v8/+/1ccf6c0943e328183cb670e14d718b7461c...
==========

[commit-bot: I haz the power, 2017-04-06 15:52:27]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/1ccf6c0943e328183cb670e14d718b7461c...