gcc -Wtype-limits warnings in consensus/params.h

Rspigler commented at 3:53 am on July 30, 2021: contributor

Expected behavior

Testing PR’s, I build from source without errors.

Actual behavior

Occasionally, I get this repeated output:

./consensus/params.h: In function ‘constexpr bool Consensus::ValidDeployment(Consensus::BuriedDeployment)’: ./consensus/params.h:26:85: warning: comparison is always true due to limited range of data type [-Wtype-limits] constexpr bool ValidDeployment(BuriedDeployment dep) { return DEPLOYMENT_HEIGHTINCB <= dep && dep <= DEPLOYMENT_SEGWIT; } ~~~~~~~~~~~~~~~~~~~~~~^~~~~~ ./consensus/params.h: In function ‘constexpr bool Consensus::ValidDeployment(Consensus::DeploymentPos)’: ./consensus/params.h:34:81: warning: comparison is always true due to limited range of data type [-Wtype-limits] constexpr bool ValidDeployment(DeploymentPos dep) { return DEPLOYMENT_TESTDUMMY <= dep && dep <= DEPLOYMENT_TAPROOT; }

To reproduce

This happens sporadically, and I have not been able to figure out how to reproduce

System information

Compiling master/testing various PR’s.

Qubes 4.0.4, in a Debian-10 standalone-VM with normal networking. Intel x86_64. SSD

Nothing in debug file. Bitcoin-qt still successfully compiles and opens.

Rspigler added the label Bug on Jul 30, 2021

MarcoFalke commented at 7:43 am on July 30, 2021: member

What is the version of the compiler?

tryphe commented at 7:46 am on July 30, 2021: contributor

This is to be expected, #19438 was merged recently which introduced these warnings.

Testing PR’s, I build from source without errors.

When you are compiling other branches and you don’t see the warning, you are compiling other branches that don’t have the changes in #19438.

Also, note that a warning should not be considered an error. Warnings could indicate problems, but not necessarily.

This happens sporadically, and I have not been able to figure out how to reproduce

When you compile, each object file namespace.o is built once. If you recompile, it may use the cache that’s around from last build, and it won’t rebuild the objects that haven’t changed. If it uses the cache and doesn’t compile the object, you won’t see the warning. If you don’t want to use the cache, use make clean to delete the build files, then run make as usual.

tryphe commented at 7:53 am on July 30, 2021: contributor

Fwiw, I’m also running Debian 10 using gcc 8.3 and can confirm the warning (I assumed other platforms would see it as well though)

jonatack commented at 7:54 am on July 30, 2021: member

I’m not seeing it with clang 11 and 13. Trying with Debian gcc 10.2.1 now. Edit: no warning with gcc 10.2.1.

Rspigler commented at 7:47 pm on July 30, 2021: contributor

@MarcoFalke gcc 8.3.0 @tryphe That makes sense, thanks. Weird that @jonatack does not see it as well. I will wait for others to confirm before closing?

tryphe commented at 10:26 pm on July 30, 2021: contributor

I think it’s okay to leave this open. A warning should be fixed, although the message is nothing to be concerned about. When the dummy value changes in the future, it could fix the warning (edit: hmm, but it also happens with DEPLOYMENT_HEIGHTINCB)

tryphe commented at 10:58 pm on July 30, 2021: contributor

I’m not sure I can grok the purpose of the runtime check DEPLOYMENT_HEIGHTINCB <= dep here. We want DEPLOYMENT_HEIGHTINCB to be the lowest value in the range, but it should already be guaranteed. Can we just use some compile-time assertions to make sure?:

0static_assert(DEPLOYMENT_HEIGHTINCB < DEPLOYMENT_CLTV, "DEPLOYMENT_HEIGHTINCB is not the lowest value in range");
1static_assert(DEPLOYMENT_HEIGHTINCB < DEPLOYMENT_DERSIG, "DEPLOYMENT_HEIGHTINCB is not the lowest value in range");
2static_assert(DEPLOYMENT_HEIGHTINCB < DEPLOYMENT_CSV, "DEPLOYMENT_HEIGHTINCB is not the lowest value in range");
3static_assert(DEPLOYMENT_HEIGHTINCB < DEPLOYMENT_SEGWIT, "DEPLOYMENT_HEIGHTINCB is not the lowest value in range");

One of the warnings is gone and the runtime check is no longer necessary.

This solution is a bit heavy handed though and kind of redundant, but maybe I’m missing the point of the runtime comparison.

tryphe commented at 1:10 am on July 31, 2021: contributor

ping @ajtowns :point_up:

MarcoFalke commented at 6:14 am on July 31, 2021: member

I’m not sure I can grok the purpose of the runtime check DEPLOYMENT_HEIGHTINCB <= dep here.

enums are just integers and there is nothing that forbids out-of-range enum values in the C++ standard, so a check is needed here to avoid UB

ajtowns commented at 7:29 am on July 31, 2021: member

Yuck. gcc’s giving a warning because the check is redundant since both TESTDUMMY and HEIGHTINCB have the minimum value for their corresponding underlying type, so there’s no lower value possible.

Could avoid the problem by having ValidDeployment just check the upper bound dep <= DEPLOYMENT_SEGWIT and dep < MAX_VERSION_BITS_DEPLOYMENTS, and adding static asserts for the lower bounds in deploymentstatus.cpp something like:

0#include <type_traits>
1
2/* ValidDeployment only checks upper bounds for ensuring validity,
3 * so check that we really start at the lowest possible value */
4
5template<typename T, typename X, typename U=typename std::underlying_type<T>::type>
6static constexpr bool is_minimum(const X x) { return std::is_same<T,X>::value && x == std::numeric_limits<U>::min(); }
7
8static_assert(is_minimum<Consensus::BuriedDeployment>(Consensus::DEPLOYMENT_HEIGHTINCB), "heightincb is not minimum value for BuriedDeployment");
9static_assert(is_minimum<Consensus::DeploymentPos>(Consensus::DEPLOYMENT_TESTDUMMY), "testdummy is not minimum value for DeploymentPos");

tryphe commented at 2:13 am on August 1, 2021: contributor

Thanks for that! I am running the changes above and the warning is gone on gcc 8.3. Let me know if you want to create a PR. I can do it if you’d like.

enums are just integers and there is nothing that forbids out-of-range enum values in the C++ standard, so a check is needed here to avoid UB

I’m not sure I understand. dep is 16 bits. DEPLOYMENT_HEIGHTINCB is -32768. If something is lower, it doesn’t compile. How is it possible to be out of range?

MarcoFalke commented at 6:25 am on August 1, 2021: member

How is it possible to be out of range?

Sorry, didn’t look close enough on the code. I commented on why this type of check is needed in general.

ajtowns commented at 8:56 am on August 1, 2021: member

Thanks for that! I am running the changes above and the warning is gone on gcc 8.3. Let me know if you want to create a PR. I can do it if you’d like.

See #22597

laanwj renamed this:
~~Compiler Output~~
gcc -Wtype-limits warnings in consensus/params.h
on Aug 2, 2021

katesalazar commented at 11:21 am on August 2, 2021: contributor

Once and if you fix this in the master branch, it’d be lovely if you backport to the 22.x branch, this is happening in the v22.0rc1 tag too.

MarcoFalke referenced this in commit 31a481be29 on Aug 3, 2021

MarcoFalke closed this on Aug 3, 2021

sidhujag referenced this in commit 4c76de657b on Aug 4, 2021

DrahtBot locked this on Aug 18, 2022

gcc -Wtype-limits warnings in consensus/params.h #22587