Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196573022:
Use gold from third_party/binutils rather then third_party/gold. (Closed)
|
Created:
6 years, 9 months ago by mithro-old Modified:
6 years, 8 months ago CC:
chromium-reviews, Jeffrey Yasskin, cmtice, Han Shen, yunlian, vapier, llozano Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionWhen "linux_use_gold_binary" is set, we now use gold from third_party/binutils rather than third_party/gold (allowing us to remove third_party/gold dependency).
As third_party/binutils is 2.24, this also satisfies the binutils version requirements for Debug Fission. Thus, when building with clang on Ubuntu Precise it should now be enabled. This should cause a significant speedup in linking for most chrome developers on Linux.
Requires;
* https://codereview.chromium.org/209853003/ - Adding binutils as a DEPS to allow DebugFission on Ubuntu Precise when compiling with clang.
BUG=352046
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262794
Patch Set 1 #Patch Set 2 : Removing the dependent patch. #Patch Set 3 : Rebase onto master. #
Total comments: 1
Patch Set 4 : Rebasing onto master. #Patch Set 5 : Speling mistake #Patch Set 6 : Split setting varibles and setting flags. #Patch Set 7 : Removing tab. #
Total comments: 4
Patch Set 8 : Small fixes. #Patch Set 9 : Fixing clang compiler_version problem. #Patch Set 10 : git cl WHY YOU UPLOAD .gitmodules!?@ #Patch Set 11 : Fixing patch with asan (and related) builders. #Patch Set 12 : Fixing tabs. #
Total comments: 1
Patch Set 13 : Adding --disable-new-dtags flag. #Messages
Total messages: 47 (0 generated)
https://codereview.chromium.org/196573022/diff/40001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/196573022/diff/40001/build/common.gypi#newcod... build/common.gypi:3056: 'binutils_version%': '<!(python <(DEPTH)/build/compiler_version.py assembler)', I think this line needs to have the -B from below passed somehow....
to be clear, you're not building & using the local binutils for chromeos builds right ? we want the system binutils to be used.
Just FYI, we are going to upgrade binutils in a few weeks, which has support for 'extract-dwo'.
On 2014/03/19 19:43:05, vapier wrote: > to be clear, you're not building & using the local binutils for chromeos builds > right ? we want the system binutils to be used. Yes, please make sure this is not enabled from ChromeOS. I'm not that familiar with common.gypi logic, but there's a chromeos variable that can be checked ( chromeos == 1 for ChromeOS), perhaps need to explicitly exclude it? I don't know how fission interacts with splitdebug (which is what is used in ChromeOS). We probably also want this in ChromeOS at some point.
On 2014/03/19 21:21:35, bjanakiraman1 wrote: > On 2014/03/19 19:43:05, vapier wrote: > > to be clear, you're not building & using the local binutils for chromeos > builds > > right ? we want the system binutils to be used. > > Yes, please make sure this is not enabled from ChromeOS. > > I'm not that familiar with common.gypi logic, but there's a chromeos variable > that can be checked ( chromeos == 1 for ChromeOS), perhaps need to explicitly > exclude it? > > I don't know how fission interacts with splitdebug (which is what is used in > ChromeOS). We probably also want this in ChromeOS at some point. Have a pointer to "splitdebug" and I can see what is going on here. Though, yes, likely you'll want to use this in ChromeOS.
we use this functionality today: https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html
On 2014/03/19 20:15:53, Han wrote: > Just FYI, we are going to upgrade binutils in a few weeks, which has support for > 'extract-dwo'. Han, you mean upgrading binutils in ChromeOS? Does ChromeOS build with gcc4.8 or clang? If it uses clang does it use the system clang or the on in third_party/llvm-build? Thanks!
normally we build with gcc-4.8. we have an ASAN config that uses clang, but that too is the system one. we control all system toolchains.
On 2014/03/20 02:44:51, mithro wrote: > On 2014/03/19 20:15:53, Han wrote: > > Just FYI, we are going to upgrade binutils in a few weeks, which has support > for > > 'extract-dwo'. > > Han, you mean upgrading binutils in ChromeOS? > > Does ChromeOS build with gcc4.8 or clang? If it uses clang does it use the > system clang or the on in third_party/llvm-build? > > Thanks! Hi, yes, we are to upgrade system wide binutils in ChromeOs.
Hi Nico / Lei, Can you please take another look at this CL? I'd like to land it after https://codereview.chromium.org/209853003/ makes it through the commit queue. When "linux_use_gold_binary" is set, we now use gold from third_party/binutils rather than third_party/gold (allowing us to remove third_party/gold dependency). I'm wondering if linux_use_gold_binary should be renamed something like "linux_use_binutils" or "use_thirdparty_binutils". As third_party/binutils is 2.24, this also satisfies the binutils version requirements for Debug Fission. Thus, when building with clang on Ubuntu Precise it should now be enabled. Tim
lgtm assuming CrOS folks are ok with this. In my head this works on CrOS, but it will be good to double check. https://codereview.chromium.org/196573022/diff/110001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/196573022/diff/110001/build/common.gypi#newco... build/common.gypi:1348: 'binutils_version%': 222, In this case, since we already know, should this omit the '%', or do you still want binutils_version to be overwritable from GYP_DEFINES? Same for binutils_version and binutils_dir below. https://codereview.chromium.org/196573022/diff/110001/build/common.gypi#newco... build/common.gypi:1355: 'binutils_dir%': 'third_party/binutils/Linux_x64/Release', Since there will always be a Release component in the path, move it down to line 3866/3869. Alternatively, specify path/to/Release/bin here and just use <(binutils_dir) below?
Did you have a chance to test with goma? On Wed, Apr 2, 2014 at 7:53 PM, <mithro@mithis.com> wrote: > Hi Nico / Lei, > > Can you please take another look at this CL? I'd like to land it after > https://codereview.chromium.org/209853003/ makes it through the commit > queue. > > When "linux_use_gold_binary" is set, we now use gold from > third_party/binutils > rather than third_party/gold (allowing us to remove third_party/gold > dependency). > > I'm wondering if linux_use_gold_binary should be renamed something like > "linux_use_binutils" or "use_thirdparty_binutils". > > As third_party/binutils is 2.24, this also satisfies the binutils version > requirements for Debug Fission. Thus, when building with clang on Ubuntu > Precise > it should now be enabled. > > Tim > > https://codereview.chromium.org/196573022/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/04/03 23:05:08, Nico wrote: > Did you have a chance to test with goma? Yes. It "kind of" works with goma, isn't transferring the dwo files back to the computer so Gold complains about the index being empty. I've just disabled the options for goma for now. Will look at patching. Tim https://codereview.chromium.org/196573022/diff/110001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/196573022/diff/110001/build/common.gypi#newco... build/common.gypi:1348: 'binutils_version%': 222, On 2014/04/03 23:00:01, Lei Zhang wrote: > In this case, since we already know, should this omit the '%', or do you still > want binutils_version to be overwritable from GYP_DEFINES? > > Same for binutils_version and binutils_dir below. Done. https://codereview.chromium.org/196573022/diff/110001/build/common.gypi#newco... build/common.gypi:1355: 'binutils_dir%': 'third_party/binutils/Linux_x64/Release', On 2014/04/03 23:00:01, Lei Zhang wrote: > Since there will always be a Release component in the path, move it down to line > 3866/3869. Alternatively, specify path/to/Release/bin here and just use > <(binutils_dir) below? Done.
Just filling a bug with the goma guys might be enough :-) On Apr 3, 2014 8:36 PM, <mithro@mithis.com> wrote: > On 2014/04/03 23:05:08, Nico wrote: > >> Did you have a chance to test with goma? >> > > Yes. It "kind of" works with goma, isn't transferring the dwo files back > to the > computer so Gold complains about the index being empty. > > I've just disabled the options for goma for now. Will look at patching. > > Tim > > > https://codereview.chromium.org/196573022/diff/110001/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/196573022/diff/110001/ > build/common.gypi#newcode1348 > build/common.gypi:1348: 'binutils_version%': 222, > On 2014/04/03 23:00:01, Lei Zhang wrote: > >> In this case, since we already know, should this omit the '%', or do >> > you still > >> want binutils_version to be overwritable from GYP_DEFINES? >> > > Same for binutils_version and binutils_dir below. >> > > Done. > > https://codereview.chromium.org/196573022/diff/110001/ > build/common.gypi#newcode1355 > build/common.gypi:1355: 'binutils_dir%': > 'third_party/binutils/Linux_x64/Release', > On 2014/04/03 23:00:01, Lei Zhang wrote: > >> Since there will always be a Release component in the path, move it >> > down to line > >> 3866/3869. Alternatively, specify path/to/Release/bin here and just >> > use > >> <(binutils_dir) below? >> > > Done. > > https://codereview.chromium.org/196573022/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by mithro@mithis.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mithro@mithis.com/196573022/150001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was checked by mithro@mithis.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mithro@mithis.com/196573022/150001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was checked by mithro@mithis.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mithro@mithis.com/196573022/170002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mithro@mithis.com/196573022/170002
Message was sent while issue was closed.
Change committed as 261778
https://codereview.chromium.org/196573022/diff/210001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/196573022/diff/210001/build/common.gypi#newco... build/common.gypi:1363: ['clang==0 and asan==0 and lsan==0 and tsan==0 and msan==0', { This fix copied from https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...
The CQ bit was checked by mithro@mithis.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mithro@mithis.com/196573022/210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_compile_dbg
The CQ bit was checked by mithro@mithis.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mithro@mithis.com/196573022/210001
Message was sent while issue was closed.
Change committed as 262031
The CQ bit was checked by mithro@mithis.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mithro@mithis.com/196573022/230001
Message was sent while issue was closed.
Change committed as 262794
Message was sent while issue was closed.
On 2014/04/09 20:20:49, I haz the power (commit-bot) wrote:
> Change committed as 262794
FYI, since this change add -B to cflags, it makes icecc compile all files
locally (basically breaking it). I don't know any details why. Icecc has a
comment that says:
/* -B overwrites the path where the compiler finds the assembler.
As we don't use that, better force local job. */
Any known work-around so that -B doesn't get set?
We kind of depend on being able to use icecc to get sane compile times, so at
Opera we'll probably have to carry patches locally to revert parts of this
patch.
Message was sent while issue was closed.
On 2014/04/10 08:43:38, David Vest wrote: > On 2014/04/09 20:20:49, I haz the power (commit-bot) wrote: > > Change committed as 262794 > > FYI, since this change add -B to cflags, it makes icecc compile all files > locally (basically breaking it). I don't know any details why. Icecc has a > comment that says: > > /* -B overwrites the path where the compiler finds the assembler. > As we don't use that, better force local job. */ icecc should probably support -B in some way. > Any known work-around so that -B doesn't get set? > > We kind of depend on being able to use icecc to get sane compile times, so at > Opera we'll probably have to carry patches locally to revert parts of this > patch. You can do a GYP_DEFINES="linux_use_gold_binary=0" which will prevent the addition of -B for using the binutils. Tim
Message was sent while issue was closed.
On 2014/04/10 08:51:47, mithro wrote: > On 2014/04/10 08:43:38, David Vest wrote: > > On 2014/04/09 20:20:49, I haz the power (commit-bot) wrote: > > > Change committed as 262794 > > > > FYI, since this change add -B to cflags, it makes icecc compile all files > > locally (basically breaking it). I don't know any details why. Icecc has a > > comment that says: > > > > /* -B overwrites the path where the compiler finds the assembler. > > As we don't use that, better force local job. */ > > icecc should probably support -B in some way. > > > Any known work-around so that -B doesn't get set? > > > > We kind of depend on being able to use icecc to get sane compile times, so at > > Opera we'll probably have to carry patches locally to revert parts of this > > patch. > > You can do a GYP_DEFINES="linux_use_gold_binary=0" which will prevent the > addition of -B for using the binutils. > > Tim This seems to have broken the ARM cross build: http://build.chromium.org/p/chromium.fyi/builders/Linux%20ARM%20Cross-Compile I guess we were using linux_use_gold_binary and this worked find before because I guess gold is multi-arch. But the other binutils in there don't seem to like ARM: Assembler messages: Fatal error: invalid -march= option: `armv7' Not sure how to proceed here. We could disable use of gold linker on ARM, but that seems unfortunate. Would it be possible to split gold from the other binutils? Or perhaps make the binutil multi-arch? Also, isn't the linux_use_gold_binary a little miss-named now as it does more than just use the gold binary now?
Message was sent while issue was closed.
On 2014/04/14 19:10:51, Sam Clegg wrote: > Also, isn't the linux_use_gold_binary a little miss-named now as it does more > than just use the gold binary now? Yes, we should rename the gyp variable or better organize linux_use_gold_binary/linux_use_gold_flags/binutils_version. Then icecc users, full ChromeOS builds, and others with their own toolchains can set the right gyp flags and avoid these problems.
Message was sent while issue was closed.
On 2014/04/14 19:10:51, Sam Clegg wrote: > On 2014/04/10 08:51:47, mithro wrote: > > On 2014/04/10 08:43:38, David Vest wrote: > > > On 2014/04/09 20:20:49, I haz the power (commit-bot) wrote: > > > > Change committed as 262794 > > > > > > FYI, since this change add -B to cflags, it makes icecc compile all files > > > locally (basically breaking it). I don't know any details why. Icecc has a > > > comment that says: > > > > > > /* -B overwrites the path where the compiler finds the assembler. > > > As we don't use that, better force local job. */ > > > > icecc should probably support -B in some way. > > > > > Any known work-around so that -B doesn't get set? > > > > > > We kind of depend on being able to use icecc to get sane compile times, so > at > > > Opera we'll probably have to carry patches locally to revert parts of this > > > patch. > > > > You can do a GYP_DEFINES="linux_use_gold_binary=0" which will prevent the > > addition of -B for using the binutils. > > > > Tim > > This seems to have broken the ARM cross build: > > http://build.chromium.org/p/chromium.fyi/builders/Linux%20ARM%20Cross-Compile > > I guess we were using linux_use_gold_binary and this worked find before because > I guess gold is multi-arch. > But the other binutils in there don't seem to like ARM: > > Assembler messages: > Fatal error: invalid -march= option: `armv7' > > Not sure how to proceed here. We could disable use of gold linker on ARM, but > that seems unfortunate. Would it be possible to split gold from the other > binutils? Or perhaps make the binutil multi-arch? Couple of options here; * You could try "linux_use_gold_flags=1 linux_use_gold_binary=0" combination here? * Setting binutils_dir to the binutils in your cross compiler? We can rebuild binutils to be multiarch enabled, but for it to work properly we'd need to apply a bunch of patches from debian. I actually started work on doing that but stopped. > Also, isn't the linux_use_gold_binary a little miss-named now as it does more > than just use the gold binary now? Yes.
Message was sent while issue was closed.
This commit seems to have caused icecc to stop working for us. Is it a known issue? do we need to do anything special to get icecc working again?
Message was sent while issue was closed.
On 2014/04/15 19:07:44, Chris Dumez wrote: > This commit seems to have caused icecc to stop working for us. Is it a known > issue? do we need to do anything special to get icecc working again? Nevermind, I saw the above comments too late. Using linux_use_gold_flags=0 makes it work for us at Samsung, thanks for the workaround.
Message was sent while issue was closed.
On 2014/04/15 19:17:15, Chris Dumez wrote: > On 2014/04/15 19:07:44, Chris Dumez wrote: > > This commit seems to have caused icecc to stop working for us. Is it a known > > issue? do we need to do anything special to get icecc working again? > > Nevermind, I saw the above comments too late. Using linux_use_gold_flags=0 makes > it work for us at Samsung, thanks for the workaround. Hi folks, We will split linux_use_gold_binary in https://codereview.chromium.org/239163003/ to linux_use_bundled_gold and linux_use_bundled_binutils. Yes, this means one more round of changes for your build systems, but going forward, this lets you better customize your builds, and the variable names make a bit more sense.
Message was sent while issue was closed.
On 2014/04/16 01:26:26, Lei Zhang wrote: > On 2014/04/15 19:17:15, Chris Dumez wrote: > > On 2014/04/15 19:07:44, Chris Dumez wrote: > > > This commit seems to have caused icecc to stop working for us. Is it a known > > > issue? do we need to do anything special to get icecc working again? > > > > Nevermind, I saw the above comments too late. Using linux_use_gold_flags=0 > makes > > it work for us at Samsung, thanks for the workaround. > > Hi folks, > > We will split linux_use_gold_binary in > https://codereview.chromium.org/239163003/ to linux_use_bundled_gold and > linux_use_bundled_binutils. Yes, this means one more round of changes for your > build systems, but going forward, this lets you better customize your builds, > and the variable names make a bit more sense. For the people using icecc, is there a way to actually detect that it is being used (apart from looking at the path of c++)?
Message was sent while issue was closed.
On 2014/04/16 06:56:46, mithro wrote: > For the people using icecc, is there a way to actually detect that it is being > used (apart from looking at the path of c++)? The only other method I can think of would be to set ICECC_DEBUG=YES in the environment and run a test compile- ignore the results but check the output for lines that start with something like "ICECC[16266] 2014-04-22 14:54:56: " with a suitable regex. |
