build: On Android, disable LTO for specific targets only when targeting GCC.

build: On Android, disable LTO for specific targets only when targeting GCC.

The set of targets for which LTO is problematic will most likely differ
between GCC and LLVM. We also need different logic for LLVM (specifically,
LTO should not be disabled when building with cfi_vptr==1, as the -flto flag
must be passed together with -fsanitize=cfi*); this unbreaks the build for
those targets on Android when cfi_vptr==1.

Also add a drive-by TODO to use -lto_library flag on Mac.

BUG=469376
R=thakis@chromium.org, fdegans@chromium.org
TBR=jam@chromium.org

Committed: https://crrev.com/e510374e7f8b5a9ee03c10d120f582b85f5b3cf8
Cr-Commit-Position: refs/heads/master@{#345492}

[jam, 2015-08-20 22:58:56]

I'm not a good reviewer for this. find someone from android team.

[jam, 2015-08-20 22:59:28]

btw I'm sorry for taking so long to respond, I missed this change in my queue.

[pcc1, 2015-08-20 23:01:12]

pcc@chromium.org changed reviewers:
+ fdegans@chromium.org

[pcc1, 2015-08-20 23:01:28]

fdegans: can you please take a look?

[Fabrice (no longer in Chrome), 2015-08-20 23:07:46]

https://codereview.chromium.org/1289623006/diff/1/build/android/disable_lto.gypi
File build/android/disable_lto.gypi (right):

https://codereview.chromium.org/1289623006/diff/1/build/android/disable_lto.g...
build/android/disable_lto.gypi:8: # disable_gcc_lto.gypi and remove this file.
I believe there are no references from external repositories to this file. Could
you try removing it and see if the tryjobs are happy with it?

[pcc1, 2015-08-20 23:11:04]

https://codereview.chromium.org/1289623006/diff/1/build/android/disable_lto.gypi
File build/android/disable_lto.gypi (right):

https://codereview.chromium.org/1289623006/diff/1/build/android/disable_lto.g...
build/android/disable_lto.gypi:8: # disable_gcc_lto.gypi and remove this file.
On 2015/08/20 23:07:46, Fabrice wrote:
> I believe there are no references from external repositories to this file.
Could
> you try removing it and see if the tryjobs are happy with it?

I can see a reference in a Google internal repository (details sent separately).
I don't know what the consequences of breaking this reference would be.

[pcc, 2015-08-20 23:21:39]

pcc@google.com changed reviewers:
+ pcc@google.com

[pcc, 2015-08-20 23:21:39]

https://codereview.chromium.org/1289623006/diff/1/build/android/disable_lto.gypi
File build/android/disable_lto.gypi (right):

https://codereview.chromium.org/1289623006/diff/1/build/android/disable_lto.g...
build/android/disable_lto.gypi:8: # disable_gcc_lto.gypi and remove this file.
On 2015/08/20 23:07:46, Fabrice wrote:
> I believe there are no references from external repositories to this file.
Could
> you try removing it and see if the tryjobs are happy with it?

Done.

[Fabrice (no longer in Chrome), 2015-08-20 23:22:07]

lgtm

[pcc, 2015-08-20 23:22:48]

jam: rubber stamp please

[Fabrice (no longer in Chrome), 2015-08-24 09:57:47]

The CQ bit was checked by fdegans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-24 09:57:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1289623006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1289623006/20001

[commit-bot: I haz the power, 2015-08-24 10:52:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-24 10:52:57]

Dry run: This issue passed the CQ dry run.

[jam, 2015-08-24 14:44:58]

jam@chromium.org changed reviewers:
+ thakis@chromium.org

[jam, 2015-08-24 14:44:58]

+thakis

[Nico, 2015-08-24 15:26:21]

lgtm if external forces make it impossible to rename the gyp variable.

Do you think we'll have to disable LTO for any targets for clang? This is only
necessary in case of toolchain bugs, which we can fix, right? (I'm a bit bummed
that we disabled it for some targets with gcc, but fix deployment time is a bit
higher there I suppose.)

https://codereview.chromium.org/1289623006/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1289623006/diff/20001/build/common.gypi#newco...
build/common.gypi:687: # On Android, when using GCC LTO, the variable use_lto
enables LTO on code
Seems like it should be called use_lto_gcc then?

[Fabrice (no longer in Chrome), 2015-08-24 15:31:54]

On 2015/08/24 15:26:21, Nico wrote:
> lgtm if external forces make it impossible to rename the gyp variable.
> 
> Do you think we'll have to disable LTO for any targets for clang? This is only
> necessary in case of toolchain bugs, which we can fix, right? (I'm a bit
bummed
> that we disabled it for some targets with gcc, but fix deployment time is a
bit
> higher there I suppose.)

Most of these should be fixed with GCC 5.1. Unfortunately, integration of GCC
5.1 in the NDK does not seem to be high priority.

https://codereview.chromium.org/1289623006/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1289623006/diff/20001/build/common.gypi#newco...
build/common.gypi:687: # On Android, when using GCC LTO, the variable use_lto
enables LTO on code
On 2015/08/24 15:26:21, Nico wrote:
> Seems like it should be called use_lto_gcc then?

use_lto is also used when building with clang.

[Nico, 2015-08-24 15:34:36]

https://codereview.chromium.org/1289623006/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1289623006/diff/20001/build/common.gypi#newco...
build/common.gypi:690: # use_lto_o2 has no effect.
Ah, I misunderstood the "on other platforms". Maybe it could be more obvious
that "other platforms" includes "android with clang" – "other platforms" made me
think of "not android".

[pcc, 2015-08-24 19:25:23]

https://codereview.chromium.org/1289623006/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1289623006/diff/20001/build/common.gypi#newco...
build/common.gypi:690: # use_lto_o2 has no effect.
On 2015/08/24 15:34:36, Nico wrote:
> Ah, I misunderstood the "on other platforms". Maybe it could be more obvious
> that "other platforms" includes "android with clang" – "other platforms" made
me
> think of "not android".

Added clarification.

https://codereview.chromium.org/1289623006/diff/20001/build/common.gypi#newco...
build/common.gypi:697: # $DYLD_LIBRARY_PATH to pick up the right version of the
linker plugin.
Also added a drive-by TODO for something we can do better here.

[Nico, 2015-08-24 19:27:49]

lgtm, thanks.

[pcc1, 2015-08-24 19:35:29]

jam: back to you

[Nico, 2015-08-25 15:35:32]

I think you can TBR=jam. The changes outside of build/ are mechanical.

[pcc1, 2015-08-25 22:10:01]

The CQ bit was checked by pcc@chromium.org

[pcc1, 2015-08-25 22:10:01]

The patchset sent to the CQ was uploaded after l-g-t-m from fdegans@chromium.org
Link to the patchset: https://codereview.chromium.org/1289623006/#ps40001
(title: "Clarify comment in build/common.gypi; add TODO to use -lto_library")

[commit-bot: I haz the power, 2015-08-25 22:10:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1289623006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1289623006/40001

[commit-bot: I haz the power, 2015-08-25 22:21:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-25 22:21:52]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[pcc1, 2015-08-25 23:14:39]

The CQ bit was checked by pcc@chromium.org

[commit-bot: I haz the power, 2015-08-25 23:15:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1289623006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1289623006/40001

[commit-bot: I haz the power, 2015-08-26 00:04:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-08-26 00:05:11]

Patchset 3 (id:??) landed as
https://crrev.com/e510374e7f8b5a9ee03c10d120f582b85f5b3cf8
Cr-Commit-Position: refs/heads/master@{#345492}