1222887 - Fix -Wunreachable-code warning in tools/power

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: Wunreachable-code_tools-power.patch

tools/power/rapl.cpp:844:5 [-Wunreachable-code] code will never be executed

clang reports a -Wunreachable-code warning for sigemptyset() because Darwin's sigemptyset() is a macro that always returns 0. Thus `if (sigemptyset(&sa.sa_mask) < 0)` is always false and `Abort("sigemptyset() failed")` is never called. Linux's sigemptyset() can return 0 or -1.

/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/usr/include/signal.h

#define	sigemptyset(set)	(*(set) = 0, 0)

Comment 1

[Nicholas Nethercote [inactive]]

Comment on attachment 8684753 [details] [diff] [review]
Wunreachable-code_tools-power.patch

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

This is fine for Mac but on Linux it removes the checking, which is bad. I suggest instead either (a) having different code paths for Linux and Mac, or (b) using a pragma to disable -Wunreachable for that line of code. What do you think?

BTW, do you know why this doesn't show up on TreeHerder? This file should be compiled with warnings as errors.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Wunreachable-code_rapl_v2.patch

(In reply to Nicholas Nethercote [:njn] from comment #1)
> Comment on attachment 8684753 [details] [diff] [review]
> Wunreachable-code_tools-power.patch
> 
> Review of attachment 8684753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is fine for Mac but on Linux it removes the checking, which is bad. I
> suggest instead either (a) having different code paths for Linux and Mac, or

Fair enough. :)

> (b) using a pragma to disable -Wunreachable for that line of code. What do
> you think?

This patch uses clang's suggested approach to suppress the warning: add extra parens around the 0 in `if (sigemptyset(&sa.sa_mask) < (0))`.

> BTW, do you know why this doesn't show up on TreeHerder? This file should be
> compiled with warnings as errors.

clang's and gcc's -Wunreachable-code warning is not enabled by default. I'm close to fixing the last dozen -Wunreachable-code warnings in mozilla-central so we can opt into the warning.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8685049 [details] [diff] [review]
Wunreachable-code_rapl_v2.patch

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

Interesting!

Comment 4

[Nicholas Nethercote [inactive]]

> clang's and gcc's -Wunreachable-code warning is not enabled by default. I'm
> close to fixing the last dozen -Wunreachable-code warnings in
> mozilla-central so we can opt into the warning.

Excellent.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54f344e2ec98

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/54f344e2ec98