Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2431223010:
[builtins] fix load of surrogate pair for BE platform (Closed)
|
Created:
4 years, 2 months ago by JaideepBajwa Modified:
4 years, 1 month ago CC:
v8-reviews_googlegroups.com, JoranSiu, john.yan, michael_dawson, MTBrandyberry Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[builtins] fix load of surrogate pair for BE platform
Swaping the order when loading surrogate pairs on big endian
platform. This fixes testcase string-iterator.js on big
endian.
BUG=
R=bmeurer@chromium.org, mstarzinger@chromium.org, jkummerow@chromium.org
Committed: https://crrev.com/f2d278a95733d05c553d01adcc54653789f06689
Cr-Commit-Position: refs/heads/master@{#40535}
Patch Set 1 #
Total comments: 2
Patch Set 2 : updated js-builtin-reducer #
Total comments: 2
Patch Set 3 : addressed comment #
Messages
Total messages: 27 (15 generated)
ptal
caitp@igalia.com changed reviewers: + caitp@igalia.com
https://codereview.chromium.org/2431223010/diff/1/src/builtins/builtins-strin... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2431223010/diff/1/src/builtins/builtins-strin... src/builtins/builtins-string.cc:1384: #if !V8_TARGET_LITTLE_ENDIAN Nice --- I originally tried to do something like this, but didn't have a way to test it to verify it was working. LGTM if this is working on the BE ports. Note: I think you also need a similar change in js-builtin-reducer.cc
https://codereview.chromium.org/2431223010/diff/1/src/builtins/builtins-strin... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2431223010/diff/1/src/builtins/builtins-strin... src/builtins/builtins-string.cc:1384: #if !V8_TARGET_LITTLE_ENDIAN On 2016/10/21 20:55:28, caitp wrote: > Nice --- I originally tried to do something like this, but didn't have a way to > test it to verify it was working. LGTM if this is working on the BE ports. > > Note: I think you also need a similar change in js-builtin-reducer.cc around https://cs.chromium.org/chromium/src/v8/src/compiler/js-builtin-reducer.cc?q=... afaik?
On 2016/10/21 21:07:23, caitp wrote: > https://codereview.chromium.org/2431223010/diff/1/src/builtins/builtins-strin... > File src/builtins/builtins-string.cc (right): > > https://codereview.chromium.org/2431223010/diff/1/src/builtins/builtins-strin... > src/builtins/builtins-string.cc:1384: #if !V8_TARGET_LITTLE_ENDIAN > On 2016/10/21 20:55:28, caitp wrote: > > Nice --- I originally tried to do something like this, but didn't have a way > to > > test it to verify it was working. LGTM if this is working on the BE ports. > > > > Note: I think you also need a similar change in js-builtin-reducer.cc > > around > https://cs.chromium.org/chromium/src/v8/src/compiler/js-builtin-reducer.cc?q=... > afaik? Thank you, I've updated js-builtin-reducer.cc
The CQ bit was checked by bjaideep@ca.ibm.com 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: This issue passed the CQ dry run.
The CQ bit was checked by bjaideep@ca.ibm.com
The patchset sent to the CQ was uploaded after l-g-t-m from caitp@igalia.com Link to the patchset: https://codereview.chromium.org/2431223010/#ps20001 (title: "updated js-builtin-reducer")
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
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/27093)
https://codereview.chromium.org/2431223010/diff/20001/src/builtins/builtins-s... File src/builtins/builtins-string.cc (right): https://codereview.chromium.org/2431223010/diff/20001/src/builtins/builtins-s... src/builtins/builtins-string.cc:1384: #if !V8_TARGET_LITTLE_ENDIAN one more nit, I would personally prefer not negating the condition (IE do `#if V8_TARGET_BIG_ENDIAN`, or `#if V8_TARGET_LITTLE_ENDIAN` instead. Easier to read :) https://codereview.chromium.org/2431223010/diff/20001/src/compiler/js-builtin... File src/compiler/js-builtin-reducer.cc (right): https://codereview.chromium.org/2431223010/diff/20001/src/compiler/js-builtin... src/compiler/js-builtin-reducer.cc:1145: #if !V8_TARGET_LITTLE_ENDIAN ditto here
The CQ bit was checked by bjaideep@ca.ibm.com 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: This issue passed the CQ dry run.
LGTM, thanks!
The CQ bit was checked by bjaideep@ca.ibm.com
The patchset sent to the CQ was uploaded after l-g-t-m from caitp@igalia.com Link to the patchset: https://codereview.chromium.org/2431223010/#ps40001 (title: "addressed comment")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
On 2016/10/24 14:28:16, commit-bot: I haz the power wrote: > Committed patchset #3 (id:40001) 👍
Message was sent while issue was closed.
Description was changed from ========== [builtins] fix load of surrogate pair for BE platform Swaping the order when loading surrogate pairs on big endian platform. This fixes testcase string-iterator.js on big endian. BUG= R=bmeurer@chromium.org, mstarzinger@chromium.org, jkummerow@chromium.org ========== to ========== [builtins] fix load of surrogate pair for BE platform Swaping the order when loading surrogate pairs on big endian platform. This fixes testcase string-iterator.js on big endian. BUG= R=bmeurer@chromium.org, mstarzinger@chromium.org, jkummerow@chromium.org Committed: https://crrev.com/f2d278a95733d05c553d01adcc54653789f06689 Cr-Commit-Position: refs/heads/master@{#40535} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f2d278a95733d05c553d01adcc54653789f06689 Cr-Commit-Position: refs/heads/master@{#40535} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
