Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2518533002:
Fix big endian support for MIPS (Closed)
|
Created:
4 years, 1 month ago by miran.karic Modified:
4 years ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
icu Visibility:
Public. |
DescriptionFix big endian support for MIPS
Recent changes in detecting big endian targets don't work when cross
compiling for a target with different endianness. This brakes MIPS.
Added additional checks for big endian MIPS and MIPS64 targets to fix
this.
Patch by miran.karic@imgtec.com
BUG=None
R=bjaideep@ca.ibm.com, jshin@chromium.org
Committed: https://chromium.googlesource.com/chromium/deps/icu/+/73e24736676b4b438270fda44e5b2c83b49fdd80
Patch Set 1 #
Total comments: 2
Patch Set 2 : Merge BE conditions #Messages
Total messages: 15 (3 generated)
miran.karic@imgtec.com changed reviewers: + bjaideep@ca.ibm.com, jshin@chromium.org
Here is what I had in mind, this would fix MIPS and PPC/s390 would work as before the change. Cross compile on a LE host for a BE target would work for MIPS only. PTAL.
lgtm Thank you, yes my changes didn't account for cross-compiling. For ppc64 we don't have separate tokens for LE vs BE, so adding ppc64 as target_arch will not work. The other solution I can think of is to have a target_byteorder token (that should work for all BE platforms), but initializing it needs some thinking, will look into it.
See my comment below. https://codereview.chromium.org/2518533002/diff/1/icu.gyp File icu.gyp (right): https://codereview.chromium.org/2518533002/diff/1/icu.gyp#newcode112 icu.gyp:112: }, 'v8_host_byteorder=="big"', { can you just OR 'v8_host_byteorder==big' with 'target_arch==... mips64"'? https://codereview.chromium.org/2518533002/diff/1/icu.gyp#newcode137 icu.gyp:137: }, 'v8_host_byteorder=="big"', { # Big Endian The same here...
On 2016/11/19 15:57:22, JaideepBajwa (OOO) wrote: > lgtm > Thank you, yes my changes didn't account for cross-compiling. > For ppc64 we don't have separate tokens for LE vs BE, so adding ppc64 as > target_arch will not work. > The other solution I can think of is to have a target_byteorder token (that > should work for all BE platforms), but initializing it needs some thinking, will > look into it. Yeah, I thought of target_byteorder, too.
On 2016/11/21 23:45:25, jungshik at google wrote: > See my comment below. > > https://codereview.chromium.org/2518533002/diff/1/icu.gyp > File icu.gyp (right): > > https://codereview.chromium.org/2518533002/diff/1/icu.gyp#newcode112 > icu.gyp:112: }, 'v8_host_byteorder=="big"', { > can you just OR 'v8_host_byteorder==big' with 'target_arch==... mips64"'? > > https://codereview.chromium.org/2518533002/diff/1/icu.gyp#newcode137 > icu.gyp:137: }, 'v8_host_byteorder=="big"', { # Big Endian > The same here... Done.
On 2016/11/22 09:14:34, miran.karic wrote: > On 2016/11/21 23:45:25, jungshik at google wrote: > > See my comment below. > > > > https://codereview.chromium.org/2518533002/diff/1/icu.gyp > > File icu.gyp (right): > > > > https://codereview.chromium.org/2518533002/diff/1/icu.gyp#newcode112 > > icu.gyp:112: }, 'v8_host_byteorder=="big"', { > > can you just OR 'v8_host_byteorder==big' with 'target_arch==... mips64"'? > > > > https://codereview.chromium.org/2518533002/diff/1/icu.gyp#newcode137 > > icu.gyp:137: }, 'v8_host_byteorder=="big"', { # Big Endian > > The same here... > > Done. Reminder, PTAL.
On 2016/11/28 08:58:35, miran.karic wrote: > On 2016/11/22 09:14:34, miran.karic wrote: > > On 2016/11/21 23:45:25, jungshik at google wrote: > > > See my comment below. > > > > > > https://codereview.chromium.org/2518533002/diff/1/icu.gyp > > > File icu.gyp (right): > > > > > > https://codereview.chromium.org/2518533002/diff/1/icu.gyp#newcode112 > > > icu.gyp:112: }, 'v8_host_byteorder=="big"', { > > > can you just OR 'v8_host_byteorder==big' with 'target_arch==... mips64"'? > > > > > > https://codereview.chromium.org/2518533002/diff/1/icu.gyp#newcode137 > > > icu.gyp:137: }, 'v8_host_byteorder=="big"', { # Big Endian > > > The same here... > > > > Done. > > Reminder, PTAL. Sorry for the delay. LGTM. Can you land it? Perhaps, I have to land it. Let me know if you can land or not.
> Sorry for the delay. LGTM. > Can you land it? Perhaps, I have to land it. Let me know if you can land or not. It seems I cannot, please land it.
@jungshik Can you land this? We would like to see this fixed soon as our big endian builders have been failing for weeks now.
Description was changed from ========== Fix big endian support for MIPS Recent changes in detecting big endian targets don't work when cross compiling for a target with different endianness. This brakes MIPS. Added additional checks for big endian MIPS and MIPS64 targets to fix this. BUG= ========== to ========== Fix big endian support for MIPS Recent changes in detecting big endian targets don't work when cross compiling for a target with different endianness. This brakes MIPS. Added additional checks for big endian MIPS and MIPS64 targets to fix this. Patch by miran.karic@imgtec.com BUG=None ==========
Description was changed from ========== Fix big endian support for MIPS Recent changes in detecting big endian targets don't work when cross compiling for a target with different endianness. This brakes MIPS. Added additional checks for big endian MIPS and MIPS64 targets to fix this. Patch by miran.karic@imgtec.com BUG=None ========== to ========== Fix big endian support for MIPS Recent changes in detecting big endian targets don't work when cross compiling for a target with different endianness. This brakes MIPS. Added additional checks for big endian MIPS and MIPS64 targets to fix this. Patch by miran.karic@imgtec.com BUG=None R=bjaideep@ca.ibm.com, jshin@chromium.org Committed: https://chromium.googlesource.com/chromium/deps/icu/+/73e24736676b4b438270fda... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 73e24736676b4b438270fda44e5b2c83b49fdd80 (presubmit successful).
Message was sent while issue was closed.
On 2016/12/09 18:35:30, jungshik at google wrote: > Committed patchset #2 (id:20001) manually as > 73e24736676b4b438270fda44e5b2c83b49fdd80 (presubmit successful). Sorry for the delay. I landed it. Will roll icu in DEPS.
Message was sent while issue was closed.
On 2016/12/09 18:40:51, jungshik at google wrote: > On 2016/12/09 18:35:30, jungshik at google wrote: > > Committed patchset #2 (id:20001) manually as > > 73e24736676b4b438270fda44e5b2c83b49fdd80 (presubmit successful). > > Sorry for the delay. I landed it. Will roll icu in DEPS. Thank you. It just rolled in DEPS and works as expected, fixing BE builders. |
