Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2798933003:
[regexp] Properly handle HeapNumbers in AdvanceStringIndex (Closed)
|
Created:
3 years, 8 months ago by jgruber Modified:
3 years, 8 months ago Reviewers:
Camillo Bruni CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[regexp] Properly handle HeapNumbers in AdvanceStringIndex
This fixes behavior for HeapNumber {index} arguments passed to
AdvanceStringIndex.
Previously, we'd blindly treat {index} as a Smi. Passing a HeapNumber instead
would result in a Smi addition on the tagged HeapNumber pointer.
BUG=chromium:709015
Review-Url: https://codereview.chromium.org/2798933003
Cr-Commit-Position: refs/heads/master@{#44458}
Committed: https://chromium.googlesource.com/v8/v8/+/ed5496f3cd1b410e1632531799edddfad8afe4f6
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 13
Patch Set 3 : Address comments and rebase #Patch Set 4 : Address comments #
Messages
Total messages: 37 (24 generated)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jgruber@chromium.org changed reviewers: + cbruni@chromium.org
Description was changed from
==========
[regexp] Properly handle HeapNumbers in AdvanceStringIndex
This fixes behavior for HeapNumber {index} arguments passed to
AdvanceStringIndex.
Previously, we'd blindly treat {index} as a Smi. Passing a HeapNumber instead
would result in a Smi addition on the tagged HeapNumber pointer.
BUG=v8:6209,chromium:708247
==========
to
==========
[regexp] Properly handle HeapNumbers in AdvanceStringIndex
This fixes behavior for HeapNumber {index} arguments passed to
AdvanceStringIndex.
Previously, we'd blindly treat {index} as a Smi. Passing a HeapNumber instead
would result in a Smi addition on the tagged HeapNumber pointer.
BUG=chromium:709015
==========
https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... File src/builtins/builtins-regexp-gen.cc (right): https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp-gen.cc:1533: CSA_ASSERT(this, IsHeapNumberMap(LoadReceiverMap(index))); nit: add IsNumber() https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp-gen.cc:1534: if (is_fastpath) CSA_ASSERT(this, TaggedIsSmi(index)); This should probably be TaggedIsPositiveSmi(index) https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp-gen.cc:1545: Goto(&next); as discussed offline: probably not worth the effort here. Smi-range heap numbers should always be normalized, feel free to keep this. https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp-gen.cc:1567: CSA_ASSERT(this, TaggedIsSmi(index_plus_one)); TaggedIsPositiveSmi(index_plus_one) https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp-gen.cc:1581: Node* const lead = StringCharCodeAt(string, var_index.value()); Please add CSA_ASSERT(this, TaggedIsPositiveSmi(index)) in StringCharCodeAt https://codereview.chromium.org/2798933003/diff/20001/test/mjsunit/regress/re... File test/mjsunit/regress/regress-6209.js (right): https://codereview.chromium.org/2798933003/diff/20001/test/mjsunit/regress/re... test/mjsunit/regress/regress-6209.js:20: assertEquals([""], fake_re[Symbol.match]("abc")); I wonder whether ClusterFuzz would pick up the following expression quicker: RegExp.prototype[Symbol.match].call(fake_re, "abc") Maybe you can add that as a second assertEquals
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jgruber@chromium.org
The CQ bit was checked by jgruber@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
Thanks for the quick review. Landing this as-is and following up on comments in separate CLs. https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... File src/builtins/builtins-regexp-gen.cc (right): https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp-gen.cc:1533: CSA_ASSERT(this, IsHeapNumberMap(LoadReceiverMap(index))); On 2017/04/06 15:10:57, Camillo Bruni wrote: > nit: add IsNumber() Acknowledged - pushing to a follow-up to keep this CL small for backmerges. https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp-gen.cc:1534: if (is_fastpath) CSA_ASSERT(this, TaggedIsSmi(index)); On 2017/04/06 15:10:57, Camillo Bruni wrote: > This should probably be TaggedIsPositiveSmi(index) Done. https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp-gen.cc:1545: Goto(&next); On 2017/04/06 15:10:58, Camillo Bruni wrote: > as discussed offline: probably not worth the effort here. Smi-range heap numbers > should always be normalized, feel free to keep this. Done. I'll add a corresponding assert in a follow-up. https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp-gen.cc:1567: CSA_ASSERT(this, TaggedIsSmi(index_plus_one)); On 2017/04/06 15:10:57, Camillo Bruni wrote: > TaggedIsPositiveSmi(index_plus_one) Done. https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp-gen.cc:1574: Branch(TaggedIsSmi(index_plus_one), &if_isunicode, &out); Also changed this to TaggedIsPositiveSmi. https://codereview.chromium.org/2798933003/diff/20001/src/builtins/builtins-r... src/builtins/builtins-regexp-gen.cc:1581: Node* const lead = StringCharCodeAt(string, var_index.value()); On 2017/04/06 15:10:57, Camillo Bruni wrote: > Please add CSA_ASSERT(this, TaggedIsPositiveSmi(index)) in StringCharCodeAt Will do in a follow-up. https://codereview.chromium.org/2798933003/diff/20001/test/mjsunit/regress/re... File test/mjsunit/regress/regress-6209.js (right): https://codereview.chromium.org/2798933003/diff/20001/test/mjsunit/regress/re... test/mjsunit/regress/regress-6209.js:20: assertEquals([""], fake_re[Symbol.match]("abc")); On 2017/04/06 15:10:58, Camillo Bruni wrote: > I wonder whether ClusterFuzz would pick up the following expression quicker: > > RegExp.prototype[Symbol.match].call(fake_re, "abc") > > Maybe you can add that as a second assertEquals I replaced the old version with this. No real reason for the method to be stored on fake_re.
The CQ bit was checked by jgruber@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
LGMT and +1 on follow-up CLs
The CQ bit was checked by jgruber@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
LGTM dammit! ;)
The CQ bit was checked by jgruber@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491504084169480,
"parent_rev": "586bf1d88df6962843e064dbac6305c0b3565e11", "commit_rev":
"ed5496f3cd1b410e1632531799edddfad8afe4f6"}
Message was sent while issue was closed.
Description was changed from
==========
[regexp] Properly handle HeapNumbers in AdvanceStringIndex
This fixes behavior for HeapNumber {index} arguments passed to
AdvanceStringIndex.
Previously, we'd blindly treat {index} as a Smi. Passing a HeapNumber instead
would result in a Smi addition on the tagged HeapNumber pointer.
BUG=chromium:709015
==========
to
==========
[regexp] Properly handle HeapNumbers in AdvanceStringIndex
This fixes behavior for HeapNumber {index} arguments passed to
AdvanceStringIndex.
Previously, we'd blindly treat {index} as a Smi. Passing a HeapNumber instead
would result in a Smi addition on the tagged HeapNumber pointer.
BUG=chromium:709015
Review-Url: https://codereview.chromium.org/2798933003
Cr-Commit-Position: refs/heads/master@{#44458}
Committed:
https://chromium.googlesource.com/v8/v8/+/ed5496f3cd1b410e1632531799edddfad8a...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/ed5496f3cd1b410e1632531799edddfad8a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
