[regexp] Use consistent map checks for fast paths

[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable,
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339, v8:5434, v8:5123
Committed: https://crrev.com/eb10dc4c91eb34bad40a5cd33be27a13885f1736
Cr-Commit-Position: refs/heads/master@{#40501}

[jgruber, 2016-10-20 11:02:35]

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

[commit-bot: I haz the power, 2016-10-20 11:02:38]

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

[jgruber, 2016-10-20 11:04:36]

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

[jgruber, 2016-10-20 11:04:37]

+igor FYI since I'm adding another TODO for you.

[commit-bot: I haz the power, 2016-10-20 11:28:28]

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

[commit-bot: I haz the power, 2016-10-20 11:28:30]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/14941)
  v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)

[jgruber, 2016-10-20 12:39:24]

This exposes an issue in RE.proto.exec: 

* In some tests, e.g. es6/regexp-flags, the regexp instance is modified.
* That results in taking the slow 'subclass' path in RegExpReplace.
* The slow path properly reads the 'global' property and sees that it is true.
* exec directly reads JSRegExp's flags value and thinks it is false.
* exec never advances lastIndex, and RegExpReplace loops forever.

This was already present in the original JS code, but hasn't triggered so far
because our fast-path check in replace was less strict.

A partial solution would be to make exec go through GetProperty if regexp is not
an unmodified JSRegExp. This would probably help with current test failures.

But that still would not fix accesses to e.g. flags in RegExpImpl::Exec /
RegExpExecStub.

[jgruber, 2016-10-20 12:44:53]

On 2016/10/20 12:39:24, jgruber wrote:
> This exposes an issue in RE.proto.exec: 
> 
> * In some tests, e.g. es6/regexp-flags, the regexp instance is modified.
> * That results in taking the slow 'subclass' path in RegExpReplace.
> * The slow path properly reads the 'global' property and sees that it is true.
> * exec directly reads JSRegExp's flags value and thinks it is false.
> * exec never advances lastIndex, and RegExpReplace loops forever.
> 
> This was already present in the original JS code, but hasn't triggered so far
> because our fast-path check in replace was less strict.
> 
> A partial solution would be to make exec go through GetProperty if regexp is
not
> an unmodified JSRegExp. This would probably help with current test failures.
> 
> But that still would not fix accesses to e.g. flags in RegExpImpl::Exec /
> RegExpExecStub.

Actually, RegExpExecStub has no accesses and we could fix the ones in 
RegExpImpl::Exec. We'd have to be conservative though and would choose the slow
path whenever the regexp instance is modified.

[jgruber, 2016-10-20 13:37:11]

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

[commit-bot: I haz the power, 2016-10-20 13:37:20]

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

[jgruber, 2016-10-20 13:40:18]

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

[commit-bot: I haz the power, 2016-10-20 13:40:20]

Dry run: 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, 2016-10-20 13:56:31]

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

[commit-bot: I haz the power, 2016-10-20 13:56:31]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11212)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[jgruber, 2016-10-20 14:10:02]

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

[commit-bot: I haz the power, 2016-10-20 14:10:07]

Dry run: 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, 2016-10-20 14:29:57]

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

[commit-bot: I haz the power, 2016-10-20 14:29:58]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11220)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[jgruber, 2016-10-21 06:29:33]

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

[commit-bot: I haz the power, 2016-10-21 06:29:37]

Dry run: 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, 2016-10-21 06:54:22]

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

[commit-bot: I haz the power, 2016-10-21 06:54:23]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/15014)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[Yang, 2016-10-21 06:54:46]

On 2016/10/20 12:44:53, jgruber wrote:
> On 2016/10/20 12:39:24, jgruber wrote:
> > This exposes an issue in RE.proto.exec: 
> > 
> > * In some tests, e.g. es6/regexp-flags, the regexp instance is modified.
> > * That results in taking the slow 'subclass' path in RegExpReplace.
> > * The slow path properly reads the 'global' property and sees that it is
true.
> > * exec directly reads JSRegExp's flags value and thinks it is false.
> > * exec never advances lastIndex, and RegExpReplace loops forever.
> > 
> > This was already present in the original JS code, but hasn't triggered so
far
> > because our fast-path check in replace was less strict.
> > 
> > A partial solution would be to make exec go through GetProperty if regexp is
> not
> > an unmodified JSRegExp. This would probably help with current test failures.
> > 
> > But that still would not fix accesses to e.g. flags in RegExpImpl::Exec /
> > RegExpExecStub.
> 
> Actually, RegExpExecStub has no accesses and we could fix the ones in 
> RegExpImpl::Exec. We'd have to be conservative though and would choose the
slow
> path whenever the regexp instance is modified.

LGTM once test failures have been fixed.

[jgruber, 2016-10-21 07:58:11]

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

[commit-bot: I haz the power, 2016-10-21 07:58:23]

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

[jgruber, 2016-10-21 07:59:43]

Description was changed from

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in RegExpReplace since checking maps (unlike the
previous check of RegExp.prototype.exec) is not observable, so we can
perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434
==========

to

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable, 
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434
==========

[commit-bot: I haz the power, 2016-10-21 08:01:31]

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

[commit-bot: I haz the power, 2016-10-21 08:01:32]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11267)

[jgruber, 2016-10-21 08:04:27]

