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

RESOLVED WORKSFORME

Status

defect
RESOLVED WORKSFORME
7 years ago
Last year

People

(Reporter: benjamin, Unassigned)

Tracking

Trunk
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 obsolete attachments)

Reporter

Description

7 years ago
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.
Reporter

Comment 1

7 years ago
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.
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #606602 - Flags: review?(ted.mielczarek)
Attachment #606602 - Flags: review?(honzab.moz)
Comment on attachment 606602 [details] [diff] [review]
Make -Werror=unused-result, rev. 1

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

r=honzab
Attachment #606602 - Flags: review?(honzab.moz) → review+
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.
Attachment #606602 - Flags: review?(ted.mielczarek)
Attachment #606602 - Flags: review+
Attachment #606602 - Flags: feedback?(n.nethercote)
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 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!
Attachment #606602 - Flags: feedback?(n.nethercote) → feedback-
Reporter

Updated

7 years ago
Blocks: 737164
Reporter

Comment 6

7 years ago
Attachment #619956 - Flags: review?(n.nethercote)
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.
Attachment #619956 - Flags: review?(n.nethercote) → review+
BTW, does |unused << foo();| have any obvious advantages over |(void)foo();|, other than being a little more explicit?
Reporter

Comment 9

7 years ago
unused << suppresses the warning/error. (void) foo() doesn't.
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.
https://hg.mozilla.org/mozilla-central/rev/45ffd4b20e11
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Depends on: 754198
Depends on: 754160

Comment 12

7 years ago
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]

Updated

7 years ago
Depends on: 754502
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla15 → ---

Updated

7 years ago
Depends on: 754582
I thought our plan for -Werror was that we'd enable it only on the tinderboxen, so as to avoid exactly this problem?
Reporter

Updated

7 years ago
Duplicate of this bug: 755034
(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.
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.
Depends on: 833931
Actually, comment 11 was the wrong cset for this bug -- the correct cset was:
 https://hg.mozilla.org/mozilla-central/rev/6a7bfd84596e
In my experience, -Wreturn-type is the only warning that can be made globally fatal without negative consequences.
OS: Linux → All
Hardware: x86_64 → All
Summary: Use -Werror=unused-result → Use -Werror=unused-result (i.e. make unused-result build warnings fatal)
Version: unspecified → Trunk
Reporter

Updated

6 years ago
Assignee: benjamin → nobody
(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)
Status: REOPENED → RESOLVED
Closed: 7 years ago2 years ago
Depends on: 1198334
Resolution: --- → WORKSFORME
Attachment #606602 - Attachment is obsolete: true
Attachment #619956 - Attachment is obsolete: true

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.