1073081 - Allow building with warnings-as-errors on B2G

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Botond Ballo [:botond]]

Currently we don't build with warnings-as-errors on B2G. This is unfortunate because people doing B2G development don't catch -Werrors during their local builds.

I'd like to enable warnings-as-errors on B2G. This involves:
  1) Fixing the existing warnings that turn into errors
     when warnings-as-errors is turned on.
  2) Turning on warnings-as-errors.

Comment 1

[Botond Ballo [:botond]]

Attachment: Warning fixes

This patch allows me to build locally on B2G with warnings-as-errors enabled (via the mechanism introduced in bug 1073003).

I need to clean up a few of the warning fixes a bit; others I'm not sure about, and would like to talk to the code's author to make sure they are right. 

Once the patch is ready to be reviewed, I'll split it into parts and post it for review. For now I'm just posting it in case anyone wants to build with it locally.

Comment 2

[Botond Ballo [:botond]]

Attachment: Additional warning fixes for KK

When switching from a flame build to a flame-kk build, I had to make a number of additional warning fixes to get KK-specific code to compile. I'm attaching them here in case anyone wants to use them.

As with the other patch, this one still needs some cleanup  before it's ready to be reviewed.

Comment 3

[Botond Ballo [:botond]]

Attachment: Warning fixes

Rebased and combined patches. They still need cleanup.

Comment 4

[Daniel Holbert [:dholbert]]

