1573501 - media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c:696:19: error: result of comparison 'ulong' (aka 'unsigned long') > 4294967295 is always false [-Werror,-Wtautological-type-limit-compare]

Status: RESOLVED FIXED

(Firefox Build System :: Toolchains, defect)

Description

[(Away)]

The upcoming clang 9 warns about this, which breaks the build due to -Werror.

https://searchfox.org/mozilla-central/rev/30b01f4f60dbcbd6b01500a26b3100c28005cf62/media/webrtc/signaling/src/sdp/sipcc/sdp_utils.c#696

    ulong l_val;
...
        if (l_val > 4294967295UL) {

Comment 1

[(Away)]

(This only happens on 32-bit builds)

Comment 2

[(Away)]

This is actually a more general problem so I'm going to move it into the build system component.

Our code has a pre-existing use of -Wtype-limits: https://searchfox.org/mozilla-central/rev/c7e8bc4996f979e5876b33afae3de3b1ab4f3ae1/build/moz.configure/warnings.configure#59

Apparently clang 8 didn't act on -Wtype-limits. Clang 9 gained support for it in https://reviews.llvm.org/D58841, and the flag enables -Wtautological-type-limit-compare. The latter flag is normally off by default (https://reviews.llvm.org/D41512 talks about exactly this type of "tautological on 32-bit builds but not 64-bit builds" pattern ), but now we're enabling the warning via -Wtype-limits.

I guess we should just disable -Wtautological-type-limit-compare.

Comment 3

[Mike Hommey [:glandium]]

Chris, what do you think?

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to :dmajor from comment #2)

Apparently clang 8 didn't act on -Wtype-limits. Clang 9 gained support for it in https://reviews.llvm.org/D58841, and the flag enables -Wtautological-type-limit-compare. The latter flag is normally off by default (https://reviews.llvm.org/D41512 talks about exactly this type of "tautological on 32-bit builds but not 64-bit builds" pattern ), but now we're enabling the warning via -Wtype-limits.

I guess we should just disable -Wtautological-type-limit-compare.

I think disabling -Wtautological-type-limit-compare globally is fine as long as we keep enabling -Wtype-limits for gcc.

clang's diagnostic docs [1] say -Wtype-limits is now a synonym for -Wtautological-constant-in-range-compare, which enables three sub-warnings [2]. We already explicitly enable the zero-compare sub-warnings [3]. The -Wtautological-type-limit-compare sub-warning, however, is annoying for reasonable code like comment 0 that is written defensively to handle various integer ranges.

The correct warning fix would require #ifdefs or macros for different CPU architectures at compile time (since this is C code and can't use C++'s std::numeric_limits templates). That is still an option because webrtc/signaling is first-party Mozilla code (while webrtc/trunk is upstream WebRTC.org code vendored into mozilla-central).

[1] https://clang.llvm.org/docs/DiagnosticsReference.html#wtype-limits
[2] https://github.com/Barro/compiler-warnings/blame/master/clang/warnings-clang-top-level-8.txt#L620-L629
[3] https://searchfox.org/mozilla-central/rev/c7e8bc4996f979e5876b33afae3de3b1ab4f3ae1/build/moz.configure/warnings.configure#110-111

Comment 5

[(Away)]

Attachment: Disable -Wtautological-type-limit-compare

This warning, new in clang 9, is noisy about patterns like:

unsigned long x = ...
if (x > 4294967295)

The condition is always false on 32-bit and LLP64 systems, but on LP64 it's a valid test.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b5339bfcdaa
Disable -Wtautological-type-limit-compare r=froydnj

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4b5339bfcdaa

Comment 8

[Alexandru Ionescu (needinfo me) [:alexandrui]]

comment deleted