Closed Bug 904979 Opened 6 years ago Closed 6 years ago

Add build option for rel-eng type builds, and make default builds enable or disable appropriate toolchain features

Categories

(Firefox Build System :: General, defect)

24 Branch
x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

As discussed on irc a few days ago, we're starting to need to differentiate release engineering-type builds from local developer builds.
I'd like to avoid non-mozilla releng-type builds not setting this option. I'm wondering what would be appropriate for this. I've been pondering erroring out on make package and make install for browser builds when DEVELOPER_OPTIONS is set, what do you think?
Attachment #789973 - Flags: review?(ted)
Assignee: nobody → mh+mozilla
(In reply to Mike Hommey [:glandium] from comment #2)
> Created attachment 789974 [details] [diff] [review]
> Use -gsplit-dwarf when supported for local builds

FWIW, this made a clobber build on my machine go from 1740s to 1362s, which is 21.7% build time improvement on a clobber. Build time in toolkit/library (which is mostly linker time) went from 85s to 11s. That's with GNU ld.
Comment on attachment 789974 [details] [diff] [review]
Use -gsplit-dwarf when supported for local builds

No wonder the build was so much faster... clang silently ignores -gsplit-dwarf... and doesn't create debug symbols at all.
Attachment #789974 - Flags: review?(ted)
(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 789974 [details] [diff] [review]
> Use -gsplit-dwarf when supported for local builds
> 
> No wonder the build was so much faster... clang silently ignores
> -gsplit-dwarf... and doesn't create debug symbols at all.

clang 3.2. clang 3.4 does recognize -gsplit-dwarf.
This gets me down to 13s of link time with GNU ld, and 1546s for the whole build with clang 3.4, although the 1740s were with clang 3.2, and it's not clear what makes most of the difference. The GNU ld difference is real, though.
Attachment #790023 - Flags: review?(ted)
Attachment #789974 - Attachment is obsolete: true
While being more aggressive, let's use gold when it's available. I've been using this trick now and then for my own builds.
Attachment #790036 - Flags: review?(ted)
Together with the other patches, this gets the actual linking time of libxul.so (as run manually by copy/pasting the command make would run) down to 3.5s on my machine, instead of ~1 minute.
Attachment #790037 - Flags: review?(ted)
(In reply to Mike Hommey [:glandium] from comment #1)
> I'd like to avoid non-mozilla releng-type builds not setting this option.
> I'm wondering what would be appropriate for this. I've been pondering
> erroring out on make package and make install for browser builds when
> DEVELOPER_OPTIONS is set, what do you think?

That seems kind of gross, there are plenty of reasons for people to want to package their local build without caring if it matches other things. We could make builds with MOZILLA_OFFICIAL set but not --enable-release error out (although we could also be leveraging --enable-release to set MOZILLA_OFFICIAL).
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> That seems kind of gross, there are plenty of reasons for people to want to
> package their local build without caring if it matches other things. We
> could make builds with MOZILLA_OFFICIAL set but not --enable-release error
> out.

I was under the impression that MOZILLA_OFFICIAL was not supposed to be used for redistribution, although in practice, it is. So I guess that's good enough.

> (although we could also be leveraging --enable-release to set
> MOZILLA_OFFICIAL).

That doesn't seem antagonistic.
This adds a failure to make package and make install when MOZILLA_OFFICIAL is set and --enable-release wasn't given. This may break c-c if they don't include mozconfig.common.
I'm not adding MOZILLA_OFFICIAL with --enable-release because non-official builds may still want to use --enable-release, and I'm not sure what MOZILLA_OFFICIAL does these days. This can be figured in a followup.
Attachment #790155 - Flags: review?(ted)
Attachment #789973 - Attachment is obsolete: true
Attachment #789973 - Flags: review?(ted)
You could just make the build error in configure if you --enable-release without MOZILLA_OFFICIAL.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> You could just make the build error in configure if you --enable-release
> without MOZILLA_OFFICIAL.

The point is to avoid MOZ_OFFICIAL builds without --enable-release, not the other way around, but yeah, that could be done in configure.
Actually, it might be better *not* in configure, because i suspect some developers are using MOZILLA_OFFICIAL to enable some of those in local builds, and that would block them. They are however less likely to do make package. And if they really need to, they can override one of the variables at make package time. It's harder to workaround if it fails in configure.
Comment on attachment 790155 [details] [diff] [review]
Add build option for rel-eng type builds

Review of attachment 790155 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/compiler-opts.m4
@@ +82,5 @@
>  AC_DEFUN([MOZ_COMPILER_OPTS],
>  [
> +  DEVELOPER_OPTIONS=1
> +  MOZ_ARG_ENABLE_BOOL(release,
> +  [  --enable-release       Build with more conservative, release engineering-oriented options.],

Might want to mention that it is likely to make builds slower.

::: build/mozconfig.common
@@ +1,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +# Common mozconfig for all release engineering builds

I would probably say "official builds".
Attachment #790155 - Flags: review?(ted) → review+
Comment on attachment 790037 [details] [diff] [review]
Disable ICF and dead code removal on local builds

Review of attachment 790037 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/compiler-opts.m4
@@ +237,5 @@
>  
> +if test "$GNU_CC" -a -z "$DEVELOPER_OPTIONS"; then
> +    CFLAGS="$CFLAGS -ffunction-sections -fdata-sections"
> +    CXXFLAGS="$CXXFLAGS -ffunction-sections -fdata-sections"
> +fi

Can you not just stick this block inside the existing GNU_CC block below?
Attachment #790037 - Flags: review?(ted) → review+
Comment on attachment 790036 [details] [diff] [review]
Force use gold, if possible, when the default linker is BFD ld, for local builds.

Review of attachment 790036 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/compiler-opts.m4
@@ +225,5 @@
> +        esac
> +        if test -n "$GOLD"; then
> +            mkdir -p $_objdir/build/unix/gold
> +            ln -s "$GOLD" $_objdir/build/unix/gold/ld
> +            if $CC -B $_objdir/build/unix/gold -Wl,--version 2>&1 | grep -q "GNU gold"; then

This is a little wacky, but I guess there's no better way to do this.

@@ +226,5 @@
> +        if test -n "$GOLD"; then
> +            mkdir -p $_objdir/build/unix/gold
> +            ln -s "$GOLD" $_objdir/build/unix/gold/ld
> +            if $CC -B $_objdir/build/unix/gold -Wl,--version 2>&1 | grep -q "GNU gold"; then
> +                LDFLAGS="$LDFLAGS -B $_objdir/build/unix/gold"

What's the failure mode here? Just somehow this setup doesn't work?
Attachment #790036 - Flags: review?(ted) → review+
Comment on attachment 790023 [details] [diff] [review]
Use -gsplit-dwarf when supported for local builds

Review of attachment 790023 [details] [diff] [review]:
-----------------------------------------------------------------

Not totally happy with this patch yet.

::: build/autoconf/ccache.m4
@@ +35,5 @@
>  fi
>  
> +dnl -gsplit-dwarf isn't supported by ccache, so it's dangerous to use both
> +if test -n "$MOZ_USING_CCACHE" && echo " $MOZ_DEBUG_FLAGS " | grep -q " -gsplit-dwarf "; then
> +    AC_MSG_ERROR([ccache does not support -gsplit-dwarf. Please choose to either build --without-ccache, or force not to use split-dwarf with --enable-debug-symbols=-g])

Since ccache isn't the default, it's probably more helpful to say "don't use --with-ccache".

This is an unfortunate tradeoff. Is the win from -gsplit-dwarf worth the cost of disabling ccache? Also, I'm a little worried that we're basically making --with-ccache useless.
Attachment #790023 - Flags: review?(ted)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #16)
> > +# Common mozconfig for all release engineering builds
> 
> I would probably say "official builds".

Considering this is something things like icecat, iceweasel or palemoon should be using, "official builds" seems charged. Releng is not really clear but still better imho.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #18)
> > +                LDFLAGS="$LDFLAGS -B $_objdir/build/unix/gold"
> 
> What's the failure mode here? Just somehow this setup doesn't work?

Either using gold that way works and we use the flag, or it doesn't and we use BFD ld.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #19)
> This is an unfortunate tradeoff. Is the win from -gsplit-dwarf worth the
> cost of disabling ccache? Also, I'm a little worried that we're basically
> making --with-ccache useless.

It depends what the usecase is. For incremental builds involving little changes, split-dwarf is a clear win. That's what cuts most of the linking time, and when you're iterating on small changes and relinking libxul a lot, it matters much more than ccache.

Note that a mozilla contributor has submitted patches to https://bugzilla.samba.org/show_bug.cgi?id=10005 . I don't know how well they work, but we could build and provide binaries, and add a test whether ccache supports the dwo files.
(In reply to Mike Hommey [:glandium] from comment #20)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #16)
> > > +# Common mozconfig for all release engineering builds
> > 
> > I would probably say "official builds".
> 
> Considering this is something things like icecat, iceweasel or palemoon
> should be using, "official builds" seems charged. Releng is not really clear
> but still better imho.

Ah, I realized this was about the mozconfig comment. Yeah, official builds would work, there.
Blocks: 905646
I'm going to land the reviewed patches and keep the -gsplit-dwarf work for a followup (already cloned as bug 905646)
So, does this mean that building B2G is broken by default now as MOZILLA_OFFICIAL is enabled by default there (as without it, crash reporter will not work) but --enable-release is probably not set by default? Sounds messy to me...
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #26)
> So, does this mean that building B2G is broken by default now as
> MOZILLA_OFFICIAL is enabled by default there (as without it, crash reporter
> will not work)

That's not entirely true. In fact, I'd argue MOZILLA_OFFICIAL is the wrong bit for that.

> but --enable-release is probably not set by default? Sounds
> messy to me...

--enable-release is set on all releng builds through mozconfig.
Note that if something is broken as a result of this landing, followup bugs can be filed.
(In reply to Mike Hommey [:glandium] from comment #27)
> (In reply to Robert Kaiser (:kairo@mozilla.com) from comment #26)
> > but --enable-release is probably not set by default? Sounds
> > messy to me...
> 
> --enable-release is set on all releng builds through mozconfig.

Except builds/compiles that don't use a mozconfig yet, (e.g. l10n dep builds, iirc call configure directly passing in specific arguments, and I have no idea what DXR or spidermonkey do top of my head, nevermind b2g builds)
(In reply to Mike Hommey [:glandium] from comment #27)
> (In reply to Robert Kaiser (:kairo@mozilla.com) from comment #26)
> > So, does this mean that building B2G is broken by default now as
> > MOZILLA_OFFICIAL is enabled by default there (as without it, crash reporter
> > will not work)
> 
> That's not entirely true. In fact, I'd argue MOZILLA_OFFICIAL is the wrong
> bit for that.

Well, it has been the bit for that for ages (the reason was that back in the days, it was decided that crash reports from unofficial builds would be noise and unwanted).

> > but --enable-release is probably not set by default? Sounds
> > messy to me...
> 
> --enable-release is set on all releng builds through mozconfig.

Well, all official B2G builds are not created by releng but by manufacturers. I guess this patch is why Geeksphone's Nightly builds for today weren't produced.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #30)
> Well, it has been the bit for that for ages (the reason was that back in the
> days, it was decided that crash reports from unofficial builds would be
> noise and unwanted).

I meant that MOZILLA_OFFICIAL is the wrong bit for b2g to enable the crash reporter. MOZILLA_OFFICIAL is not the only way to enable the crash reporter.
That being said, comment 28 still applies.
And I said that MOZILLA_OFFICIAL has been required to enable crash reporter for ages. The more I read this and think about it, the more I actually think this bug should be backed out on the basis of that.
Depends on: 906260
Blocks: 906225
No longer depends on: 906225
So is there a way to disable -gsplit-dwarf easily?  I depend on being able to rsync dist/bin from a debug build to another machine and then debug the result, but I'd rather not have to track whatever other changes we make to debugging options.
Flags: needinfo?(mh+mozilla)
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #34)
> So is there a way to disable -gsplit-dwarf easily?  I depend on being able
> to rsync dist/bin from a debug build to another machine and then debug the
> result, but I'd rather not have to track whatever other changes we make to
> debugging options.

-gsplit-dwarf is not there yet, it's moved to bug 905646.
Flags: needinfo?(mh+mozilla)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.