(Adding dependency on bug 915555, since that bug's warning surely needs to be resolved [via GCC upgrade] before we can build with warnings treated as errors.)

Comment 5

[Botond Ballo [:botond]]

Attachment: [WIP] Warning fixes

Rebased again.

Comment 6

[Botond Ballo [:botond]]

Attachment: [WIP] Warning fixes

Rebased.

Comment 7

[Botond Ballo [:botond]]

Attachment: [WIP] Warning fixes

Rebased.

Comment 8

[Botond Ballo [:botond]]

Attachment: [WIP] Warning fixes

Rebased. I'll try to split up this patch and put it up for review soon.

Comment 9

[Botond Ballo [:botond]]

I cleaned up the patch and split it up into reviewable parts. Will wait for a buildonly Try push before posting them for review: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee3e43ef42be.

Comment 10

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #9)
> I cleaned up the patch and split it up into reviewable parts. Will wait for
> a buildonly Try push before posting them for review:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee3e43ef42be.

Looking green!

Comment 11

[Botond Ballo [:botond]]

Attachment: Part 1 - Fix -Wdelete-non-virtual-dtor warnings by marking some classes as MOZ_FINAL

Comment 12

[Botond Ballo [:botond]]

Attachment: Part 2 - Fix -Wdelete-non-virtual-dtor warnings by adding virtual destructors

Comment 13

[Botond Ballo [:botond]]

Attachment: Part 3 - Fix -Wsign-compare and -Wsign-conversion warnings

Comment 14

[Botond Ballo [:botond]]

Attachment: Part 4 - Fix -Wattribute warnings by removing unnecessary forward declarations

Comment 15

[Botond Ballo [:botond]]

Attachment: Part 5 - Fix -Wattribute warnings by removing MOZ_EXPORT from forward declarations

Comment 16

[Botond Ballo [:botond]]

Attachment: Part 6 - Fix -Wattribute warnings by disabling them

Comment 17

[Botond Ballo [:botond]]

Attachment: Part 7 - Fix -Waddress warnings about Nuwa functions

Comment 18

[Botond Ballo [:botond]]

Attachment: Part 8 - Fix a -Waddress warning

Comment 19

[Botond Ballo [:botond]]

Attachment: Part 9 - Fix -Wunused-result, -Wunused-variable, and -Wunused-but-set-variable warnings

Comment 20

[Botond Ballo [:botond]]

Attachment: Part 10 - Fix -Wreorder warnings

Comment 21

[Botond Ballo [:botond]]

Attachment: Part 11 - Fix warnings about macro redefinitions by #undef'ing macros

Comment 22

[Botond Ballo [:botond]]

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

Comment 23

[Botond Ballo [:botond]]

Attachment: Part 13 - Fix warnings about pointer casts

Comment 24

[Botond Ballo [:botond]]

Attachment: Part 14 - Fix -Wmultichar warnings by disabling them

Comment 25

[Botond Ballo [:botond]]

Attachment: Part 15 - Fix -Woverloaded-virtual warnings in the MediaDecoder hierarchy

Comment 26

[Botond Ballo [:botond]]

Attachment: Part 16 - Fix -Woverloaded-virtual warnings in gonk code

Comment 27

[Botond Ballo [:botond]]

Attachment: Part 17 - Remove a couple of unused functions

Comment 28

[Botond Ballo [:botond]]

Attachment: Part 18 - Fix an incorrect comparison between enums of different types

Comment 29

[Botond Ballo [:botond]]

Attachment: Part 19 - Remove an old preprocessor #else branch

Comment 30

[Botond Ballo [:botond]]

Attachment: Part 20 - Fix an operator precedence bug

Comment 31

[Bruce Sun [:brsun]]

Comment on attachment 8530361 [details] [diff] [review]
Part 15 - Fix -Woverloaded-virtual warnings in the MediaDecoder hierarchy

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

Thanks for catching this defect. Comparing to add "using MediaOmxCommonDecoder::CreateStateMachine" in the definition of all subclasses, a better solution to fix this problem might be choosing a different name of CreateStateMachine(MediaOmxCommonReader* aReader) for MediaOmxCommonDecoder and all subclasses. Maybe CreateStateMachineWithReader() could be a clearer name.

cpearce: what do you think?

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8530361 [details] [diff] [review]
Part 15 - Fix -Woverloaded-virtual warnings in the MediaDecoder hierarchy

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

I agree with Bruce. Please rename MediaOmxCommonDecoder::CreateStateMachine to MediaOmxCommonDecoder::CreateStateMachineFromReader

Comment 33

[StevenLee[:slee]]

Comment on attachment 8530353 [details] [diff] [review]
Part 8 - Fix a -Waddress warning

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

sorry for late response. I think mwu is the correct one to review the code.

Comment 34

[Michael Wu [:mwu]]

Comment on attachment 8530353 [details] [diff] [review]
Part 8 - Fix a -Waddress warning

Seems ok to do, since we shouldn't need to pretend we support GB anymore.

Comment 35

[Botond Ballo [:botond]]

Attachment: Part 15 - Fix -Woverloaded-virtual warnings in the MediaDecoder hierarchy

Updated Part 15 patch to rename MediaOmxCommonDecoder::CreateStateMachine to CreateStateMachineFromReader as per comment 32.

Comment 36

[(not currently active) Ted Mielczarek]

Comment on attachment 8530360 [details] [diff] [review]
Part 14 - Fix -Wmultichar warnings by disabling them

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

Just curious, why is the -Werror=multichar only a problem on B2G?

::: dom/media/fmp4/gonk/moz.build
@@ +24,5 @@
>  
>  # Suppress some GCC warnings being treated as errors:
>  #  - about attributes on forward declarations for types that are already
>  #    defined, which complains about an important MOZ_EXPORT for android::AString
> +#  - about multi-character constants which are used in codec-related code

Is this something we could get fixed in the various places it's being used, or is it hopeless?

@@ +29,3 @@
>  if CONFIG['GNU_CC']:
> +  CXXFLAGS += [
> +    '-Wno-error=attributes', 

nit: trailing whitespace (in a few other places too)

Comment 37

[(not currently active) Ted Mielczarek]

Comment on attachment 8530351 [details] [diff] [review]
Part 6 - Fix -Wattribute warnings by disabling them

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

Is this something we can fix, or do we just have to live with it?

Comment 38

[(no longer active)]

Comment on attachment 8530351 [details] [diff] [review]
Part 6 - Fix -Wattribute warnings by disabling them

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

I think this is fine.  This warning is pretty useless IMO anyway.

Comment 39

[(no longer active)]

Comment on attachment 8530355 [details] [diff] [review]
Part 10 - Fix -Wreorder warnings

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

::: dom/ipc/PreallocatedProcessManager.cpp
@@ +121,1 @@
>    , mShutdown(false)

Nit: sorry to make you do this here, but this looks terrible now.  How about putting the commas after each field?

Comment 40

[(no longer active)]

Comment on attachment 8530357 [details] [diff] [review]
Part 11 - Fix warnings about macro redefinitions by #undef'ing macros

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

I think a better fix here would be to #undef the macros at the end of the files that define them, not at the beginning of next files.  That way the code will be robust against shifting the order of files.

Comment 41

[(no longer active)]

Comment on attachment 8530359 [details] [diff] [review]
Part 13 - Fix warnings about pointer casts

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

::: dom/media/fmp4/gonk/GonkAudioDecoderManager.cpp
@@ +122,5 @@
>    const uint8_t *data = static_cast<const uint8_t*>(mAudioBuffer->data());
>    size_t dataOffset = mAudioBuffer->range_offset();
>    size_t size = mAudioBuffer->range_length();
>  
> +  nsAutoArrayPtr<AudioDataValue> buffer(new AudioDataValue[size/2] );

Nit: no space before closing paren.

@@ +123,5 @@
>    size_t dataOffset = mAudioBuffer->range_offset();
>    size_t size = mAudioBuffer->range_length();
>  
> +  nsAutoArrayPtr<AudioDataValue> buffer(new AudioDataValue[size/2] );
> +  memcpy(buffer.get(), (char*)data+dataOffset, size);

I don't think this is necessary since data+dataOffset should cast to const void*.

Comment 42

[(no longer active)]

Comment on attachment 8530360 [details] [diff] [review]
Part 14 - Fix -Wmultichar warnings by disabling them

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

This is kind of sad.  If possible, please consider adding these flags to specific SOURCES files, otherwise this is fine.

Comment 43

[Botond Ballo [:botond]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #36)
> Comment on attachment 8530360 [details] [diff] [review]
> Part 14 - Fix -Wmultichar warnings by disabling them
> 
> Review of attachment 8530360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just curious, why is the -Werror=multichar only a problem on B2G?

I believe the code that contains the multichar constants is only compiled in B2G.

While attempting to confirm that, though, I discovered that some of the changes in this patch appear to be unnecessary; possibly some code has changed to no longer contain the multichar constants. I'll do a Try push and get back to you, possibly with a smaller patch.

> ::: dom/media/fmp4/gonk/moz.build
> @@ +24,5 @@
> >  
> >  # Suppress some GCC warnings being treated as errors:
> >  #  - about attributes on forward declarations for types that are already
> >  #    defined, which complains about an important MOZ_EXPORT for android::AString
> > +#  - about multi-character constants which are used in codec-related code
> 
> Is this something we could get fixed in the various places it's being used,
> or is it hopeless?

A multichar constant is basically an integer constant whose value is expressed by the character each byte in the value makes. The reason compilers warn about them is that the standard does not define how exactly the characters are mapped to an integer value.

The uses in the code appear to be for constants used in protocols. For example, the value of a constant named 'kNotifyPostReleaseBuffer', which is a particular message type, is 'nprb' [1]. I guess it's convenient for debugging to be able to look at a message in binary form, and have the message type be apparent? And the standard not specifying how the characters are mapped to an integer value ends up not being a problem because implementations agree in practice? The authors of this code could probably answer these questions better.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/GonkVideoDecoderManager.h?rev=b6785b8cb812#171

Comment 44

[Botond Ballo [:botond]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #37)
> Comment on attachment 8530351 [details] [diff] [review]
> Part 6 - Fix -Wattribute warnings by disabling them
> 
> Review of attachment 8530351 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this something we can fix, or do we just have to live with it?

I haven't been able to find a way to fix it. Let me describe what I've tried:

1) As-is, the code gives the following Werror:

       error: type attributes ignored after type is already defined [-Werror=attributes]

   for a forward declaration that includes attributes, such as: 

       class MOZ_EXPORT MetaData;

   This happens when the include is processed within a translation unit after
   the type in question has already been defined.

2) The obvious thing to try is to remove the forward-declaration.
   However, in other translation units, the include is processed
   without a preceding file that defines the type, so the forward-
   declaration is necessary.

3) The next thing I tried is removing the MOZ_EXPORT from the
   forward-declaration. However, this produces linker errors.

It's possible there's an approach for fixing this that I haven't thought of; I'm open to suggestions.

Comment 45

[Botond Ballo [:botond]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #39)
> Comment on attachment 8530355 [details] [diff] [review]
> Part 10 - Fix -Wreorder warnings
> 
> Review of attachment 8530355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/PreallocatedProcessManager.cpp
> @@ +121,1 @@
> >    , mShutdown(false)
> 
> Nit: sorry to make you do this here, but this looks terrible now.  How about
> putting the commas after each field?

Discussed this in person - I'll keep the formatting as is, to be consistent with the format proposed for the style guide in https://groups.google.com/forum/#!topic/mozilla.dev.platform/JvT7QyIJg0g.

Comment 46

[Botond Ballo [:botond]]

I've addressed all review comments locally. 

Build-only Try push to make sure everything builds: https://tbpl.mozilla.org/?tree=Try&rev=594829308269

Once that's green, I'll post updated patches for review.

Comment 47

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #46)
> Build-only Try push to make sure everything builds:
> https://tbpl.mozilla.org/?tree=Try&rev=594829308269

Got Windows bustage because I forgot to condition a change to a moz.build on GNU_CC. 

Trying again: https://tbpl.mozilla.org/?tree=Try&rev=d4459e4b044f

Comment 48

[Botond Ballo [:botond]]

Comment on attachment 8530359 [details] [diff] [review]
Part 13 - Fix warnings about pointer casts

This patch does indeed appear to be unnecessary.

Comment 49

[Botond Ballo [:botond]]

Attachment: Part 11 - Fix warnings about macro redefinitions by #undef'ing macros

Comment 50

[Botond Ballo [:botond]]

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

Comment 51

[Botond Ballo [:botond]]

Attachment: Part 14 - Fix -Wmultichar warnings by disabling them

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #42)
> Comment on attachment 8530360 [details] [diff] [review]
> Part 14 - Fix -Wmultichar warnings by disabling them
> 
> Review of attachment 8530360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is kind of sad.  If possible, please consider adding these flags to
> specific SOURCES files, otherwise this is fine.

I did this for dom/media/moz.build. For dom/media/fmp4/gonk/moz.build, 3 of the 4 files in the directory used a multichar constant, so I just left the flag directory-wide.

Comment 52

[Thinker Li [:sinker]]

Comment on attachment 8530352 [details] [diff] [review]
Part 7 - Fix -Waddress warnings about Nuwa functions

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

::: dom/ipc/ContentChild.cpp
@@ -2315,5 @@
>  
>  static void
>  DoNuwaFork()
>  {
> -    NS_ASSERTION(NuwaSpawnPrepare != nullptr,

This is necessary to make sure that weak symbols are there and fail early.  |((void *)&NuwaSpawnPrepare) != nullptr| could fix the issue.

Comment 53

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8530352 [details] [diff] [review]
Part 7 - Fix -Waddress warnings about Nuwa functions

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

I disagree with Thinker, because

1) These are non-fatal debug assertions, which nobody pays attention to on b2g.
2) If the assertions do fail, we'll immediately crash trying to call the non-existent functions.

So removing these doesn't really hurt anything, IMO.

r=me

Comment 54

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

and 3) Those symbols do not appear to be weak on b2g.

Comment 55

[Dave Hylands [:dhylands]]

Comment on attachment 8530365 [details] [diff] [review]
Part 19 - Remove an old preprocessor #else branch

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

::: hal/gonk/GonkHal.cpp
@@ -1209,5 @@
>  #ifndef KLOG_SIZE_BUFFER
>    // Upstream bionic in commit
>    // e249b059637b49a285ed9f58a2a18bfd054e5d95
>    // deprecated the old klog defs.
>    // Our current bionic does not hit this

Can we edit this comment to say that ICS doesn't have KLOG_SIZE_BUFFER but JB and onwards does?

Comment 56

[Botond Ballo [:botond]]

Attachment: Part 19 - Remove an old preprocessor #else branch

(In reply to Dave Hylands [:dhylands][PTO] from comment #55)
> Comment on attachment 8530365 [details] [diff] [review]
> Part 19 - Remove an old preprocessor #else branch
> 
> Review of attachment 8530365 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: hal/gonk/GonkHal.cpp
> @@ -1209,5 @@
> >  #ifndef KLOG_SIZE_BUFFER
> >    // Upstream bionic in commit
> >    // e249b059637b49a285ed9f58a2a18bfd054e5d95
> >    // deprecated the old klog defs.
> >    // Our current bionic does not hit this
> 
> Can we edit this comment to say that ICS doesn't have KLOG_SIZE_BUFFER but
> JB and onwards does?

Updated patch to add comment. Carrying r+. 

Thanks!

Comment 57

[(no longer active)]

Comment on attachment 8535357 [details] [diff] [review]
Part 14 - Fix -Wmultichar warnings by disabling them

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

Thanks!

BTW this also applies to clang-cl in addition to gcc/clang itself (we don't define GNU_CC for clang-cl in the build system.  Do you mind extending the compiler check to include clang-cl too before landing, please?

Comment 58

[Botond Ballo [:botond]]

Added check for clang-cl and landed:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=7fa8db59f356

Thanks to all for the reviews!

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f578c350bb8
https://hg.mozilla.org/mozilla-central/rev/6ab2dbf39c3f
https://hg.mozilla.org/mozilla-central/rev/bb88a7f640fa
https://hg.mozilla.org/mozilla-central/rev/f292b87f24a3
https://hg.mozilla.org/mozilla-central/rev/3506bbdc98bb
https://hg.mozilla.org/mozilla-central/rev/60b12cab3b04
https://hg.mozilla.org/mozilla-central/rev/9930760dfebc
https://hg.mozilla.org/mozilla-central/rev/707843f409c8
https://hg.mozilla.org/mozilla-central/rev/e66856cc6f8c
https://hg.mozilla.org/mozilla-central/rev/4c6ba80850d6
https://hg.mozilla.org/mozilla-central/rev/0ce542e44d9f
https://hg.mozilla.org/mozilla-central/rev/9450c7bb0b6b
https://hg.mozilla.org/mozilla-central/rev/368d4743c126
https://hg.mozilla.org/mozilla-central/rev/952839f8ea63
https://hg.mozilla.org/mozilla-central/rev/31cbba901d07
https://hg.mozilla.org/mozilla-central/rev/3ebcec626dcd
https://hg.mozilla.org/mozilla-central/rev/8df0ab3b426a
https://hg.mozilla.org/mozilla-central/rev/3cc200eb27bc
https://hg.mozilla.org/mozilla-central/rev/7fa8db59f356
https://hg.mozilla.org/mozilla-central/rev/88fbef622ae3

Comment 60

[Kartikaya Gupta (email:[e-mail])]

\o/

Comment 61

[Botond Ballo [:botond]]

Nical, Benjamin: the Part 18 and Part 20 patches in this bug fix actual bugs caught by the warnings, and we may want to consider uplifting them.

I don't understand the relevant code well enough to comment on the "user impact" for an uplift request - could you comment on that, and then I'll request uplift (or, feel free to just request uplift yourself)? Thanks!

Comment 62

[Nicolas Silva [:nical]]

Part 18 is b2g-specific. It's trivial to uplift, but probably a bit late for 2.1 (since we haven't observed bugs caused by it). But we should at least take it for 2.2 which will branch from 37 (I think?).
If it's not too late for 2.1 I would take it there as well, but since we haven't observed bugs caused by the mistake the patch solves I think it's ok to pass on it. Milan, what do you think?

Comment 63

[Milan Sreckovic [:milan] (needinfo for best results)]

It will be difficult to argue for 2.1 because we don't have an STR.  As long as this stays in the current train, we should be OK for 2.2.

Comment 64

[Benjamin Chen [:bechen]]

(In reply to Botond Ballo [:botond] from comment #61)
> Nical, Benjamin: the Part 18 and Part 20 patches in this bug fix actual bugs
> caught by the warnings, and we may want to consider uplifting them.
> 
> I don't understand the relevant code well enough to comment on the "user
> impact" for an uplift request - could you comment on that, and then I'll
> request uplift (or, feel free to just request uplift yourself)? Thanks!

From logical, part 20 looks like a real bug, I think we should test the code first then decide to uplift or not.

Hi Ethan:
Could you please put an AMR rtsp audio stream on your server so that we can test the code?

Comment 65

[Ethan Tseng [:ethan]]

(In reply to Benjamin Chen [:bechen] from comment #64)
> From logical, part 20 looks like a real bug, I think we should test the code
> first then decide to uplift or not.
Agree.
The != operator has higher precedence than the & operator. It should be a bug.

> Hi Ethan:
> Could you please put an AMR rtsp audio stream on your server so that we can
> test the code?
I cannot find AMR clips in my repertoire.
I will try to find some tool to convert AAC to AMR.