Closed Bug 1073081 Opened 5 years ago Closed 5 years ago

Allow building with warnings-as-errors on B2G

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(19 files, 13 obsolete files)

7.18 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
734 bytes, patch
ehsan
: review+
Details | Diff | Splinter Review
7.62 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
842 bytes, patch
ehsan
: review+
Details | Diff | Splinter Review
3.37 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
4.38 KB, patch
ehsan
: review+
ted
: review+
Details | Diff | Splinter Review
6.54 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.13 KB, patch
mwu
: review+
Details | Diff | Splinter Review
15.60 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
3.67 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
8.37 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
1.83 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
1.14 KB, patch
nical
: review+
Details | Diff | Splinter Review
955 bytes, patch
bechen
: review+
Details | Diff | Splinter Review
4.84 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
3.75 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
36.61 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
1.87 KB, patch
ehsan
: review+
ted
: review+
Details | Diff | Splinter Review
955 bytes, patch
botond
: review+
Details | Diff | Splinter Review
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.
Attached patch Warning fixes (obsolete) — Splinter Review
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.
Blocks: 1073003
Attached patch Additional warning fixes for KK (obsolete) — Splinter Review
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.
See Also: → 1085635
Attached patch Warning fixes (obsolete) — Splinter Review
Rebased and combined patches. They still need cleanup.
Attachment #8495410 - Attachment is obsolete: true
Attachment #8499860 - Attachment is obsolete: true
(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.)
Depends on: 915555
Attached patch [WIP] Warning fixes (obsolete) — Splinter Review
Rebased again.
Attachment #8508226 - Attachment is obsolete: true
Attached patch [WIP] Warning fixes (obsolete) — Splinter Review
Rebased.
Attachment #8511179 - Attachment is obsolete: true
Attached patch [WIP] Warning fixes (obsolete) — Splinter Review
Rebased.
Attachment #8515349 - Attachment is obsolete: true
Attached patch [WIP] Warning fixes (obsolete) — Splinter Review
Rebased. I'll try to split up this patch and put it up for review soon.
Attachment #8520081 - Attachment is obsolete: true
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.
(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!
Attachment #8530351 - Flags: review?(ted)
Attachment #8530351 - Flags: review?(ehsan.akhgari)
Attachment #8530353 - Flags: review?(slee)
Attachment #8530355 - Flags: review?(ehsan.akhgari)
Attachment #8530358 - Flags: review?(ehsan.akhgari)
Attachment #8530359 - Flags: review?(ehsan.akhgari)
Attachment #8530360 - Flags: review?(ted)
Attachment #8530360 - Flags: review?(ehsan.akhgari)
Attachment #8530363 - Flags: review?(sotaro.ikeda.g)
Attachment #8530365 - Flags: review?(dhylands)
Attachment #8530366 - Flags: review?(bechen)
Attachment #8530364 - Flags: review?(nical.bugzilla) → review+
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?
Attachment #8530361 - Flags: review?(brsun) → review?(cpearce)
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
Attachment #8530361 - Flags: review?(cpearce)
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.
Attachment #8530353 - Flags: review?(slee) → review?(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.
Attachment #8530353 - Flags: review?(mwu) → review+
Attachment #8530362 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8530363 - Flags: review?(sotaro.ikeda.g) → review+
Updated Part 15 patch to rename MediaOmxCommonDecoder::CreateStateMachine to CreateStateMachineFromReader as per comment 32.
Attachment #8530361 - Attachment is obsolete: true
Attachment #8531756 - Flags: review?(cpearce)
Attachment #8531756 - Flags: review?(cpearce) → review+
Attachment #8530366 - Flags: review?(bechen) → review+
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)
Attachment #8530360 - Flags: review?(ted) → review+
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?
Attachment #8530351 - Flags: review?(ted) → review+
Attachment #8530346 - Flags: review?(ehsan.akhgari) → review+
Attachment #8530347 - Flags: review?(ehsan.akhgari) → review+
Attachment #8530348 - Flags: review?(ehsan.akhgari) → review+
Attachment #8530349 - Flags: review?(ehsan.akhgari) → review+
Attachment #8530350 - Flags: review?(ehsan.akhgari) → review+
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.
Attachment #8530351 - Flags: review?(ehsan.akhgari) → review+
Attachment #8530354 - Flags: review?(ehsan.akhgari) → review+
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?
Attachment #8530355 - Flags: review?(ehsan.akhgari) → review+
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.
Attachment #8530357 - Flags: review?(ehsan.akhgari) → review-
Attachment #8530358 - Flags: review?(ehsan.akhgari) → review+
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*.
Attachment #8530359 - Flags: review?(ehsan.akhgari)
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.
Attachment #8530360 - Flags: review?(ehsan.akhgari) → review+
(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
(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.
(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.
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.
(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 on attachment 8530359 [details] [diff] [review]
Part 13 - Fix warnings about pointer casts

This patch does indeed appear to be unnecessary.
Attachment #8530359 - Attachment is obsolete: true
Attachment #8530357 - Attachment is obsolete: true
Attachment #8535355 - Flags: review?(ehsan.akhgari)
Attachment #8530358 - Attachment is obsolete: true
Attachment #8535356 - Flags: review?(ehsan.akhgari)
(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.
Attachment #8530360 - Attachment is obsolete: true
Attachment #8535357 - Flags: review?(ted)
Attachment #8535357 - Flags: review?(ehsan.akhgari)
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.
Attachment #8530352 - Flags: review?(tlee) → review-
Attachment #8535357 - Flags: review?(ted) → review+
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
Attachment #8530352 - Flags: review- → review+
and 3) Those symbols do not appear to be weak on b2g.
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?
Attachment #8530365 - Flags: review?(dhylands) → review+
(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!
Attachment #8530365 - Attachment is obsolete: true
Attachment #8535827 - Flags: review+
Attachment #8535355 - Flags: review?(ehsan.akhgari) → review+
Attachment #8535356 - Flags: review?(ehsan.akhgari) → review+
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?
Attachment #8535357 - Flags: review?(ehsan.akhgari) → review+
Added check for clang-cl and landed:

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

Thanks to all for the reviews!
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!
Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(bechen)
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?
Flags: needinfo?(nical.bugzilla) → needinfo?(milan)
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.
Flags: needinfo?(milan)
(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?
Flags: needinfo?(bechen) → needinfo?(ettseng)
(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.
Flags: needinfo?(ettseng)
Duplicate of this bug: 1115441
You need to log in before you can comment on or make changes to this bug.