736501 - Use -Werror=unused-result (i.e. make unused-result build warnings fatal)

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Benjamin Smedberg]

We have NS_WARN_UNUSED_RESULT, and there's no particular reason that it shouldn't be fatal: you can explicitly override it with "unused <<" in the cases where it really doesn't matter what the result is. I want this for the infallible string refactoring I'm doing.

Comment 1

[Benjamin Smedberg]

Attachment: Make -Werror=unused-result, rev. 1

Requesting ted's review on the basic concept, mayhemer on the offline-cache-parent additions of unused. I'm confident in the DOM/plugin IPC usages.

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 606602 [details] [diff] [review]
Make -Werror=unused-result, rev. 1

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

r=honzab

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 606602 [details] [diff] [review]
Make -Werror=unused-result, rev. 1

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

Looks good, but njn should take a peek at the configure bits, since he's been hacking on our warnings CFLAGS a lot recently.

Comment 4

[Nicholas Nethercote [inactive]]

In bug 711895 I've added macros MOZ_C_SUPPORTS_WARNING and MOZ_CXX_SUPPORTS_WARNING for this common pattern.  That patch has bounced twice due to obscure breakage on non-try platforms, and is awaiting review (which hopefully won't take long) for its 3rd landing.  How soon do you need this?  It'd make my life easier if you could wait until bug 711895 lands, though if you need it quickly I can adapt.

There's a comment higher up that explains all the warnings we've enabled/disabled.  (The current code only explains some of them, with bug 711895 all are explained.)  Can you add a comment about -Werror=unused-result?  (Again, this will conflict with my patch.)

You've updated configure.in, you should update js/src/configure.in in the same way.

Is there any reason you've added the warning for C++ but not for C?  In bug 711895 I've also tried to make the warnings we use for the two languages more consistent.  If adding it for C is feasible, that'd be great.

I suspect you'll get some breakage on obscure platforms.  Fatal warnings are very difficult to turn on by default, in my experience.  That's why --enable-warnings-as-errors is off by default, for example.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 606602 [details] [diff] [review]
Make -Werror=unused-result, rev. 1

I'm giving f- because I'd like to see an updated patch and answers to my questions.  Thanks!

Comment 6

[Benjamin Smedberg]

Attachment: Make -Werror=unused-result, rev. 2

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 619956 [details] [diff] [review]
Make -Werror=unused-result, rev. 2

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

The configure.in changes look much better, thanks for waiting on bug 711895.

Comment 8

[Nicholas Nethercote [inactive]]

BTW, does |unused << foo();| have any obvious advantages over |(void)foo();|, other than being a little more explicit?

Comment 9

[Benjamin Smedberg]

unused << suppresses the warning/error. (void) foo() doesn't.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 619956 [details] [diff] [review]
Make -Werror=unused-result, rev. 2

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

::: configure.in
@@ +1845,5 @@
>      _WARNINGS_CXXFLAGS="${_WARNINGS_CXXFLAGS} -Wall -Wpointer-arith -Woverloaded-virtual"
>      MOZ_CXX_SUPPORTS_WARNING(-W, error=return-type, ac_cxx_has_werror_return_type)
>      MOZ_CXX_SUPPORTS_WARNING(-W, type-limits, ac_cxx_has_wtype_limits)
>      MOZ_CXX_SUPPORTS_WARNING(-W, empty-body, ac_cxx_has_wempty_body)
> +    MOZ_CXX_SUPPORTS_WARNING(-W, error=unused-result, ac_cxx_has_werror_unused_result)

Not something that has to be fixed now, but this list is growing to the point that we might want to consider putting the entire thing into an m4 file and just having a MOZ_CHECK_SUPPORTED_WARNINGS or something.

Comment 11

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/45ffd4b20e11

Comment 12

[Brad Jackson]

This breaks my build. I'm using GCC 4.7. There could be more than these.

/home/bsjacks/mozilla/memory/jemalloc/jemalloc.c:1530:8: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
/home/bsjacks/mozilla/memory/jemalloc/jemalloc.c:1531:8: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
/home/bsjacks/mozilla/memory/jemalloc/jemalloc.c:1532:8: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]
/home/bsjacks/mozilla/memory/jemalloc/jemalloc.c:1533:8: error: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Werror=unused-result]

/home/bsjacks/mozilla/js/src/shell/js.cpp:4216:53: error: ignoring return value of ‘size_t fwrite(const void*, size_t, size_t, FILE*)’, declared with attribute warn_unused_result [-Werror=unused-result]

Comment 13

[Matt Brubeck (:mbrubeck)]

I backed this out temporarily because it broke the build for developers using some versions of GCC:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1021fb3a0474

See bug 754198 for details.  This patch can re-land after those errors are fixed.

Comment 14

[Justin Lebar (not reading bugmail)]

I thought our plan for -Werror was that we'd enable it only on the tinderboxen, so as to avoid exactly this problem?

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to Justin Lebar [:jlebar] from comment #14)
> I thought our plan for -Werror was that we'd enable it only on the
> tinderboxen, so as to avoid exactly this problem?

That's true, but it's orthogonal to this bug.

The tinderboxen-only(ish) thing is --enable-warnings-as-errors, which uses -Werror (making all warnings fatal) only in a relatively small set of whitelisted known-to-be-warning-free directories.

However -- for specific warnings that are severe and/or targeted enough, we can *also* make those specific warnings be globally fatal. I think that's what bsmedberg was getting at here.

Comment 17

[Daniel Holbert [:dholbert]]

Regardless of whether we can safely make unused-result fatal, the rest of this bug's patch (using 'unused <<' sparingly to fix instances of this warning) can probably be re-landed and clean up the build-spam caused by this warning.

So -- I spun off bug 833931 on landing the pure-warning-fixes part of this bug's patches. (basically everything in comment 11 except for the configure.in tweak).  This bug (as described in its summary) can stay tracking the configure.in tweak (making this warning globally fatal), if we ever decide to finish that part off, and any remaining instances of the warning can be tracked in other helper-bugs.

Comment 18

[Daniel Holbert [:dholbert]]

Actually, comment 11 was the wrong cset for this bug -- the correct cset was:
 https://hg.mozilla.org/mozilla-central/rev/6a7bfd84596e

Comment 19

[Nicholas Nethercote [inactive]]

In my experience, -Wreturn-type is the only warning that can be made globally fatal without negative consequences.

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> This bug (as described in its summary) can stay
> tracking the configure.in tweak (making this warning globally fatal), if we
> ever decide to finish that part off, and any remaining instances of the
> warning can be tracked in other helper-bugs.

This ^^ remaining part is now effectively fixed, on our treeherder builders at least (and developers who opt-in via "ac_add_options --enable-warnings-as-errors"), by virtue of *all* build warnings now being fatal by default.

Fatal-warnings became the default in bug 1198334, so I'll resolve this as WORKSFORME with a dependency on that bug.

([ab]using WORKSFORME to emphasize that the patches posted here didn't actually land as part of this bug -- not directly at least)