Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
494713002:
[MIPS] Add support in common.gypi for building MIPS64 (Closed)
Description[MIPS] Add support in common.gypi for building MIPS64
Add missing variables (android_ndk_*) for building Chromium for MIPS64.
In addition to this, set default mips_arch_variant-s for different MIPS
platforms.
BUG=400684
Committed: https://crrev.com/c8a5da7455b57b2399e4a69e8100c098d9870052
Cr-Commit-Position: refs/heads/master@{#292148}
Patch Set 1 #Patch Set 2 : Finetuning. #
Total comments: 9
Patch Set 3 : Changes per code review. #Patch Set 4 : Fix issue with non-MIPS arch. #Patch Set 5 : More finetuning while waiting for code-review. #Patch Set 6 : Reduce ndk configurations. #
Total comments: 6
Patch Set 7 : Address comments. #Messages
Total messages: 16 (0 generated)
PTAL.
https://codereview.chromium.org/494713002/diff/20001/build/common.gypi File build/common.gypi (left): https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#oldcod... build/common.gypi:3923: '-mhard-float', Did you intend to remove the hard-float ABI here? https://codereview.chromium.org/494713002/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#newcod... build/common.gypi:190: 'mips_arch_variant%': 'r1', Please do this as a seperate condition (with target_arch=="mipsel" explicitly). Also please move these down to bottom of the conditions block (below OS==ios on line 251) https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#newcod... build/common.gypi:1718: ['mips_arch_variant=="r6"', { What is the use-case of using the mips64 toolchain / sysroot for a 32bit mipsel r6 devices? This seems wrong to me. Do we need non-64bit mipsel r6 at all? https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#newcod... build/common.gypi:1992: # by default except on arm and mips. update comment
https://codereview.chromium.org/494713002/diff/20001/build/common.gypi File build/common.gypi (left): https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#oldcod... build/common.gypi:3923: '-mhard-float', On 2014/08/22 13:54:40, rmcilroy wrote: > Did you intend to remove the hard-float ABI here? No, hard-float is default, so this seemed unnecessary. https://codereview.chromium.org/494713002/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#newcod... build/common.gypi:190: 'mips_arch_variant%': 'r1', On 2014/08/22 13:54:40, rmcilroy wrote: > Please do this as a seperate condition (with target_arch=="mipsel" explicitly). > Also please move these down to bottom of the conditions block (below OS==ios on > line 251) Done. https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#newcod... build/common.gypi:1718: ['mips_arch_variant=="r6"', { On 2014/08/22 13:54:40, rmcilroy wrote: > What is the use-case of using the mips64 toolchain / sysroot for a 32bit mipsel > r6 devices? This seems wrong to me. Do we need non-64bit mipsel r6 at all? The idea is to use a unified toolchain. Yes, we will need mips32r6 version as well. https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#newcod... build/common.gypi:1992: # by default except on arm and mips. On 2014/08/22 13:54:40, rmcilroy wrote: > update comment Done.
On 2014/08/22 14:28:13, petarj wrote: > https://codereview.chromium.org/494713002/diff/20001/build/common.gypi > File build/common.gypi (left): > > https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#oldcod... > build/common.gypi:3923: '-mhard-float', > On 2014/08/22 13:54:40, rmcilroy wrote: > > Did you intend to remove the hard-float ABI here? > > No, hard-float is default, so this seemed unnecessary. > > https://codereview.chromium.org/494713002/diff/20001/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#newcod... > build/common.gypi:190: 'mips_arch_variant%': 'r1', > On 2014/08/22 13:54:40, rmcilroy wrote: > > Please do this as a seperate condition (with target_arch=="mipsel" > explicitly). > > Also please move these down to bottom of the conditions block (below OS==ios > on > > line 251) > > Done. > > https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#newcod... > build/common.gypi:1718: ['mips_arch_variant=="r6"', { > On 2014/08/22 13:54:40, rmcilroy wrote: > > What is the use-case of using the mips64 toolchain / sysroot for a 32bit > mipsel > > r6 devices? This seems wrong to me. Do we need non-64bit mipsel r6 at all? > > The idea is to use a unified toolchain. > Yes, we will need mips32r6 version as well. > > https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#newcod... > build/common.gypi:1992: # by default except on arm and mips. > On 2014/08/22 13:54:40, rmcilroy wrote: > > update comment > > Done. The change seems to miss to set mips_arch_variant for non-mips arches.
New patch uploaded. PTAL.
> The change seems to miss to set mips_arch_variant for non-mips arches. This sounds like mips_arch_variant is being referenced for non-mips arches which sounds like a bug - is this the case? https://codereview.chromium.org/494713002/diff/20001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/494713002/diff/20001/build/common.gypi#newcod... build/common.gypi:1718: ['mips_arch_variant=="r6"', { On 2014/08/22 14:28:13, petarj wrote: > On 2014/08/22 13:54:40, rmcilroy wrote: > > What is the use-case of using the mips64 toolchain / sysroot for a 32bit > mipsel > > r6 devices? This seems wrong to me. Do we need non-64bit mipsel r6 at all? > > The idea is to use a unified toolchain. > Yes, we will need mips32r6 version as well. In that case, do we need mips32r1 (and mips64r2 for that matter)? My (limited) understanding is that the intention is that Android Apps should move to mips32/64 r6 for future, so do we ever need to build Chrome for any other mips variant? I would prefer to limit the matrix of possible configurations as much as possible.
On 2014/08/22 16:57:51, rmcilroy wrote: > > The change seems to miss to set mips_arch_variant for non-mips arches. > > This sounds like mips_arch_variant is being referenced for non-mips arches which > sounds like a bug - is this the case? > That was my comment for the patch set #3. There are no issues with the current patch set. > In that case, do we need mips32r1 (and mips64r2 for that matter)? My (limited) > understanding is that the intention is that Android Apps should move to > mips32/64 r6 for future, so do we ever need to build Chrome for any other mips > variant? I would prefer to limit the matrix of possible configurations as much > as possible. > Here are relevant reasons for this variety: 1. Chrome for Android should be built for MIPS32R1 as it will be distributed as an APK from Google Play store and that is our Android app ABI. 2. Android Webview/system image can be built with other options depending on the platform it will be used on, and thus other MIPS revisions (MIPS32R1, MIPS32R2, MIPS32R6) should be present in the build file. 3. Last, Chromium for Linux is used on platforms with different MIPS revisions (MIPS32R1, MIPS32R2, MIPS32R6), and thus we keep and test common options.
> > This sounds like mips_arch_variant is being referenced for non-mips arches which > > sounds like a bug - is this the case? > > That was my comment for the patch set #3. There are no issues with the > current patch set. The way you solved this though was to add a default value which is set even if mips is not the target architecture, so the bug where mips_arch_variant is being referenced on non-mips builds still exists. > Here are relevant reasons for this variety: > > 1. Chrome for Android should be built for MIPS32R1 as it will be > distributed as an APK from Google Play store and that is our Android > app ABI. This is fine. > 2. Android Webview/system image can be built with other options > depending on the platform it will be used on, and thus other MIPS > revisions (MIPS32R1, MIPS32R2, MIPS32R6) should be present in the build > file. The webview doesn't use the NDK, so there doesn't need to be support for r1,r2 and r6 NDK toolchain configurations. > 3. Last, Chromium for Linux is used on platforms with different MIPS > revisions (MIPS32R1, MIPS32R2, MIPS32R6), and thus we keep and test > common options. Similarly Linux doesn't use the NDK, so again we don't need these NDK toolchain configurations. I think you can get away with just MIPS32R1 and MIPS64R6 for the NDK toolchain setup, since these are the only revisions for which Chrome will be built. You can manually build for other revisions by setting the gyp variables (e.g., "android_ndk_sysroot") manually at gyp time, and can even have bots which build these configurations if you want to do automated testing locally, however common.gypi should only have the versions which are officially supported.
On 2014/08/26 09:41:18, rmcilroy wrote: > The way you solved this though was to add a default value which is set even if > mips is not the target architecture, so the bug where mips_arch_variant is being > referenced on non-mips builds still exists. > This is done that way because we need to copy conditionally-set variable (mips_arch_variant in this case) from one scope to another. For instance, the same is done for arm_version just a few lines above. And if you remove that line (setting default value for arm_version), it will cause the very same issue when the code is configured for mips. > The webview doesn't use the NDK, so there doesn't need to be support for r1,r2 > and r6 NDK toolchain configurations. > True, we can remove NDK toolchain configurations (though they were handy when building content_shell for different revisions, but I understand your objections to limit common.gypi only to necessary configurations). > I think you can get away with just MIPS32R1 and MIPS64R6 for the NDK toolchain > setup, since these are the only revisions for which Chrome will be built. You > can manually build for other revisions by setting the gyp variables (e.g., > "android_ndk_sysroot") manually at gyp time, and can even have bots which build > these configurations if you want to do automated testing locally, however > common.gypi should only have the versions which are officially supported. > Understood. Will upload a new patch shortly.
New patch has been uploaded.
lgtm once comments are addressed - thanks! https://codereview.chromium.org/494713002/diff/100001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/494713002/diff/100001/build/common.gypi#newco... build/common.gypi:180: 'mips_arch_variant%': '', Mention in the comment is set in the conditions block below for mips targets. https://codereview.chromium.org/494713002/diff/100001/build/common.gypi#newco... build/common.gypi:254: }], insert a newline between the conditions https://codereview.chromium.org/494713002/diff/100001/build/common.gypi#newco... build/common.gypi:3949: 'ldflags': [ '-mips32r6', '-Wl,-melf32ltsmip', ], please fold this into the above 'android_webview_build==0 and mips_arch_variant=="r6"' so something like: ['android_webview_build==0 and mips_arch_variant=="r6"', { 'cflags': ['-mips32r6', '-Wa,-mips32r6'], 'conditions': [ ['OS=="android"', { 'ldflags': [ '-mips32r6', '-Wl,-melf32ltsmip', ], }], ], }],
https://chromiumcodereview.appspot.com/494713002/diff/100001/build/common.gypi File build/common.gypi (right): https://chromiumcodereview.appspot.com/494713002/diff/100001/build/common.gyp... build/common.gypi:180: 'mips_arch_variant%': '', On 2014/08/26 17:40:19, rmcilroy wrote: > Mention in the comment is set in the conditions block below for mips targets. Done. https://chromiumcodereview.appspot.com/494713002/diff/100001/build/common.gyp... build/common.gypi:254: }], On 2014/08/26 17:40:19, rmcilroy wrote: > insert a newline between the conditions Done. https://chromiumcodereview.appspot.com/494713002/diff/100001/build/common.gyp... build/common.gypi:3949: 'ldflags': [ '-mips32r6', '-Wl,-melf32ltsmip', ], On 2014/08/26 17:40:19, rmcilroy wrote: > please fold this into the above 'android_webview_build==0 and > mips_arch_variant=="r6"' so something like: > > ['android_webview_build==0 and mips_arch_variant=="r6"', { > 'cflags': ['-mips32r6', '-Wa,-mips32r6'], > 'conditions': [ > ['OS=="android"', { > 'ldflags': [ '-mips32r6', '-Wl,-melf32ltsmip', ], > }], > ], > }], Done.
The CQ bit was checked by petarj@mips.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petarj@mips.com/494713002/120001
Message was sent while issue was closed.
Committed patchset #7 (120001) as 0903318799461e046f355aec2ba561a08673762e
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/c8a5da7455b57b2399e4a69e8100c098d9870052 Cr-Commit-Position: refs/heads/master@{#292148} |
