1073003 - Enable warnings-as-errors on B2G

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Botond Ballo [:botond] [away until July 9]]

I'd like to be able to do B2G builds with warnings-as-errors enabled, so that my local B2G build can catch these, rather than relying on a desktop build or the Try server.

As the first step, I'd like to add an option to .userconfig to enable warnings-as-errors.

Comment 1

[Botond Ballo [:botond] [away until July 9]]

Attachment: bug1073003.patch

Comment 2

[Michael Wu [:mwu]]

On B2G, I think we should just enable this on every device/base version that it would work on. Unlike desktop, the compiler that's used is always the same for a given device, so random bustage on different build systems is unlikely as long as you've tested it.

Comment 3

[Botond Ballo [:botond] [away until July 9]]

OK, I'll re-flag for review once bug 1073081 is fixed.

Comment 5

[Botond Ballo [:botond] [away until July 9]]

Now that bug 1073081 has landed, the next step is to flip the switch.

Try push with warnings-as-errors hardcoded in configure.in, to see if everything still builds: https://tbpl.mozilla.org/?tree=Try&rev=745826dbf19d

(The actual patch to flip the switch will be to gonk-misc/default-gecko-config, but I don't know how to do a Try push that picks up that change.)

Comment 6

[Botond Ballo [:botond] [away until July 9]]

(In reply to Botond Ballo [:botond] from comment #5)
> Try push with warnings-as-errors hardcoded in configure.in, to see if
> everything still builds: https://tbpl.mozilla.org/?tree=Try&rev=745826dbf19d

Things are building!

Comment 7

[Botond Ballo [:botond] [away until July 9]]

Attachment: Enable warnings-as-errors

Comment 8

[Michael Wu [:mwu]]

Is it possible to stick it in http://hg.mozilla.org/mozilla-central/file/cb8ad2251c09/b2g/confvars.sh ? I try not to put new things in default-gecko-config now. I think you can stick it in a "$OS_TARGET" = "Android" test so it only builds on Gonk.

Comment 9

[Botond Ballo [:botond] [away until July 9]]

Turns out the tree I was basing my Try push in comment 5 on was out of date, and changes to trunk since I last pulled my tree for fixing bug 1073081 introduced a few new Werrors. Patches coming up.

Comment 10

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 1 - Fix more -Wreorder warnings

Comment 11

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 2 - Fix more warnings about macro redefinitions by changing macros to constants (yay!)

Comment 12

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 3 - Fix more warnings about macro redefinitions by renaming macros

Comment 13

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 4 - Fix more -Wunused-variable warnings

Comment 14

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 5 - Fix -Wformat warnings

Comment 15

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 6 - Fix more -Wsign-compare warnings

Comment 16

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 7 - Enable warnings-as-errors on B2G

Comment 17

[Botond Ballo [:botond] [away until July 9]]

Build-only Try push: https://tbpl.mozilla.org/?tree=Try&rev=14dd42984c42

This time, I'm going to land the fixes and the flip-the-switch patch together, so there should be no further whac-a-mole!

Comment 18

[Botond Ballo [:botond] [away until July 9]]

(In reply to Botond Ballo [:botond] from comment #17)
> Build-only Try push: https://tbpl.mozilla.org/?tree=Try&rev=14dd42984c42

Missed a couple of Werrors in opt builds.

Comment 19

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 4 - Fix more -Wunused-variable and -Wunused-but-set-variable warnings

Comment 20

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 6.5 - Disable -Wattributes in media/webrtc/signaling

Comment 21

[Botond Ballo [:botond] [away until July 9]]

Comment on attachment 8538340 [details] [diff] [review]
Part 6.5 - Disable -Wattributes in media/webrtc/signaling

Hope I did this right, it seems this part of the tree uses a different build system than other parts.

Comment 22

[Botond Ballo [:botond] [away until July 9]]

(In reply to Botond Ballo [:botond] from comment #17)
> Build-only Try push: https://tbpl.mozilla.org/?tree=Try&rev=14dd42984c42

Also, we can't turn on warnings-as-errors on builds with gcc 4.4 (i.e. ICS builds), due to bug 915555.

Is there a way to turn on warnings-as-errors for builds with gcc >= 4.5 only?

(My Try push in comment 5 clearly used an incorrect patch for enabling warnings-as-errors, as it didn't catch any of these things!)

Comment 23

[Botond Ballo [:botond] [away until July 9]]

Comment on attachment 8538340 [details] [diff] [review]
Part 6.5 - Disable -Wattributes in media/webrtc/signaling

(In reply to Botond Ballo [:botond] from comment #21)
> Comment on attachment 8538340 [details] [diff] [review]
> Part 6.5 - Disable -Wattributes in media/webrtc/signaling
> 
> Hope I did this right, it seems this part of the tree uses a different build
> system than other parts.

Nope - I failed to add a condition to only set '-Wno-error=attributes' on gcc-like compilers, causing MSVC to choke on it (https://tbpl.mozilla.org/?tree=Try&rev=3e6d19324403).

How do I express such a condition in a .gyp file? I had a quick look through the .gyp files in the tree and didn't see a similar condition elsewhere (but perhaps I just don't know what to look for).

Comment 24

[Michael Wu [:mwu]]

(In reply to Botond Ballo [:botond] from comment #22)
> Is there a way to turn on warnings-as-errors for builds with gcc >= 4.5 only?
> 

The closest thing might be to exclude ICS builds by checking $PLATFORM_SDK_VERSION -gt 15 .

Comment 25

[(no longer active)]

Comment on attachment 8538092 [details] [diff] [review]
Part 5 - Fix -Wformat warnings

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +525,5 @@
>      }
>      MOZ_ASSERT(timestamp >= 0 && renderTimeMs >= 0);
>  
>      CODEC_LOGD("Decoder NewFrame: %d bytes, %dx%d, timestamp %lld, renderTimeMs %lld",
> +               buffer->GetSize().width * buffer->GetSize().height, 

Trailing whitespace!

But more importantly, is this correct?  Maybe there should also be a constant multiplier such as sizeof(TypeOfStuffInTheBuffer)?  I think you should ask someone familiar with this code to review this patch.

Comment 26

[(no longer active)]

(In reply to Michael Wu [:mwu] from comment #2)
> On B2G, I think we should just enable this on every device/base version that
> it would work on. Unlike desktop, the compiler that's used is always the
> same for a given device, so random bustage on different build systems is
> unlikely as long as you've tested it.

So this is not what the current patch does, it enables warnings as errors unconditionally, which means that if you test against device X, you may burn the build on device Y.  Is this intended?

Comment 27

[Michael Wu [:mwu]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #26)
> So this is not what the current patch does, it enables warnings as errors
> unconditionally, which means that if you test against device X, you may burn
> the build on device Y.  Is this intended?

Yeah I think enabling it everywhere but on ICS builds is fine. The reason is that for a given base version (ICS/JB/KK), every device uses the same toolchain. It's possible that we might include some system/device header that's different on a particular device that causes a warning, but it seems unlikely.

Comment 28

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 7 - Enable warnings-as-errors on B2G (other than ICS)

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8538092 [details] [diff] [review]
Part 5 - Fix -Wformat warnings

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

::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
@@ +525,5 @@
>      }
>      MOZ_ASSERT(timestamp >= 0 && renderTimeMs >= 0);
>  
>      CODEC_LOGD("Decoder NewFrame: %d bytes, %dx%d, timestamp %lld, renderTimeMs %lld",
> +               buffer->GetSize().width * buffer->GetSize().height, 

I seriously doubt this is correct - byte sizes of gfx buffers are rarely width*height.  a) a pixel is often 4 (or maybe 3) bytes in RGB-land, b) in YUVland, a pixel is often 1.5 bytes c) alignment matters for YUV (odd widths and heights often cause rounding up - but only of the chroma component

Comment 30

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 5 - Fix -Wformat warnings

(In reply to Randell Jesup [:jesup] from comment #29)
> Comment on attachment 8538092 [details] [diff] [review]
> Part 5 - Fix -Wformat warnings
> 
> Review of attachment 8538092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/media-conduit/WebrtcOMXH264VideoCodec.cpp
> @@ +525,5 @@
> >      }
> >      MOZ_ASSERT(timestamp >= 0 && renderTimeMs >= 0);
> >  
> >      CODEC_LOGD("Decoder NewFrame: %d bytes, %dx%d, timestamp %lld, renderTimeMs %lld",
> > +               buffer->GetSize().width * buffer->GetSize().height, 
> 
> I seriously doubt this is correct - byte sizes of gfx buffers are rarely
> width*height.  a) a pixel is often 4 (or maybe 3) bytes in RGB-land, b) in
> YUVland, a pixel is often 1.5 bytes c) alignment matters for YUV (odd widths
> and heights often cause rounding up - but only of the chroma component

As discussed on IRC, the size of the buffer in bytes is not straightforward to obtain, and we are already printing the dimensions, so I just updated the patch to remove the bytes part.

Comment 31

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 6.5 - -Wattributes in media/webrtc/signaling

(In reply to Botond Ballo [:botond] from comment #23)
> Comment on attachment 8538340 [details] [diff] [review]
> Part 6.5 - Disable -Wattributes in media/webrtc/signaling
> 
> (In reply to Botond Ballo [:botond] from comment #21)
> > Comment on attachment 8538340 [details] [diff] [review]
> > Part 6.5 - Disable -Wattributes in media/webrtc/signaling
> > 
> > Hope I did this right, it seems this part of the tree uses a different build
> > system than other parts.
> 
> Nope - I failed to add a condition to only set '-Wno-error=attributes' on
> gcc-like compilers, causing MSVC to choke on it
> (https://tbpl.mozilla.org/?tree=Try&rev=3e6d19324403).
> 
> How do I express such a condition in a .gyp file? I had a quick look through
> the .gyp files in the tree and didn't see a similar condition elsewhere (but
> perhaps I just don't know what to look for).

Took a stab at this. Upon closer inspection, the .gyp files seem to be making decisions based on platforms rather than compilers, so I conditioned the flag on the platform not being Windows.

Comment 32

[Botond Ballo [:botond] [away until July 9]]

And one more build-only Try push: https://tbpl.mozilla.org/?tree=Try&rev=5347a4cf514

Comment 33

[(no longer active)]

Comment on attachment 8539360 [details] [diff] [review]
Part 6.5 - -Wattributes in media/webrtc/signaling

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

This looks good, but this is jesup's land.

Comment 34

[Botond Ballo [:botond] [away until July 9]]

(In reply to Botond Ballo [:botond] from comment #32)
> And one more build-only Try push:
> https://tbpl.mozilla.org/?tree=Try&rev=5347a4cf514

All clean!

Comment 35

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8539360 [details] [diff] [review]
Part 6.5 - -Wattributes in media/webrtc/signaling

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

::: media/webrtc/signaling/signaling.gyp
@@ +262,5 @@
> +        ['OS!="win"', {
> +          'cflags_mozilla': [
> +            # This warning complains about important MOZ_EXPORT attributes
> +            # on forward declarations for Android API types.
> +            '-Wno-error=attributes',

Something is confused here - is this for Android, or is this for everything but Windows (including B2G)?  Also, the description here is a bit confusing (perhaps the reason for my question)

Comment 36

[Botond Ballo [:botond] [away until July 9]]

(In reply to Randell Jesup [:jesup] from comment #35)
> Comment on attachment 8539360 [details] [diff] [review]
> Part 6.5 - -Wattributes in media/webrtc/signaling
> 
> Review of attachment 8539360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/signaling.gyp
> @@ +262,5 @@
> > +        ['OS!="win"', {
> > +          'cflags_mozilla': [
> > +            # This warning complains about important MOZ_EXPORT attributes
> > +            # on forward declarations for Android API types.
> > +            '-Wno-error=attributes',
> 
> Something is confused here - is this for Android, or is this for everything
> but Windows (including B2G)?  Also, the description here is a bit confusing
> (perhaps the reason for my question)

Hmm, good point. My original intention (and what I've done for similar warnings in bug 1073081) was to disable the warning in a directory for all gcc-like compilers (which are the ones that give this warning), and here I tried to approximate "all gcc-like compilers" by "not Windows". However, since the warning is - I believe - specific to Android API types, it should be sufficient to disable it on Android only. Let me try that.

Comment 37

[Botond Ballo [:botond] [away until July 9]]

(In reply to Randell Jesup [:jesup] from comment #35)
> Also, the description here is a bit confusing
> (perhaps the reason for my question)

I explain the reason we need to disable the warning in more detail here: https://bugzilla.mozilla.org/show_bug.cgi?id=1073081#c44.

I'm open to suggestions for making the comment in the patch more clear.

Comment 38

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 6.5 - Disable -Wattributes in media/webrtc/signaling on Android platforms

Updated patch to disable the warning on Android platforms only.

Build-only Try push: https://tbpl.mozilla.org/?tree=Try&rev=be0accfdeed8

Comment 39

[(no longer active)]

(In reply to Botond Ballo [:botond] from comment #36)
> Hmm, good point. My original intention (and what I've done for similar
> warnings in bug 1073081) was to disable the warning in a directory for all
> gcc-like compilers (which are the ones that give this warning), and here I
> tried to approximate "all gcc-like compilers" by "not Windows". However,
> since the warning is - I believe - specific to Android API types, it should
> be sufficient to disable it on Android only. Let me try that.

Also note that there is a mingw port of Firefox on Windows that is actively maintained.

Comment 40

[Botond Ballo [:botond] [away until July 9]]

(In reply to Botond Ballo [:botond] from comment #38)
> Created attachment 8539555 [details] [diff] [review]
> Part 6.5 - Disable -Wattributes in media/webrtc/signaling on Android
> platforms
> 
> Updated patch to disable the warning on Android platforms only.
> 
> Build-only Try push: https://tbpl.mozilla.org/?tree=Try&rev=be0accfdeed8

That didn't seem to work. The "B2G Emulator KK" builds are failing because of that warning, suggesting that the 'OS=="android"' check isn't true there (I checked the build log and the warning flag does not make it to the compiler invocation command.) Randell, do you know if this is expected?

Comment 41

[Botond Ballo [:botond] [away until July 9]]

Perhaps B2G emulators are considered 'linux' rather than 'android'?

Trying with '(OS=="linux") or (OS=="android")': https://tbpl.mozilla.org/?tree=Try&rev=d06ab87ce894

Comment 42

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Botond Ballo [:botond] from comment #41)
> Perhaps B2G emulators are considered 'linux' rather than 'android'?
> 
> Trying with '(OS=="linux") or (OS=="android")':
> https://tbpl.mozilla.org/?tree=Try&rev=d06ab87ce894

'OS=="android" or moz_widget_toolkit_gonk==1

Comment 43

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 7 - Disable -Wattributes in media/webrtc/signaling on Android and B2G platforms

(In reply to Randell Jesup [:jesup] from comment #42)
> 'OS=="android" or moz_widget_toolkit_gonk==1

Thanks!

https://tbpl.mozilla.org/?tree=Try&rev=1ef151c0ec1a

Comment 44

[Botond Ballo [:botond] [away until July 9]]

Ehsan was of the opinion that :jesup's review is sufficient for the Part 6.5 patch, and that it's better not to sit on these patches, so I went ahead and landed them:

https://hg.mozilla.org/integration/mozilla-inbound/rev/39853e6515eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f6693a2f21b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a12d10d57fd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/07b42d1d60aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/d778ea49056a
https://hg.mozilla.org/integration/mozilla-inbound/rev/033bfc2c3483
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ef52a09f98
https://hg.mozilla.org/integration/mozilla-inbound/rev/42e5e10c44f5

Comment 45

[Botond Ballo [:botond] [away until July 9]]

I had to make a few additional bustage fixes to fix Werrors that seem to have appeared since my patch had a clean Try push on Friday:

https://hg.mozilla.org/integration/mozilla-inbound/rev/aef3ce3b89b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d81c5b78567
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0bb596da2c

I realize the last one ended up a bit on the large side for a bustage fix. My options were to back out everything, or to press on until things compiled, and given how challenging it's been to catch up fixing Werrors in new code people are landing, I chose to press on.

I will split the last patch up by warning type and post its parts for post-humous review.

Comment 46

[Botond Ballo [:botond] [away until July 9]]

Attachment: Remove a spurious line added to a moz.build file

I accidentally added an extra line to a moz.build file while writing my bustage patch. Posting a follow-up to remove it.

Sorry about that!

Comment 47

[Botond Ballo [:botond] [away until July 9]]

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/4902a8efe178

Comment 48

[Botond Ballo [:botond] [away until July 9]]

Comment on attachment 8539000 [details] [diff] [review]
Part 7 - Enable warnings-as-errors on B2G (other than ICS)

I'm also putting up the flip-the-switch patch for re-review, as :glandium pointed out that it should have properly been reviewed by a build peer. Apologies for that as well.

Comment 49

[Mike Hommey [:glandium]]

Comment on attachment 8539000 [details] [diff] [review]
Part 7 - Enable warnings-as-errors on B2G (other than ICS)

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

::: b2g/confvars.sh
@@ +58,5 @@
>  
>  if test "$OS_TARGET" = "Android"; then
>  MOZ_NUWA_PROCESS=1
>  MOZ_B2G_LOADER=1
> +# Warnings-as-errors cannot be enabled on ICS builds due to bug 915555

Which says it's a gcc version problem, not a platform sdk version problem. So the test should be against GCC_VERSION or GCC_MAJOR_VERSION and GCC_MINOR_VERSION.

Note that normally, we don't enable this in confvars.sh, but in the case of Android/gonk, we can make the case that the toolchain used is much more controlled. This however tends to make trying different toolchain versions more cumbersome.

Comment 50

[Carsten Book [:Tomcat]]

sorry this didn't work out with the checkins and followup. Inbound had still failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4907940&repo=mozilla-inbound

so backed out all the csets in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c18ab3671faf

Comment 51

[Botond Ballo [:botond] [away until July 9]]

It turns out the reason so many bustage fixes were necessary (comment 45) even though I had a clean Try push (comment 43) was that since that Try push, bug 1110031 has landed, enabling warnings-as-errors in additional directories in the tree, and thus causing my push to be busted due to b2g Werrors in those directories.

I had managed to fix all of these bustages in the pushes in comment 45, except ones that occurred in non-unified builds, which appear to be why the patches were backed out.

Is there a way to trigger non-unified builds in a Try push to avoid this in the future?

Comment 52

[Botond Ballo [:botond] [away until July 9]]

Comment on attachment 8540547 [details] [diff] [review]
Remove a spurious line added to a moz.build file

This patch is no longer needed now that the offending patch which it corrected was backed out.

Comment 53

[Botond Ballo [:botond] [away until July 9]]

(In reply to Botond Ballo [:botond] from comment #45)
> I realize the last one ended up a bit on the large side for a bustage fix.
> My options were to back out everything, or to press on until things
> compiled, and given how challenging it's been to catch up fixing Werrors in
> new code people are landing, I chose to press on.
> 
> I will split the last patch up by warning type and post its parts for
> post-humous review.

As promised, split patches coming up for review (except it's no longer post-humous, as the original patch was backed out).

Comment 54

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 8 - Fix -Wswitch warnings. r=echen

Comment 55

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 9 - Fix more -Wreorder warnings

Comment 56

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 10 - Fix more warnings about macro redefinitions

Comment 57

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 11 - Fix more -Wunused-variable / -Wunused-but-set-variable warnings

Comment 58

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 12 - Fix -Wunused-variable in Nuwa.cpp

Comment 59

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 13 - Remove an unused function in Nuwa.cpp

Comment 60

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 14 - Fix -Wattribute and -Wmultichar warnings in select directories by disabling them

Comment 61

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 15 - Fix a -Woverloaded-virtual warning by renaming a method

Comment 62

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 16 - Avoid returning a reference to a temporary

Comment 63

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 17 - Fix -Wnarrowing warnings

Comment 64

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 18 - Fix -Wsign-conversion warnings

Comment 65

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 19 - Enable warnings-as-errors on B2G for gcc > 4.4

Comment 66

[Botond Ballo [:botond] [away until July 9]]

Build-only Try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b673d9733a72

This is an attempt at a build-only Try push with non-unified builds. I don't know if this is the proper way to do it, but I added 'mk_add_options MOZ_DISABLE_UNIFIED_COMPILATIONS=1' to build/mozconfig.common.override, similar to what the instructions for forcing PGO builds say here [1]:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d0b8b4669018

[1] https://wiki.mozilla.org/ReleaseEngineering/TryChooser#What_if_I_want_PGO_for_my_build

Comment 67

[Botond Ballo [:botond] [away until July 9]]

(In reply to Botond Ballo [:botond] from comment #66)
> Build-only Try push:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b673d9733a72
> 
> This is an attempt at a build-only Try push with non-unified builds. I don't
> know if this is the proper way to do it, but I added 'mk_add_options
> MOZ_DISABLE_UNIFIED_COMPILATIONS=1' to build/mozconfig.common.override,
> similar to what the instructions for forcing PGO builds say here [1]:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d0b8b4669018
> 
> [1]
> https://wiki.mozilla.org/ReleaseEngineering/
> TryChooser#What_if_I_want_PGO_for_my_build

Turns out this doesn't work for B2G, which doesn't use mozconfig.common.override. Instead, I hacked configure.in directly, and this time got a Try push with actual non-unified builds on B2G:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b570e028cdc7

Comment 68

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 18.5 - Fix warnings about calling 'delete' on an object of incomplete type

This patch fixes the Werrors in the non-unified builds.

To explain a bit what's going on here, as it may not be obvious:

The warnings were of the form:

In file included from ../../../dist/include/SharedBuffer.h:12:0,
                 from ../../../dist/include/AudioSegment.h:11,
                 from ../../../../../refactoring/dom/media/encoder/TrackEncoder.h:11,
                 from ../../../../../refactoring/dom/media/encoder/MediaEncoder.h:10,
                 from ../../../../../refactoring/dom/media/encoder/MediaEncoder.cpp:5:
../../../dist/include/nsAutoPtr.h: In instantiation of 'nsAutoPtr<T>::~nsAutoPtr() [with T = android::OMXAudioEncoder]':
../../../../../refactoring/dom/media/encoder/OmxTrackEncoder.h:45:7:   required from here
../../../dist/include/nsAutoPtr.h:74:5: error: possible problem detected in invocation of delete operator: [-Werror]
../../../dist/include/nsAutoPtr.h:74:5: error: invalid use of incomplete type 'class android::OMXAudioEncoder' [-Werror]
In file included from ../../../../../refactoring/dom/media/encoder/MediaEncoder.cpp:26:0:
../../../../../refactoring/dom/media/encoder/OmxTrackEncoder.h:13:7: error: forward declaration of 'class android::OMXAudioEncoder' [-Werror]
../../../dist/include/nsAutoPtr.h:74:5: note: neither the destructor nor the class-specific operator delete will be called, even if they are declared when the class is defined

They concerned a class with a member of type nsAutoPtr<X>, where the type X only had a forward-declaration visible. This class had (1) an implicitly-defined destructor, and (2) an user-declared constructor that was defined inline. Both (1) and (2) caused a warning, for the following reasons:

  (1) When the compiler sees a class with no user-declared destructor, it
      synthesizes a default destructor that just calls the destructors of
      the base subobjects and data members. This destructor is generated
      in every translation unit that creates an object of the class.

      In this case, the class had a member of type nsAutoPtr<X>, so the
      compiler needed to emit a call to the destructor of that. That
      destructor in turn calls 'delete mRawPtr', where 'mRawPtr' is a
      pointer to X. This delete statement in turn translates to a call to
      X's destructor, and to 'operator delete', which may be overloaded for 
      a class, However, since X is incomplete at this point, the compiler
      can't emit a call to the destructor, and it can't tell whether 
      'operator delete' has been overloaded so it just emits a call to the
      global 'operator delete'.

  (2) The constructor of the class may also need to call the destructors
      of members, in case an exception occurs at a point during the
      constructor call where the member has already been constructed.
      (In this case, there is no such point, because constructing the
      nsAutoPtr<X> is the last thing the constructor does, and in any
      case we compile with exceptions disabled, but the warning analysis
      is not smart enough to detect that (this might be a bug worth
      filing against gcc and clang)). Since the user-declared constructor
      is defined inline, the compiler needs to generate the constructor
      definition (which potentially contains this member destructor call)
      in every translation unit that creates an object of the class,
      including ones where the definition of X is not available.

The fix is to avoid having the compiler generate the constructor and destructor definitions in every translation unit that includes the header, by declaring both the constructor and the destructor, and defining them out of line.

Comment 69

[Botond Ballo [:botond] [away until July 9]]

More buildonly Try pushes:

Non-unified: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bc60978587a
Unified:     https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c78c108be84

Comment 70

[John Lin [:jhlin][:jolin]]

Comment on attachment 8543424 [details] [diff] [review]
Part 18.5 - Fix warnings about calling 'delete' on an object of incomplete type

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

Looks good. Thanks!

Comment 71

[John Lin [:jhlin][:jolin]]

Comment on attachment 8543110 [details] [diff] [review]
Part 16 - Avoid returning a reference to a temporary

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

I was considering introducing a static sp<> pointing to nullptr and returning that for the 'not available' case, but this totally works too. :)
Thanks!

Comment 72

[(not currently active) Ted Mielczarek]

Comment on attachment 8543108 [details] [diff] [review]
Part 14 - Fix -Wattribute and -Wmultichar warnings in select directories by disabling them

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

::: dom/media/webrtc/moz.build
@@ +61,5 @@
>  include('/ipc/chromium/chromium-config.mozbuild')
>  
> +# Suppress some GCC/clang warnings being treated as errors:
> +#  - about attributes on forward declarations for types that are already
> +#    defined, which complains about important MOZ_EXPORT attributes for 

super-nit: trailing space

Comment 73

[Botond Ballo [:botond] [away until July 9]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #72)
> Comment on attachment 8543108 [details] [diff] [review]
> Part 14 - Fix -Wattribute and -Wmultichar warnings in select directories by
> disabling them
> 
> Review of attachment 8543108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/webrtc/moz.build
> @@ +61,5 @@
> >  include('/ipc/chromium/chromium-config.mozbuild')
> >  
> > +# Suppress some GCC/clang warnings being treated as errors:
> > +#  - about attributes on forward declarations for types that are already
> > +#    defined, which complains about important MOZ_EXPORT attributes for 
> 
> super-nit: trailing space

Fixed locally. Thanks for the review!

Comment 74

[Mike Hommey [:glandium]]

Comment on attachment 8543105 [details] [diff] [review]
Part 12 - Fix -Wunused-variable in Nuwa.cpp

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

This file could certainly get some C++ love to remove those macros...

Comment 75

[Mike Hommey [:glandium]]

Comment on attachment 8543106 [details] [diff] [review]
Part 13 - Remove an unused function in Nuwa.cpp

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

It was used by __wrap_tgkill until bug 1091533 removed it.

Comment 76

[Mike Hommey [:glandium]]

Comment on attachment 8543113 [details] [diff] [review]
Part 19 - Enable warnings-as-errors on B2G for gcc > 4.4

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

::: b2g/confvars.sh
@@ +60,5 @@
>  MOZ_NUWA_PROCESS=1
>  MOZ_B2G_LOADER=1
> +# Warnings-as-errors cannot be enabled on gcc <= 4.4 due to bug 915555.
> +if [ "$GCC_MAJOR_VERSION" -gt 4 ] || \
> +   ([ "$GCC_MAJOR_VERSION" -eq 4 ] && [ "$GCC_MINOR_VERSION" -gt 4 ]); then

if test "$GCC_MAJOR_VERSION" -gt 4 -o "$GCC_MAJOR_VERSION" -eq 4 -a "$GCC_MINOR_VERSION" -gt 4; then

Comment 77

[Botond Ballo [:botond] [away until July 9]]

Comment on attachment 8538089 [details] [diff] [review]
Part 2 - Fix more warnings about macro redefinitions by changing macros to constants (yay!)

This patch is obsoleted by bug 989303.

Comment 78

[Botond Ballo [:botond] [away until July 9]]

Attachment: Part 19 - Enable warnings-as-errors on B2G for gcc > 4.4

(In reply to Mike Hommey [:glandium] from comment #76)
> Comment on attachment 8543113 [details] [diff] [review]
> Part 19 - Enable warnings-as-errors on B2G for gcc > 4.4
> 
> Review of attachment 8543113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/confvars.sh
> @@ +60,5 @@
> >  MOZ_NUWA_PROCESS=1
> >  MOZ_B2G_LOADER=1
> > +# Warnings-as-errors cannot be enabled on gcc <= 4.4 due to bug 915555.
> > +if [ "$GCC_MAJOR_VERSION" -gt 4 ] || \
> > +   ([ "$GCC_MAJOR_VERSION" -eq 4 ] && [ "$GCC_MINOR_VERSION" -gt 4 ]); then
> 
> if test "$GCC_MAJOR_VERSION" -gt 4 -o "$GCC_MAJOR_VERSION" -eq 4 -a
> "$GCC_MINOR_VERSION" -gt 4; then

Updated patch to make this change, carrying r+.

Comment 79

[Botond Ballo [:botond] [away until July 9]]

One more set of build-only Try pushes, this time with the patches rebased on latest m-i, to minimize the chance of bustage.

Regular:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c773ca90198f

Non-unified builds:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5db695e0e55b

Comment 80

[Botond Ballo [:botond] [away until July 9]]

Landed!

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=10faada5da34

Comment 81

[Botond Ballo [:botond] [away until July 9]]

The title of this bug has fallen a bit out of sync with its scope. Updating.

Comment 82

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fde8c97e7a8
https://hg.mozilla.org/mozilla-central/rev/5aefc41261b0
https://hg.mozilla.org/mozilla-central/rev/41c2ffa3c09d
https://hg.mozilla.org/mozilla-central/rev/4d942e4e68f4
https://hg.mozilla.org/mozilla-central/rev/b23df71e9c37
https://hg.mozilla.org/mozilla-central/rev/a7972d1adc99
https://hg.mozilla.org/mozilla-central/rev/de9147547904
https://hg.mozilla.org/mozilla-central/rev/b022fa348b73
https://hg.mozilla.org/mozilla-central/rev/c717d72546c7
https://hg.mozilla.org/mozilla-central/rev/0340001fb80a
https://hg.mozilla.org/mozilla-central/rev/e186ff7a5b40
https://hg.mozilla.org/mozilla-central/rev/6d55adbfd24d
https://hg.mozilla.org/mozilla-central/rev/f1ff5ea60279
https://hg.mozilla.org/mozilla-central/rev/0f9759d2b521
https://hg.mozilla.org/mozilla-central/rev/dd0a52ad9cb3
https://hg.mozilla.org/mozilla-central/rev/6550cdd34f6f
https://hg.mozilla.org/mozilla-central/rev/7a10ad29050e
https://hg.mozilla.org/mozilla-central/rev/35801c7c975c
https://hg.mozilla.org/mozilla-central/rev/10faada5da34

Comment 83

[(PTO June 19-July 7) Ryan VanderMeulen]

And in a fit of *fantastic* timing, the gcc 4.8 update for B2G ICS landed on b2g-inbound in the mean time. Which of course has made for fun times when this merged back on to it :(
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1119973&repo=b2g-inbound

Theory from IRC is that we can just remove the typedef, so here's a Try run for that:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=78c91e72f29f

Comment 84

[(PTO June 19-July 7) Ryan VanderMeulen]

And another:
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4070405&repo=try

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e05912f160c3

Dave Hylands mentioned on IRC that he's working through them locally as well (and there's more coming). So I'm probably going to wait on him to get a successful build at this point since that'll be much faster than Try.

Comment 85

[(PTO June 19-July 7) Ryan VanderMeulen]

Dave says this builds locally:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4fe6b4abe233

Comment 86

[Dave Hylands [:dhylands]]

Attachment: Part 20 - Fix stlport warnings which cause errors in emulator build

Comment 87

[Dave Hylands [:dhylands]]

Attachment: Part 21 - Fix other warnings causing errors on emulator build

Comment 88

[Dave Hylands [:dhylands]]

Attachment: Part 22 - Add stlport patch as a file in the tree and update README.mozilla

Comment 89

[Nathan Froyd [:froydnj]]

Comment on attachment 8546067 [details] [diff] [review]
Part 20 - Fix stlport warnings which cause errors in emulator build

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

My preference would be to just delete the offending lines, rather than comment them, but I don't suppose it matters *too* much.

Comment 90

[Nathan Froyd [:froydnj]]

Comment on attachment 8546072 [details] [diff] [review]
Part 22 - Add stlport patch as a file in the tree and update README.mozilla

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

::: build/stlport/README.mozilla
@@ +8,5 @@
>  - android-mozilla-config.patch: Adjusts Android-specific configuration
>    to the mozilla codebase use of the STL.
> +
> +- fix-warnings-as-errors.patch: Fixes warnings which were causing the
> +  B2G emulator-ICS to fail (related to bug 1073003).

Nit: "B2G emulator-ICS build"?

Comment 91

[Mike Hommey [:glandium]]

We should just import android ndk changes to stlport, which already removed those lines. I actually have a local patch for that already.

Comment 92

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/integration/b2g-inbound/rev/49caf88e970c
https://hg.mozilla.org/integration/b2g-inbound/rev/f9061e5d1c02
https://hg.mozilla.org/integration/b2g-inbound/rev/84d140bfbb58

Comment 93

[Dave Hylands [:dhylands]]

(In reply to Mike Hommey [:glandium] from comment #91)
> We should just import android ndk changes to stlport, which already removed
> those lines. I actually have a local patch for that already.

That would be the better solution.

If you want, I'd be happy to back my changes out and combine with your patch - once I get a green try run.

Comment 94

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Another wError fix: https://hg.mozilla.org/integration/b2g-inbound/rev/ccfafae7330f

Comment 95

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/49caf88e970c
https://hg.mozilla.org/mozilla-central/rev/f9061e5d1c02
https://hg.mozilla.org/mozilla-central/rev/84d140bfbb58
https://hg.mozilla.org/mozilla-central/rev/ccfafae7330f

Comment 96

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

It seems bustage happened on lollipop based build.

Comment 97

[(PTO June 19-July 7) Ryan VanderMeulen]

Given that we don't run any Lollipop-based builds in automation, I highly doubt this will be the last time that happens.

Comment 98

[Botond Ballo [:botond] [away until July 9]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #96)
> It seems bustage happened on lollipop based build.

If I can do a local lollipop-based build, I'm happy to try fixing this.

Could you point me to some instructions for doing a lollipop-based build?

Comment 99

[Botond Ballo [:botond] [away until July 9]]

(In reply to Botond Ballo [:botond] from comment #98)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #96)
> > It seems bustage happened on lollipop based build.
> 
> If I can do a local lollipop-based build, I'm happy to try fixing this.
> 
> Could you point me to some instructions for doing a lollipop-based build?

I was able to do a local lollipop build by checking out b2g and running './config.sh nexus-5-l', and I can reproduce the Werrors. I filed bug 1119980 for them, and will be posting a patch there.

Comment 100

[Juan Gomez [:_AtilA_] (CET/CEST)]

I'm having warnings-as-errors bustages on Peak (ICS, while migrating to gcc-4.8) and Flatfish (JB4.2, gcc-4.6).
Peak error:

MediaOmxDecoder.o
In file included from ../../../dist/system_wrappers/stagefright/DataSource.h:3:0,
                 from /media/data_disk/b2g/build.peak/gecko/dom/media/omx/MediaStreamSource.h:12,
                 from /media/data_disk/b2g/build.peak/gecko/dom/media/omx/MediaOmxCommonReader.cpp:13:
/media/data_disk/b2g/build.peak/frameworks/base/include/media/stagefright/DataSource.h:63:21: error: 'virtual ssize_t android::DataSource::readAt(off64_t, void*, size_t, int)' was hidden [-Werror=overloaded-virtual]
     virtual ssize_t readAt(off64_t offset, void *data, size_t size, int buffer_flag) { return readAt(offset, data, size); }
                     ^
In file included from /media/data_disk/b2g/build.peak/gecko/dom/media/omx/MediaOmxCommonReader.cpp:13:0:
/media/data_disk/b2g/build.peak/gecko/dom/media/omx/MediaStreamSource.h:31:19: error:   by 'android::MediaStreamSource::readAt' [-Werror=overloaded-virtual]
   virtual ssize_t readAt(off_t offset, void *data, size_t size) {
                   ^
cc1plus: all warnings being treated as errors

Comment 101

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

I think I saw some bugmail go by for someone pushing a fix for that warning to inbound... (Not sure, away from a computer at the moment. Posting this from a tablet.)

Comment 102

[Juan Gomez [:_AtilA_] (CET/CEST)]

I filed a new bug: 1120239 and proposed a patch.

Comment 103

[Botond Ballo [:botond] [away until July 9]]

(In reply to Mike Hommey [:glandium] from comment #74)
> Comment on attachment 8543105 [details] [diff] [review]
> Part 12 - Fix -Wunused-variable in Nuwa.cpp
> 
> Review of attachment 8543105 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This file could certainly get some C++ love to remove those macros...

Filed bug 1120694 to track this.