On 2016/10/20 12:44:53, jgruber wrote:
> On 2016/10/20 12:39:24, jgruber wrote:
> > This exposes an issue in RE.proto.exec: 
> > 
> > * In some tests, e.g. es6/regexp-flags, the regexp instance is modified.
> > * That results in taking the slow 'subclass' path in RegExpReplace.
> > * The slow path properly reads the 'global' property and sees that it is
true.
> > * exec directly reads JSRegExp's flags value and thinks it is false.
> > * exec never advances lastIndex, and RegExpReplace loops forever.
> > 
> > This was already present in the original JS code, but hasn't triggered so
far
> > because our fast-path check in replace was less strict.
> > 
> > A partial solution would be to make exec go through GetProperty if regexp is
> not
> > an unmodified JSRegExp. This would probably help with current test failures.
> > 
> > But that still would not fix accesses to e.g. flags in RegExpImpl::Exec /
> > RegExpExecStub.
> 
> Actually, RegExpExecStub has no accesses and we could fix the ones in 
> RegExpImpl::Exec. We'd have to be conservative though and would choose the
slow
> path whenever the regexp instance is modified.

Conclusion after more investigation: in the current spec draft 
(https://tc39.github.io/ecma262/#sec-regexpbuiltinexec) RegExpBuiltinExec
now accesses OriginalFlags (instead of calling GetProperty for global and
sticky).

That means our exec implementation is correct, and the infinite loop in
regexp-flags.js
is spec-compliant.

Commented that test case for now, and pinged littledan@ about spec issues.

[jgruber, 2016-10-21 08:05:17]

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

[commit-bot: I haz the power, 2016-10-21 08:05:21]

Dry run: 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, 2016-10-21 08:10:54]

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

[commit-bot: I haz the power, 2016-10-21 08:10:55]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/16623)

[jgruber, 2016-10-21 08:47:52]

Description was changed from

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable, 
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434
==========

to

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable, 
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434,v8:5123
==========

[jgruber, 2016-10-21 08:53:41]

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

[commit-bot: I haz the power, 2016-10-21 08:53:53]

Dry run: 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, 2016-10-21 09:01:23]

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

[commit-bot: I haz the power, 2016-10-21 09:01:24]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/15028)

[jgruber, 2016-10-21 09:30:31]

The CQ bit was checked by jgruber@chromium.org

[jgruber, 2016-10-21 09:30:31]

The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2434983002/#ps120001
(title: "Unmark coerce-* tests as failing")

[commit-bot: I haz the power, 2016-10-21 09:30:42]

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, 2016-10-21 09:58:12]

Description was changed from

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable, 
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434,v8:5123
==========

to

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable, 
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434,v8:5123
==========

[commit-bot: I haz the power, 2016-10-21 09:58:13]

Committed patchset #7 (id:120001)

[Michael Achenbach, 2016-10-21 11:14:43]

A revert of this CL (patchset #7 id:120001) has been created in
https://chromiumcodereview.appspot.com/2438283002/ by machenbach@chromium.org.

The reason for reverting is:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui....

[jgruber, 2016-10-21 11:53:54]

Description was changed from

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable, 
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434,v8:5123
==========

to

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable, 
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434,v8:5123
==========

[jgruber, 2016-10-21 11:54:37]

On 2016/10/21 11:14:43, machenbach (slow) wrote:
> A revert of this CL (patchset #7 id:120001) has been created in
> https://chromiumcodereview.appspot.com/2438283002/ by
mailto:machenbach@chromium.org.
> 
> The reason for reverting is:
>
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui....

Relanding - I can't see a timing difference in the extra-transition locally.

[jgruber, 2016-10-21 11:54:46]

The CQ bit was checked by jgruber@chromium.org

[commit-bot: I haz the power, 2016-10-21 11:54:51]

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, 2016-10-21 11:58:32]

Description was changed from

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable, 
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434,v8:5123
==========

to

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable, 
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434,v8:5123
==========

[commit-bot: I haz the power, 2016-10-21 11:58:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-17 22:09:23]

Description was changed from

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable, 
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434,v8:5123
==========

to

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable,
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434,v8:5123

Committed: https://crrev.com/bac992a11440b7972fe40720d056fc3398c08495
Cr-Commit-Position: refs/heads/master@{#40495}
==========

[commit-bot: I haz the power, 2016-11-17 22:09:24]

Patchset 7 (id:??) landed as
https://crrev.com/bac992a11440b7972fe40720d056fc3398c08495
Cr-Commit-Position: refs/heads/master@{#40495}

[commit-bot: I haz the power, 2016-11-17 22:09:39]

Description was changed from

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable,
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434,v8:5123

Committed: https://crrev.com/bac992a11440b7972fe40720d056fc3398c08495
Cr-Commit-Position: refs/heads/master@{#40495}
==========

to

==========
[regexp] Use consistent map checks for fast paths

These map checks were implemented for TF code already. This CL makes
sure that parts implemented in C++ follow the same logic, which is:

An object is an unmodified regexp if:
1) it's a receiver,
2) its map is the initial regexp map,
3) its prototype is a receiver,
4) and its prototype's map is the initial prototype's initial map.

We can now be smarter in @@replace and @@split since checking maps
(unlike the previous check of RegExp.prototype.exec) is not observable,
so we can perform fast-path checks at a time of our choosing.

BUG=v8:5339,v8:5434,v8:5123

Committed: https://crrev.com/eb10dc4c91eb34bad40a5cd33be27a13885f1736
Cr-Commit-Position: refs/heads/master@{#40501}
==========

[commit-bot: I haz the power, 2016-11-17 22:09:41]

Patchset 7 (id:??) landed as
https://crrev.com/eb10dc4c91eb34bad40a5cd33be27a13885f1736
Cr-Commit-Position: refs/heads/master@{#40501}