add a linux_use_debug_fission gyp variable for -gsplit-dwarf

add a linux_use_debug_fission gyp variable for -gsplit-dwarf

Since -gsplit-dwarf breaks some extremely useful build tools (eg ccache and icecc), it deserves its own gyp variable.  This setting replaces the previous hack, setting binutils_version=0.

Background:
ccache: https://bugzilla.samba.org/show_bug.cgi?id=10005
icecc: https://github.com/icecc/icecream/issues/86

BUG=352046
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267381

[Mostyn Bramley-Moore, 2014-04-05 08:58:19]

@Nico: I think this -gsplit-dwarf deserves its own flag, PTAL...

[dshwang, 2014-04-05 11:11:12]

non-owner lgtm.
I currently use binutils_version=0 hack for icecc.
fyi, icecc and ccache gave up fixing debug fission issue;
https://github.com/icecc/icecream/issues/86

if google provides goma as open source, this CL could be reverted :)

[Nico, 2014-04-05 14:08:22]

You can set binutils_version (spelling) to 0 to disable fission if you must.

(At least I do incremental builds more often than full builds, so I'd rather use
this than icecc fwiw.

I saw a patch on the clang mailing list for icecc dwo support, so it looks like
that's being worked on and might be available soon.)

[Mostyn Bramley-Moore, 2014-04-05 14:31:19]

On 2014/04/05 14:08:22, Nico wrote:
> You can set binutils_version (spelling) to 0 to disable fission if you must.

I'm a bit worried that this more general flag (binutils_version) will soon be
used for other things that I don't want to modify- I assume this will prevent us
from using other interesting but non-disruptive linker flags that appear in the
future.  This flag is disruptive, and the binutils_version=0 hack feels bad.

> (At least I do incremental builds more often than full builds, so I'd rather
use
> this than icecc fwiw.
> 
> I saw a patch on the clang mailing list for icecc dwo support, so it looks
like
> that's being worked on and might be available soon.)

That sounds promising, but I (and many other external contributors, I suspect)
also make heavy use of ccache, which is broken by this flag too.

[mithro-old, 2014-04-05 15:00:31]

On 2014/04/05 14:31:19, Mostyn Bramley-Moore wrote:
> On 2014/04/05 14:08:22, Nico wrote:
> > You can set binutils_version (spelling) to 0 to disable fission if you must.
> 
> I'm a bit worried that this more general flag (binutils_version) will soon be
> used for other things that I don't want to modify- I assume this will prevent
us
> from using other interesting but non-disruptive linker flags that appear in
the
> future.  This flag is disruptive, and the binutils_version=0 hack feels bad.
> 
> > (At least I do incremental builds more often than full builds, so I'd rather
> use
> > this than icecc fwiw.
> > 
> > I saw a patch on the clang mailing list for icecc dwo support, so it looks
> like
> > that's being worked on and might be available soon.)
> 
> That sounds promising, but I (and many other external contributors, I suspect)
> also make heavy use of ccache, which is broken by this flag too.

My patch which does something similar is at
https://codereview.chromium.org/203153005/

I believe almost all large C++ projects (and some C projects) will eventually
adopt this flag, so support in things like ccache are very likely -- but it is a
little bit of a chicken and egg issue. 

For example, Mozilla is also starting to use this flag too for the same reason
we want too (https://bugzilla.mozilla.org/show_bug.cgi?id=905646), from that
bug;
> By the way, using -gsplit-dwarf has cut down my link time of libxul from
somewhere near 5 minutes to 1m30s range.

[Mostyn Bramley-Moore, 2014-04-05 16:07:36]

On 2014/04/05 15:00:31, mithro wrote:
> My patch which does something similar is at
> https://codereview.chromium.org/203153005/

My patch has some scoping issues that would need to be fixed, but yours looks OK
to me.  I'd be happy with either one landing.

> I believe almost all large C++ projects (and some C projects) will eventually
> adopt this flag, so support in things like ccache are very likely -- but it is
a
> little bit of a chicken and egg issue. 

I am looking forward to using it, as soon as my tools support it.

[Nico, 2014-04-05 18:33:51]

On Sat, Apr 5, 2014 at 7:31 AM, <mostynb@opera.com> wrote:

> On 2014/04/05 14:08:22, Nico wrote:
>
>> You can set binutils_version (spelling) to 0 to disable fission if you
>> must.
>>
>
> I'm a bit worried that this more general flag (binutils_version) will soon
> be
> used for other things that I don't want to modify- I assume this will
> prevent us
> from using other interesting but non-disruptive linker flags that appear
> in the
> future.


Maybe we can worry about that if that happens? (Who knows, whatever other
feature this might be ending used for might only work with fission – it's
hard to make predicitons.)


>  This flag is disruptive, and the binutils_version=0 hack feels bad.
>
>
>  (At least I do incremental builds more often than full builds, so I'd
>> rather
>>
> use
>
>> this than icecc fwiw.
>>
>
>  I saw a patch on the clang mailing list for icecc dwo support, so it looks
>>
> like
>
>> that's being worked on and might be available soon.)
>>
>
> That sounds promising, but I (and many other external contributors, I
> suspect)
> also make heavy use of ccache, which is broken by this flag too.
>
> https://codereview.chromium.org/226613005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mostyn Bramley-Moore, 2014-04-06 19:30:33]

On 2014/04/05 18:33:51, Nico wrote:
> On Sat, Apr 5, 2014 at 7:31 AM, <mailto:mostynb@opera.com> wrote:
> 
> > On 2014/04/05 14:08:22, Nico wrote:
> >
> >> You can set binutils_version (spelling) to 0 to disable fission if you
> >> must.
> >>
> >
> > I'm a bit worried that this more general flag (binutils_version) will soon
> > be
> > used for other things that I don't want to modify- I assume this will
> > prevent us
> > from using other interesting but non-disruptive linker flags that appear
> > in the
> > future.
> 
> 
> Maybe we can worry about that if that happens? (Who knows, whatever other
> feature this might be ending used for might only work with fission – it's
> hard to make predicitons.)

I can live with the binutils_version hack while it doesn't cause trouble, I
guess.  

Aside: is there a nice way to watch for binutils_version changes in
build/common.gypi (eg using the chromium issue tracker)?

[dshwang, 2014-04-18 18:12:41]

After 9c23369f536c322bff69f24bdc853014b6f7beae, icecc needs more options;
-Dlinux_use_bundled_gold=0 and -Dlinux_use_bundled_binutils=0
In my opinion, use_icecc or something is needed now.

[dshwang, 2014-04-18 18:13:22]

On 2014/04/18 18:12:41, dshwang_afk wrote:
> After 9c23369f536c322bff69f24bdc853014b6f7beae,
Above commit's CL is https://codereview.chromium.org/196573022

[dshwang, 2014-04-23 14:20:25]

binutils_version began to be used in other places;
https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...
my binutils_version=222 hack for icecc made me in trouble;
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/1oqi_Z76p6E

IMO, it's time to introduce use_icecc or something.

[Mostyn Bramley-Moore, 2014-04-25 21:28:08]

Updated this CL based on discussions in
https://codereview.chromium.org/249443002/

@dshwang, Chris: does this work for your build setup too?

@Lei Zhang, Nico: PTAL.

[Mostyn Bramley-Moore, 2014-04-25 21:28:14]

https://codereview.chromium.org/226613005/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/226613005/diff/60001/build/common.gypi#newcod...
build/common.gypi:761: ['OS=="linux" and target_arch=="x64" and chromeos==0', {
I'm not sure if this must be disabled for chromeos, can someone suggest a
chromeos reviewer?

[dshwang, 2014-04-26 15:21:19]

> @dshwang, Chris: does this work for your build setup too?

Works well. Thank you.

https://codereview.chromium.org/226613005/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/226613005/diff/60001/build/common.gypi#newcod...
build/common.gypi:761: ['OS=="linux" and target_arch=="x64" and chromeos==0', {
On 2014/04/25 21:28:14, Mostyn Bramley-Moore wrote:
> I'm not sure if this must be disabled for chromeos, can someone suggest a
> chromeos reviewer?

IMO chromeos is not needed. Above chromeos buildbot failure is due to thread
option, not debug fission.

https://codereview.chromium.org/226613005/diff/60001/build/common.gypi#newcod...
build/common.gypi:3905: ['binutils_version>=223', {
This CL is needed because of this line. We can not use binutils_version=0
workaround anymore.

[dshwang, 2014-04-26 15:26:37]

IMO it's better to point out links in description, which both build tools give
up supporting debug fission.
ccache: https://bugzilla.samba.org/show_bug.cgi?id=10005#c3
icecc: https://github.com/icecc/icecream/issues/86

[Mostyn Bramley-Moore, 2014-04-27 17:59:48]

Updated the description.

https://codereview.chromium.org/226613005/diff/60001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/226613005/diff/60001/build/common.gypi#newcod...
build/common.gypi:761: ['OS=="linux" and target_arch=="x64" and chromeos==0', {
On 2014/04/26 15:21:19, dshwang wrote:
> On 2014/04/25 21:28:14, Mostyn Bramley-Moore wrote:
> > I'm not sure if this must be disabled for chromeos, can someone suggest a
> > chromeos reviewer?
> 
> IMO chromeos is not needed. Above chromeos buildbot failure is due to thread
> option, not debug fission.

OK- removed it.

https://codereview.chromium.org/226613005/diff/60001/build/common.gypi#newcod...
build/common.gypi:3905: ['binutils_version>=223', {
On 2014/04/26 15:21:19, dshwang wrote:
> This CL is needed because of this line. We can not use binutils_version=0
> workaround anymore.

I'm not sure if it's worth leaving a comment here, but I will add it to the CL
description instead.

[dshwang, 2014-04-27 18:10:19]

On 2014/04/27 17:59:48, Mostyn Bramley-Moore wrote:
>
https://codereview.chromium.org/226613005/diff/60001/build/common.gypi#newcod...
> build/common.gypi:3905: ['binutils_version>=223', {
> On 2014/04/26 15:21:19, dshwang wrote:
> > This CL is needed because of this line. We can not use binutils_version=0
> > workaround anymore.
> 
> I'm not sure if it's worth leaving a comment here, but I will add it to the CL
> description instead.

of course, it's not needed. I just wrote information.

[Lei Zhang, 2014-04-29 07:28:14]

IMO we should have a variable like this. It is much clearer than setting
binutils_version=0. Plus, binutils_version is helpful on systems with newer
versions of gold for automatically passing --disable-new-dtags, so currently
setting it to 0 means disabling debug fission, but not passing the needed
--disable-new-dtags flag.

https://codereview.chromium.org/226613005/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/226613005/diff/80001/build/common.gypi#newcod...
build/common.gypi:3213: ['linux_use_debug_fission==1 and (clang==1 or
gcc_version>=48) and binutils_version>=223', {
Shouldn't the linux_use_gold_flags==1 stay, since --gdb-index is a gold flag?

[Mostyn Bramley-Moore, 2014-04-29 07:37:31]

https://codereview.chromium.org/226613005/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/226613005/diff/80001/build/common.gypi#newcod...
build/common.gypi:3213: ['linux_use_debug_fission==1 and (clang==1 or
gcc_version>=48) and binutils_version>=223', {
On 2014/04/29 07:28:15, Lei Zhang wrote:
> Shouldn't the linux_use_gold_flags==1 stay, since --gdb-index is a gold flag?

Right.  Should we do it here, or where linux_use_debug_fission is defined?  I
think I prefer the latter.

[Lei Zhang, 2014-04-29 08:18:24]

https://codereview.chromium.org/226613005/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/226613005/diff/80001/build/common.gypi#newcod...
build/common.gypi:3213: ['linux_use_debug_fission==1 and (clang==1 or
gcc_version>=48) and binutils_version>=223', {
On 2014/04/29 07:37:32, Mostyn Bramley-Moore wrote:
> On 2014/04/29 07:28:15, Lei Zhang wrote:
> > Shouldn't the linux_use_gold_flags==1 stay, since --gdb-index is a gold
flag?
> 
> Right.  Should we do it here, or where linux_use_debug_fission is defined?  I
> think I prefer the latter.

linux_use_gold_flags is defined right above linux_use_debug_fission in the same
variable scope. I think linux_use_gold_flags needs to be one variable scope up
from linux_use_debug_fission for linux_use_debug_fission to use it as part of a
conditional.

[Mostyn Bramley-Moore, 2014-04-29 08:56:22]

https://codereview.chromium.org/226613005/diff/80001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/226613005/diff/80001/build/common.gypi#newcod...
build/common.gypi:3213: ['linux_use_debug_fission==1 and (clang==1 or
gcc_version>=48) and binutils_version>=223', {
On 2014/04/29 08:18:24, Lei Zhang wrote:
> On 2014/04/29 07:37:32, Mostyn Bramley-Moore wrote:
> > On 2014/04/29 07:28:15, Lei Zhang wrote:
> > > Shouldn't the linux_use_gold_flags==1 stay, since --gdb-index is a gold
> flag?
> > 
> > Right.  Should we do it here, or where linux_use_debug_fission is defined? 
I
> > think I prefer the latter.
> 
> linux_use_gold_flags is defined right above linux_use_debug_fission in the
same
> variable scope. I think linux_use_gold_flags needs to be one variable scope up
> from linux_use_debug_fission for linux_use_debug_fission to use it as part of
a
> conditional.

I'll add the check here for now then, it's simpler.

(Aside: does gn use similar scoping rules to gyp?  I hope not...)

[Nico, 2014-04-30 19:52:11]

If thestig and mithro are happy with this, lgtm

[Lei Zhang, 2014-04-30 19:54:38]

lgtm

[Mostyn Bramley-Moore, 2014-04-30 19:59:18]

On 2014/04/30 19:52:11, Nico wrote:
> If thestig and mithro are happy with this, lgtm

Since mithro submitted a similar (but now out of date) patch in
https://codereview.chromium.org/203153005/, I will assume this is OK with him
and send to the CQ.

[Mostyn Bramley-Moore, 2014-04-30 19:59:32]

The CQ bit was checked by mostynb@opera.com

[commit-bot: I haz the power, 2014-04-30 20:02:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/226613005/90001

[commit-bot: I haz the power, 2014-04-30 20:55:57]

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

[commit-bot: I haz the power, 2014-04-30 20:55:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Mostyn Bramley-Moore, 2014-04-30 20:59:48]

The CQ bit was checked by mostynb@opera.com

[commit-bot: I haz the power, 2014-04-30 21:01:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mostynb@opera.com/226613005/90001

[commit-bot: I haz the power, 2014-04-30 23:42:36]

Change committed as 267381