refactor: Make 64-bit shift explicit

hebasto commented at 4:03 pm on April 30, 2023: member

Required to enable level 3 warnings. See: #1113 (review).

In addition, this PR enables MSVC warning C4334 for the entire codebase.

ci: Treat all compiler warnings as errors in "Windows (VS 2022)" task b2e29e43d0

hebasto cross-referenced this on Apr 30, 2023 from issue build: Meta-issue for follow-ups to initial CMake merge (#1113) by hebasto

real-or-random commented at 8:41 am on May 2, 2023: contributor

Hm, weak concept NACK.

I’m not convinced that it’s worth complicating our source code or spending the time to optimize for MSVC warnings. MSVC warnings are very strange. And warnings are mostly useful for devs. No regular contributors here use MSVC currently, and I don’t expect any future contributors to use MSVC a lot. gcc is so much better even on Windows.

I’d also be fine with what you suggested in the initial CMake PR:

0  try_add_compile_option(/W3)
1  try_add_compile_option(/wd4146)
2  try_add_compile_option(/wd4244)
3  try_add_compile_option(/wd4267)
4  try_add_compile_option(/wd4334)

Adding these just documents the reality on MSVC, but we don’t need to touch our code base. I’m ~0 on #1293. That one could be reasonable because then we have a clean /W2 at least.

fanquake commented at 8:53 am on May 2, 2023: member

I’m not convinced that it’s worth complicating our source code or spending the time to optimize for MSVC warnings.

Agree with this entirely, and we’ve pushed back against these kind of “refactor perfectly working code, for the sake of Windows (warnings)” changes in the Core repository.

hebasto commented at 11:15 am on May 2, 2023: member

I’m not convinced that it’s worth complicating our source code or spending the time to optimize for MSVC warnings.

I agree with this statement, in general. This change, i.e., s/1/1ULL/ does not look like “complicating our source code” at all :)

we’ve pushed back against these kind of “refactor perfectly working code, for the sake of Windows (warnings)” changes in the Core repository.

I strongly disagree with neglecting Windows as a build platform. Diversity is essential for surviving. And diversity of well supported platform for Bitcoin Core is essential in long run (assuming you can’t see the future).

Btw, https://github.com/bitcoin/bitcoin/pull/26252.

hebasto commented at 11:17 am on May 2, 2023: member

@real-or-random

Anyway, feel free to close this PR.

in src/ecmult_impl.h:686 in b07c304c85 outdated

682@@ -683,7 +683,7 @@ static int secp256k1_ecmult_pippenger_batch(const secp256k1_callback* error_call
683     }
684     state_space->ps = (struct secp256k1_pippenger_point_state *) secp256k1_scratch_alloc(error_callback, scratch, entries * sizeof(*state_space->ps));
685     state_space->wnaf_na = (int *) secp256k1_scratch_alloc(error_callback, scratch, entries*(WNAF_SIZE(bucket_window+1)) * sizeof(int));
686-    buckets = (secp256k1_gej *) secp256k1_scratch_alloc(error_callback, scratch, (1<<bucket_window) * sizeof(*buckets));
687+    buckets = (secp256k1_gej *) secp256k1_scratch_alloc(error_callback, scratch, (1ULL << bucket_window) * sizeof(*buckets));

real-or-random commented at 11:47 am on May 2, 2023:

0    buckets = (secp256k1_gej *) secp256k1_scratch_alloc(error_callback, scratch, ((size_t)1 << bucket_window) * sizeof(*buckets));

size_t is the correct type here

hebasto commented at 12:37 pm on May 2, 2023:

Thanks! Updated.

in CMakeLists.txt:210 in b07c304c85 outdated

206@@ -207,6 +207,7 @@ include(TryAppendCFlags)
207 if(MSVC)
208   # Keep the following commands ordered lexicographically.
209   try_append_c_flags(/W2) # Moderate warning level.
210+  try_append_c_flags(/w24334) # Enable warning C4334 "result of 32-bit shift implicitly converted to 64 bits" as a level 2 one.

real-or-random commented at 12:02 pm on May 2, 2023:

I don’t think we should try to enable more warnings here manually.

We may want to go with /W3 (with some explicitly disabled) but this could be part of another PR then, though I’m not entirely if I would to support that change. MSVC is very picky about some things, and we may run into other false positives in the future, requiring us to make more changes etc.

hebasto commented at 12:37 pm on May 2, 2023:

Removed.

real-or-random commented at 12:07 pm on May 2, 2023: contributor

I’m not convinced that it’s worth complicating our source code or spending the time to optimize for MSVC warnings.

I agree with this statement, in general. This change, i.e., s/1/1ULL/ does not look like “complicating our source code” at all :)

Ok, you’re perfectly right here, of course. I think I rather meant “Concept NACK on complicating our source to make MSVC happy”. But that comment is a bit superficial for this PR, which I think indeed actually improves the source code, independently of whether it makes a warning go away.

I strongly disagree with neglecting Windows as a build platform. Diversity is essential for surviving. And diversity of well supported platform for Bitcoin Core is essential in long run (assuming you can’t see the future).

I don’t think we’re neglecting it as a build platform. It’s rather deprioritizing it a bit as a dev platform, and then it’s a question of the right balance. I think we should not overdo it. Compiler warnings are useful in general, but making multiple compilers happy at the same time is difficult. (And anyway, I don’t think disabling warnings on the command line options instead of changing source means neglecting a platform.)

refactor: Make 64-bit shift explicit

This change fixes MSVC level-3 warning C4334.
See: https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4334

Required to enable level 3 warnings (/W3).

d1e48e5474

hebasto force-pushed on May 2, 2023

hebasto commented at 12:36 pm on May 2, 2023: member

Addressed @real-or-random’s comments.

real-or-random approved

real-or-random commented at 2:10 pm on May 2, 2023: contributor

utACK d1e48e5474a2be29e17a477874a4963f8f612a5a

jonasnick commented at 3:04 pm on May 11, 2023: contributor

ACK d1e48e5474a2be29e17a477874a4963f8f612a5a

jonasnick merged this on May 11, 2023

jonasnick closed this on May 11, 2023

hebasto deleted the branch on May 11, 2023

sipa referenced this in commit b4eb644b6c on May 12, 2023

hebasto referenced this in commit 49c52ea2b1 on May 13, 2023

vmta referenced this in commit e1120c94a1 on Jun 4, 2023

sipa referenced this in commit 901336eee7 on Jun 21, 2023

vmta referenced this in commit 8f03457eed on Jul 1, 2023

jonasnick cross-referenced this on Jul 24, 2023 from issue Upstream PRs 1268, 1276, 1267, 1265, 1230, 1279, 1273, 1263, 1231, 1285, 1283, 1205, 1286, 1275, 1234, 1239, 1240, 1284, 1277, 1289, 1270, 1296, 1301, 1299, 1066, 1300, 1292, 1305, 1303, 1133, 1306, 1207, 1304, 1307, 1311, 1309, 1312 by jonasnick

refactor: Make 64-bit shift explicit #1292