Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1951133002:
Use gold and the bundled binutils on x86 by default (Closed)
|
Created:
4 years, 7 months ago by Dirk Pranke Modified:
4 years, 7 months ago CC:
chromium-reviews, Tom Anderson Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse gold and the bundled binutils on x86 by default
The only reason we were not using gold by default on x86
is that using icf was buggy. However, we think we've updated
to a working version of gold, and so we should now use it.
R=mcgrathr@chromium.org, thakis@chromium.org
BUG=590004
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_compile_dbg_32_ng,linux_chromium_dbg_32_ng
Committed: https://crrev.com/11668d5556912ae81fa645cea8a5b7bb381407c7
Cr-Commit-Position: refs/heads/master@{#393645}
Patch Set 1 #Patch Set 2 : fix typo #Patch Set 3 : fix more typos #
Total comments: 2
Patch Set 4 : split icf out into a different CL #Patch Set 5 : merge #Messages
Total messages: 28 (13 generated)
Description was changed from ========== Use gold on x86 by default (with icf turned off). The only reason we're not using gold by default on x86 is that using icf is buggy in our version. However, since we don't really care about distributing x86 chrome builds at this point, it seems reasonable to turn it off so that we don't need to worry about compatibility with old versions of bfd any more. R=mcgrathr@chromium.org, thestig@chromium.org, thakis@chromium.org BUG=590004 ========== to ========== Use gold on x86 by default (with icf turned off). The only reason we're not using gold by default on x86 is that using icf is buggy in our version. However, since we don't really care about distributing x86 chrome builds at this point, it seems reasonable to turn it off so that we don't need to worry about compatibility with old versions of bfd any more. R=mcgrathr@chromium.org, thestig@chromium.org, thakis@chromium.org BUG=590004 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_compile_dbg_32_ng,linux_chromium_dbg_32_ng ==========
The CQ bit was checked by dpranke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1951133002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1951133002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dpranke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1951133002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1951133002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dpranke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1951133002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1951133002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gold not working when targeting ia32 is news to me, do you have a source for that? Is it broken with --icf=safe too? https://codereview.chromium.org/1951133002/diff/40001/build/common.gypi File build/common.gypi (left): https://codereview.chromium.org/1951133002/diff/40001/build/common.gypi#oldco... build/common.gypi:921: # for component=static_library builds. This comment is obsolete, we switched to 64-bit host gold always quite a long time ago. https://codereview.chromium.org/1951133002/diff/40001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1951133002/diff/40001/build/common.gypi#newco... build/common.gypi:924: # icf=all is buggy on ia32, so disabled by default. Where is this from? I can't see evidence of this, and it's new from what I can tell.
There are two relevant Gold bugs I know about: * https://sourceware.org/bugzilla/show_bug.cgi?id=19047 This affects all REL (vs RELA) machines, which for us means arm32 and x86-32 (x86-64 is RELA). It affects all ICF modes, i.e. anything but --icf=none. Whether it will strike in practice depends on exact details of relocs generated by the compiler, and so is not easy to predict or work around. The only failure mode I'm aware of is the linker crashing, but since the bug manifests as a bad pointer access, stranger outcomes might be possible. This is fixed upstream by commit 84d543b7ed93bf6511cdf45791f4f0b717dfb718 on trunk, but that did not make the 2.26 release and has not been backported (not even to the 2.26 branch after release). * https://sourceware.org/bugzilla/show_bug.cgi?id=17704; https://bugs.chromium.org/p/chromium/issues/detail?id=576197 This probably affects both x86-32 and x86-64 and almost certainly doesn't affect arm. It affects only --icf=all and not --icf=safe. The failure mode is strange runtime crashes in obscure circumstances. Patches are proposed on the bug, but none has been directly submitted upstream and upstream maintainers have not responded to the issue. tmsriram@google.com is the person responsible upstream for gold's ICF. He fixed the first bug, but hasn't done anything about backporting (I asked him to, but I haven't poked him about it again). I haven't seen him respond about the second bug. Its fix is conceptually straightforward (among identical code sections, keep the one with the largest sh_addralign rather than the first one seen), but I don't know how straightforward that is in the code and I haven't reviewed the patches proposed on the bug.
We could backport the first fix if it applies cleanly (prior art: https://codereview.chromium.org/1580443002).
On 2016/05/05 16:54:01, Nico wrote: > We could backport the first fix if it applies cleanly (prior art: > https://codereview.chromium.org/1580443002). It does apply cleanly on top of 2.26. I'll poke upstream about applying it to the 2.26 release branch too.
Description was changed from ========== Use gold on x86 by default (with icf turned off). The only reason we're not using gold by default on x86 is that using icf is buggy in our version. However, since we don't really care about distributing x86 chrome builds at this point, it seems reasonable to turn it off so that we don't need to worry about compatibility with old versions of bfd any more. R=mcgrathr@chromium.org, thestig@chromium.org, thakis@chromium.org BUG=590004 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_compile_dbg_32_ng,linux_chromium_dbg_32_ng ========== to ========== Use gold and the bundled binutils on x86 by default The only reason we were not using gold by default on x86 is that using icf was buggy. However, we think we've updated to a working version of gold, and so we should now use it. R=mcgrathr@chromium.org, thakis@chromium.org BUG=590004 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_compile_dbg_32_ng,linux_chromium_dbg_32_ng ==========
dpranke@chromium.org changed reviewers: - thestig@chromium.org
Okay, let's try this again, splitting things out into multiple patches. First, I'll assume that Roland's binutils roll lands. Second, I've posted a GN-only patch that will I believe reflect the state of icf flags correctly to https://codereview.chromium.org/1952353004/. Once those two have landed, I think it'll be safe to flip GN x86 builds to GN + binutils by default (this patch). Then, once this finally lands, I think we can flip the x86 bots to GN by default (a follow-on CL). Please take another look to see if I have this straight ...
lgtm
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcgrathr@chromium.org Link to the patchset: https://codereview.chromium.org/1951133002/#ps80001 (title: "merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1951133002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1951133002/80001
Message was sent while issue was closed.
Description was changed from ========== Use gold and the bundled binutils on x86 by default The only reason we were not using gold by default on x86 is that using icf was buggy. However, we think we've updated to a working version of gold, and so we should now use it. R=mcgrathr@chromium.org, thakis@chromium.org BUG=590004 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_compile_dbg_32_ng,linux_chromium_dbg_32_ng ========== to ========== Use gold and the bundled binutils on x86 by default The only reason we were not using gold by default on x86 is that using icf was buggy. However, we think we've updated to a working version of gold, and so we should now use it. R=mcgrathr@chromium.org, thakis@chromium.org BUG=590004 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_compile_dbg_32_ng,linux_chromium_dbg_32_ng ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Use gold and the bundled binutils on x86 by default The only reason we were not using gold by default on x86 is that using icf was buggy. However, we think we've updated to a working version of gold, and so we should now use it. R=mcgrathr@chromium.org, thakis@chromium.org BUG=590004 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_compile_dbg_32_ng,linux_chromium_dbg_32_ng ========== to ========== Use gold and the bundled binutils on x86 by default The only reason we were not using gold by default on x86 is that using icf was buggy. However, we think we've updated to a working version of gold, and so we should now use it. R=mcgrathr@chromium.org, thakis@chromium.org BUG=590004 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_compile_dbg_32_ng,linux_chromium_dbg_32_ng Committed: https://crrev.com/11668d5556912ae81fa645cea8a5b7bb381407c7 Cr-Commit-Position: refs/heads/master@{#393645} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/11668d5556912ae81fa645cea8a5b7bb381407c7 Cr-Commit-Position: refs/heads/master@{#393645} |
