Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2496043003:
[builtins] Fix pointer comparison in ToString builtin. (Closed)
|
Created:
4 years, 1 month ago by Michael Starzinger Modified:
4 years, 1 month ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[builtins] Fix pointer comparison in ToString builtin.
This fixes the bogus {Word32Equal} comparison in the ToString builtin
implementing Object.prototype.toString to be a pointer-size {WordEqual}
comparison instead. Comparing just the lower half-word is insufficient
on 64-bit architectures.
R=jgruber@chromium.org
TEST=mjsunit/regress/regress-crbug-664506
BUG=chromium:664506
Committed: https://crrev.com/79aee39f24d6753ce6fbf36eb09f5fb63614e0d8
Cr-Commit-Position: refs/heads/master@{#40963}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 18 (10 generated)
The CQ bit was checked by mstarzinger@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...
Description was changed from
==========
[builtins] Fix pointer comparison in ToString builtin.
This fixes the bogus {Word32Equal} comparions in the ToString builtin
implementing Object.prototype.toString to be a pointer-size {WordEqual}
comparison instead. Comparing just the lower half-word is insufficient
on 64-bit architectures.
R=jgruber@chromium.org
TEST=mjsunit/regress/regress-crbug-664506
BUG=chromium:664506
==========
to
==========
[builtins] Fix pointer comparison in ToString builtin.
This fixes the bogus {Word32Equal} comparison in the ToString builtin
implementing Object.prototype.toString to be a pointer-size {WordEqual}
comparison instead. Comparing just the lower half-word is insufficient
on 64-bit architectures.
R=jgruber@chromium.org
TEST=mjsunit/regress/regress-crbug-664506
BUG=chromium:664506
==========
jgruber@google.com changed reviewers: + jgruber@google.com
lgtm
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 mstarzinger@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
[builtins] Fix pointer comparison in ToString builtin.
This fixes the bogus {Word32Equal} comparison in the ToString builtin
implementing Object.prototype.toString to be a pointer-size {WordEqual}
comparison instead. Comparing just the lower half-word is insufficient
on 64-bit architectures.
R=jgruber@chromium.org
TEST=mjsunit/regress/regress-crbug-664506
BUG=chromium:664506
==========
to
==========
[builtins] Fix pointer comparison in ToString builtin.
This fixes the bogus {Word32Equal} comparison in the ToString builtin
implementing Object.prototype.toString to be a pointer-size {WordEqual}
comparison instead. Comparing just the lower half-word is insufficient
on 64-bit architectures.
R=jgruber@chromium.org
TEST=mjsunit/regress/regress-crbug-664506
BUG=chromium:664506
==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2496043003/diff/1/test/mjsunit/regress/regres... File test/mjsunit/regress/regress-crbug-664506.js (right): https://codereview.chromium.org/2496043003/diff/1/test/mjsunit/regress/regres... test/mjsunit/regress/regress-crbug-664506.js:5: // Flags: --expose-gc --predictable --random-seed=-1109634722 Hmm, I think the test suite always overrides random-seed. We'd need a new feature to use the Flags version if present.
Message was sent while issue was closed.
https://codereview.chromium.org/2496043003/diff/1/test/mjsunit/regress/regres... File test/mjsunit/regress/regress-crbug-664506.js (right): https://codereview.chromium.org/2496043003/diff/1/test/mjsunit/regress/regres... test/mjsunit/regress/regress-crbug-664506.js:5: // Flags: --expose-gc --predictable --random-seed=-1109634722 On 2016/11/14 13:40:10, machenbach (slow) wrote: > Hmm, I think the test suite always overrides random-seed. We'd need a new > feature to use the Flags version if present. Nope, I don't think that is true. The test suite adds a random-seed flag and then appends the flag from the comment. That means the fixed random-seed appearing here appears later in the command line and will win. Also I tried locally when I wrote this regression test and it reproduces reliably. IMHO we are fine. WDYT?
Message was sent while issue was closed.
On 2016/11/14 14:50:27, Michael Starzinger wrote: > https://codereview.chromium.org/2496043003/diff/1/test/mjsunit/regress/regres... > File test/mjsunit/regress/regress-crbug-664506.js (right): > > https://codereview.chromium.org/2496043003/diff/1/test/mjsunit/regress/regres... > test/mjsunit/regress/regress-crbug-664506.js:5: // Flags: --expose-gc > --predictable --random-seed=-1109634722 > On 2016/11/14 13:40:10, machenbach (slow) wrote: > > Hmm, I think the test suite always overrides random-seed. We'd need a new > > feature to use the Flags version if present. > > Nope, I don't think that is true. The test suite adds a random-seed flag and > then appends the flag from the comment. That means the fixed random-seed > appearing here appears later in the command line and will win. Also I tried > locally when I wrote this regression test and it reproduces reliably. IMHO we > are fine. WDYT? Right. All good then!
Message was sent while issue was closed.
Description was changed from
==========
[builtins] Fix pointer comparison in ToString builtin.
This fixes the bogus {Word32Equal} comparison in the ToString builtin
implementing Object.prototype.toString to be a pointer-size {WordEqual}
comparison instead. Comparing just the lower half-word is insufficient
on 64-bit architectures.
R=jgruber@chromium.org
TEST=mjsunit/regress/regress-crbug-664506
BUG=chromium:664506
==========
to
==========
[builtins] Fix pointer comparison in ToString builtin.
This fixes the bogus {Word32Equal} comparison in the ToString builtin
implementing Object.prototype.toString to be a pointer-size {WordEqual}
comparison instead. Comparing just the lower half-word is insufficient
on 64-bit architectures.
R=jgruber@chromium.org
TEST=mjsunit/regress/regress-crbug-664506
BUG=chromium:664506
Committed: https://crrev.com/79aee39f24d6753ce6fbf36eb09f5fb63614e0d8
Cr-Commit-Position: refs/heads/master@{#40963}
==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/79aee39f24d6753ce6fbf36eb09f5fb63614e0d8 Cr-Commit-Position: refs/heads/master@{#40963} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
