Make compiler warn about tautological run-time comparisons

practicalswift commented at 4:10 pm on October 30, 2019: contributor

Submitting as draft pull request: seeking Concept ACK:s.

Tautological run-time checks may be an indication of incorrect logic. Enabling compiler warnings for tautological checks can help identify such issues early in the development cycle.

If it is clear already at compile-time that a run-time check is tautological it is better to make it a compile-time check using static_assert (if possible and assuming the check should be kept).

Compile-time checking is preferred over run-time checking as noted in the developer notes:

static_assert is preferred over assert where possible. Generally; compile-time checking is preferred over run-time checking.

Suggested changes in this PR:

Commit 1. Make compiler warn about tautological comparisons (-Wtautological-constant-in-range-compare in Clang)
Commit 2. Get rid of current warnings by replacing the run-time tautological comparisons with equivalent compile-time assertions (static_assert)

Compiler output given -Wtautological-constant-in-range-compare before this PR:

 0$ make
 1util/strencodings.cpp:304:11: warning: result of comparison 'long long' >= -9223372036854775808 is always true [-Wtautological-type-limit-compare]
 2        n >= std::numeric_limits<int64_t>::min() &&
 3        ~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4util/strencodings.cpp:305:11: warning: result of comparison 'long long' <= 9223372036854775807 is always true [-Wtautological-type-limit-compare]
 5        n <= std::numeric_limits<int64_t>::max();
 6        ~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 7util/strencodings.cpp:338:11: warning: result of comparison 'unsigned long long' <= 18446744073709551615 is always true [-Wtautological-type-limit-compare]
 8        n <= std::numeric_limits<uint64_t>::max();
 9        ~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
10core_write.cpp:100:15: warning: result of comparison of 0 <= unsigned enum expression is always true [-Wtautological-unsigned-enum-zero-compare]
11        if (0 <= opcode && opcode <= OP_PUSHDATA4) {
12            ~ ^  ~~~~~~
13script/script.h:449:20: warning: result of comparison of unsigned enum expression < 0 is always false [-Wtautological-unsigned-enum-zero-compare]
14        if (opcode < 0 || opcode > 0xff)
15            ~~~~~~ ^ ~
16script/script.h:449:34: warning: result of comparison 'opcodetype' > 255 is always false [-Wtautological-type-limit-compare]
17        if (opcode < 0 || opcode > 0xff)
18                          ~~~~~~ ^ ~~~~
19script/interpreter.cpp:343:28: warning: result of comparison of 0 <= unsigned enum expression is always true [-Wtautological-unsigned-enum-zero-compare]
20            if (fExec && 0 <= opcode && opcode <= OP_PUSHDATA4) {
21                         ~ ^  ~~~~~~
22$

Compiler output given -Wtautological-constant-in-range-compare after this PR:

0$ make
1$

build: Warn on tautological comparisons (Clang -Wtautological-constant-in-range-compare) 79149466d9

practicalswift renamed this:
~~Make compiler warn about tautological comparisons~~
Make compiler warn about tautological run-time comparisons
on Oct 30, 2019

DrahtBot added the label Build system on Oct 30, 2019

DrahtBot added the label Consensus on Oct 30, 2019

DrahtBot added the label Utils/log/libs on Oct 30, 2019

Use compile-time checking (static_assert) instead of run-time tautological comparisons 2b53af1208

practicalswift force-pushed on Oct 30, 2019

in src/util/strencodings.cpp:337 in 2b53af1208

333@@ -334,8 +334,8 @@ bool ParseUInt64(const std::string& str, uint64_t *out)
334     if(out) *out = (uint64_t)n;
335     // Note that strtoull returns a *unsigned long long int*, so even if it doesn't report an over/underflow
336     // we still have to check that the returned value is within the range of an *uint64_t*.
337-    return endp && *endp == 0 && !errno &&
338-        n <= std::numeric_limits<uint64_t>::max();
339+    static_assert(std::numeric_limits<decltype(n)>::max() <= std::numeric_limits<uint64_t>::max(), "Assumption: n <= std::numeric_limits<uint64_t>::max()");

laanwj commented at 4:23 pm on October 30, 2019:

I don’t like this change. The old code is written to work for the case where a unsigned long long has a larger range than uint64_t. The new code throws a compile-time assertion error in that case. This is not the same.

laanwj commented at 4:32 pm on October 30, 2019: member

I think this is over-sweeping. A comparison that is tautological on one platform might not be on another.

practicalswift closed this on Oct 30, 2019

practicalswift reopened this on Oct 30, 2019

practicalswift commented at 5:29 pm on October 30, 2019: contributor

@laanwj

If I’m reading Parse{U,}Int64 correctly these functions are not written in a platform independent way today. The “sizeof(unsigned long long) > sizeof(uint64_t) fix” you’re referring to in the current code does not look correct if the intention is that these functions should give identical results across platforms.

More specifically my reading is that input test cases can be constructed such that the output of these functions will differ depending on sizeof(unsigned long long).

Is that your reading too? Please correct me if I’m missing something: if so I apologise in advance.

Assuming that this view is shared (regarding the platform dependence of Parse{U,}Int64), what is the better option when the first user decides to compile Bitcoin Core on a platform where sizeof(unsigned long long) > sizeof(uint64_t):

a.). To give a compile-time error, or
b.). To compile happily and give the user a “unique” Parse{U,}Int64 experience? :)

laanwj commented at 5:46 pm on October 30, 2019: member

The idea is that ParseUint64 rejects values outside the range of uint64_t as parse error. So strtoull could return values outside the range of uint64_t, but this will be treated as a parse error.

On platforms where uint64_t == unsigned long long this works, because, values outside uint64_t will be rejected by strtoull. The comparison is tautological.
On platforms where uint64_t < unsigned long long, this works, because strtoull will return the larger value, but the n <= std::numeric_limits<uint64_t>::max() check detects it is out of range and returns a parse error.

If you think this doesn’t currently work, feel free to fix it. I’m, however, unwilling to add an unnecessary static assertion here.

practicalswift closed this on Oct 30, 2019

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 16, 2022

Make compiler warn about tautological run-time comparisons #17320