silenc gcc Werror=logical-op

[divinity76]

ref #379

/home/travis/build/php/php-src/ext/hash/blake3/upstream_blake3/c/blake3.c: In function ‘compress_subtree_to_parent_node’:

/home/travis/build/php/php-src/ext/hash/blake3/upstream_blake3/c/blake3.c:354:22: error: logical ‘and’ of mutually exclusive tests is always false [-Werror=logical-op]

  354 |   while (num_cvs > 2 && num_cvs <= MAX_SIMD_DEGREE_OR_2) {

      |                      ^~

cc1: all warnings being treated as errors

make: *** [Makefile:1910: ext/hash/blake3/upstream_blake3/c/blake3.lo] Error 1

should resolve #379

[divinity76]

per the instructions in the code above, i have confirmed that this silence gcc8.5.0 from the tarball https://ftp.gnu.org/gnu/gcc/gcc-8.5.0/gcc-8.5.0.tar.xz

hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/BLAKE3/c$ gcc -Werror=logical-op example.c blake3.c blake3.h blake3_dispatch.c
blake3_impl.h blake3_portable.c -DBLAKE3_NO_AVX512 -DBLAKE3_NO_SSE41 -DBLAKE3_NO_SSE2 -DBLAKE3_NO_AVX2
blake3.c: In function ‘compress_subtree_to_parent_node’:
blake3.c:354:22: error: logical ‘and’ of mutually exclusive tests is always false [-Werror=logical-op]
   while (num_cvs > 2 && num_cvs <= MAX_SIMD_DEGREE_OR_2) {
                      ^~
cc1: some warnings being treated as errors
hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/BLAKE3/c$ nano blake3.c
hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/BLAKE3/c$ gcc -Werror=logical-op example.c blake3.c blake3.h blake3_dispatch.c blake3_impl.h blake3_portable.c -DBLAKE3_NO_AVX512 -DBLAKE3_NO_SSE41 -DBLAKE3_NO_SSE2 -DBLAKE3_NO_AVX2
hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/BLAKE3/c$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/temp/gcc-8.5.0/bin/usr/local/bin/../libexec/gcc/x86_64-pc-linux-gnu/8.5.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ./configure --disable-multilib
Thread model: posix
gcc version 8.5.0 (GCC)
hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/BLAKE3/c$

[oconnor663]

Gosh, we've made several attempts to silence these warnings, and it still hasn't worked! But this looks promising. If we're just going to #ifdef the whole loop away in the degree=2 case, maybe we don't need the num_cvs <= MAX_SIMD_DEGREE_OR_2 part of the condition? (As per the comment there, that was added in a previous attempt to silence these warnings.)

[divinity76]

if the statement

// The second half of this loop condition is always true

is correct, then yes we can remove it :)

that means it is impossible for MAX_SIMD_DEGREE_OR_2 to be 4 and num_cvs >=5? I don't know the codepath above well enough to say, but hopefully the person who wrote the comment does 👍

remove it? ping @oconnor663

[BurningEnlightenment]

@divinity76 as you can see in the file blame, the loop condition has been introduced in an attempt to suppress buffer overflow warnings related to memcpying past the end of out_array (specifically b8e2dda), so it is fine to remove it as long as no warnings are emitted. Also please move the #if further up to include the out_array definition, otherwise people might be hammered with unused variable warnings.

[divinity76]

thanks, removed the second while condition, and moved the #if higher up 👍

[BurningEnlightenment]

Looks good to me assuming you verified that GCC 8.5 doesn't emit any new warnings.

[divinity76]

i don't usually keep a copy of gcc8.5.0 around, but yes tested it now, it works:

hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/usr/local/bin/BLAKE3/c$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/temp/gcc-8.5.0/bin/usr/local/bin/../libexec/gcc/x86_64-pc-linux-gnu/8.5.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ./configure --disable-multilib
Thread model: posix
gcc version 8.5.0 (GCC) 
hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/usr/local/bin/BLAKE3/c$ git diff
diff --git a/c/blake3_impl.h b/c/blake3_impl.h
index beab5cf..9043470 100644
--- a/c/blake3_impl.h
+++ b/c/blake3_impl.h
@@ -71,8 +71,8 @@ enum blake3_flags {
 
 // There are some places where we want a static size that's equal to the
 // MAX_SIMD_DEGREE, but also at least 2.
-#define MAX_SIMD_DEGREE_OR_2 (MAX_SIMD_DEGREE > 2 ? MAX_SIMD_DEGREE : 2)
-
+//#define MAX_SIMD_DEGREE_OR_2 (MAX_SIMD_DEGREE > 2 ? MAX_SIMD_DEGREE : 2)
+#define MAX_SIMD_DEGREE_OR_2 2
 static const uint32_t IV[8] = {0x6A09E667UL, 0xBB67AE85UL, 0x3C6EF372UL,
                                0xA54FF53AUL, 0x510E527FUL, 0x9B05688CUL,
                                0x1F83D9ABUL, 0x5BE0CD19UL};
hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/usr/local/bin/BLAKE3/c$ gcc -Wall -Wextra -Wpedantic -Werror=logical-op example.c blake3.c blake3.h blake3_dispatch.c blake3_impl.h blake3_portable.c -DBLAKE3_NO_AVX512 -DBLAKE3_NO_SSE41 -DBLAKE3_NO_SSE2 -DBLAKE3_NO_AVX2
blake3.c: In function ‘compress_subtree_to_parent_node’:
blake3.c:354:22: error: logical ‘and’ of mutually exclusive tests is always false [-Werror=logical-op]
   while (num_cvs > 2 && num_cvs <= MAX_SIMD_DEGREE_OR_2) {
                      ^~
cc1: some warnings being treated as errors
hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/usr/local/bin/BLAKE3/c$ cd ..
hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/usr/local/bin/BLAKE3$ curl 'https://github.com/BLAKE3-team/BLAKE3/pull/380.diff' -L | git apply -
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100  1641    0  1641    0     0   4403      0 --:--:-- --:--:-- --:--:--  4403
hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/usr/local/bin/BLAKE3$ cd c
hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/usr/local/bin/BLAKE3/c$ git diff
diff --git a/c/blake3.c b/c/blake3.c
index 692f4b0..e87193c 100644
--- a/c/blake3.c
+++ b/c/blake3.c
@@ -341,21 +341,22 @@ INLINE void compress_subtree_to_parent_node(
   size_t num_cvs = blake3_compress_subtree_wide(input, input_len, key,
                                                 chunk_counter, flags, cv_array);
   assert(num_cvs <= MAX_SIMD_DEGREE_OR_2);
-
-  // If MAX_SIMD_DEGREE is greater than 2 and there's enough input,
-  // compress_subtree_wide() returns more than 2 chaining values. Condense
-  // them into 2 by forming parent nodes repeatedly.
-  uint8_t out_array[MAX_SIMD_DEGREE_OR_2 * BLAKE3_OUT_LEN / 2];
-  // The second half of this loop condition is always true, and we just
+  // This condition is always true, and we just
   // asserted it above. But GCC can't tell that it's always true, and if NDEBUG
   // is set on platforms where MAX_SIMD_DEGREE_OR_2 == 2, GCC emits spurious
   // warnings here. GCC 8.5 is particularly sensitive, so if you're changing
   // this code, test it against that version.
-  while (num_cvs > 2 && num_cvs <= MAX_SIMD_DEGREE_OR_2) {
+#if MAX_SIMD_DEGREE_OR_2 > 2
+  // If MAX_SIMD_DEGREE_OR_2 is greater than 2 and there's enough input,
+  // compress_subtree_wide() returns more than 2 chaining values. Condense
+  // them into 2 by forming parent nodes repeatedly.
+  uint8_t out_array[MAX_SIMD_DEGREE_OR_2 * BLAKE3_OUT_LEN / 2];
+  while (num_cvs > 2) {
     num_cvs =
         compress_parents_parallel(cv_array, num_cvs, key, flags, out_array);
     memcpy(cv_array, out_array, num_cvs * BLAKE3_OUT_LEN);
   }
+#endif
   memcpy(out, cv_array, 2 * BLAKE3_OUT_LEN);
 }
 
diff --git a/c/blake3_impl.h b/c/blake3_impl.h
index beab5cf..9043470 100644
--- a/c/blake3_impl.h
+++ b/c/blake3_impl.h
@@ -71,8 +71,8 @@ enum blake3_flags {
 
 // There are some places where we want a static size that's equal to the
 // MAX_SIMD_DEGREE, but also at least 2.
-#define MAX_SIMD_DEGREE_OR_2 (MAX_SIMD_DEGREE > 2 ? MAX_SIMD_DEGREE : 2)
-
+//#define MAX_SIMD_DEGREE_OR_2 (MAX_SIMD_DEGREE > 2 ? MAX_SIMD_DEGREE : 2)
+#define MAX_SIMD_DEGREE_OR_2 2
 static const uint32_t IV[8] = {0x6A09E667UL, 0xBB67AE85UL, 0x3C6EF372UL,
                                0xA54FF53AUL, 0x510E527FUL, 0x9B05688CUL,
                                0x1F83D9ABUL, 0x5BE0CD19UL};
hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/usr/local/bin/BLAKE3/c$ gcc -Wall -Wextra -Wpedantic -Werror=logical-op example.c blake3.c blake3.h blake3_dispatch.c blake3_impl.h blake3_portable.c -DBLAKE3_NO_AVX512 -DBLAKE3_NO_SSE41 -DBLAKE3_NO_SSE2 -DBLAKE3_NO_AVX2
hans@DESKTOP-EE15SLU:/temp/gcc-8.5.0/bin/usr/local/bin/BLAKE3/c$ echo $?
0

[oconnor663]

Landed and followed up with a comment change in 8fc3618. Thanks for the fix! And also thanks for pinging me to review.