Fix VC++ 2015 64-bit truncation warning in zlib

third_party/zlib/crc_folding.c

 /*
  * Compute the CRC32 using a parallelized folding approach with the PCLMULQDQ
  * instruction.
  *
  * A white paper describing this algorithm can be found at:
  * http://www.intel.com/content/dam/www/public/us/en/documents/white-papers/fast-crc-computation-generic-polynomials-pclmulqdq-paper.pdf
  *
  * Copyright (C) 2013 Intel Corporation. All rights reserved.
  * Authors:
  *      Wajdi Feghali   <wajdi.k.feghali@intel.com>
@@ skipping 265 matching lines @@
     __m128i xmm_t0, xmm_t1, xmm_t2, xmm_t3;
 
     CRC_LOAD(s)
 
     if (len < 16) {
         if (len == 0)
             return;
         goto partial;
     }
 
-    algn_diff = 0 - (unsigned long)src & 0xF;
+    // Double cast to avoid truncation errors in 64-bit builds by making the
+    // truncation explicit.
+    algn_diff = 0 - (unsigned long)(size_t)src & 0xF;

[Peter Kasting, 2015/10/05 03:56:04]

I'm sorry, I don't understand what this is doing.

Why did we select size_t as the type here?  Why is this done as a double cast
instead of introducing a temp of some appropriate type?  Frankly, why are we
storing a known-negative value in an unsigned long to begin with (i.e. this code
seems confusingly written to start)?

[brucedawson, 2015/10/05 16:03:23]

The "known-negative in an unsigned" is definitely weird - I was just ignoring
that in order to make the minimal change to suppress the warning.

The warning (complaining about legal but suspicious behavior) is because VC++
2015 thinks it is suspicious that we are converting a pointer to an integer of
smaller size. This often indicates code that will break on 64-bit builds.
Casting to size_t separates out the pointer-to-integer conversion from the
truncation and makes them both explicit. size_t was chosen because it assumed to
be the size of a pointer.

An intermediate variable certainly could be used if that would make it clearer.
I could also improve the comment to better explain the rationale behind the
double cast.

Or, the warning could be suppressed in the .gn/.gyp files so that the source
doesn't need to be touched.

[Peter Kasting, 2015/10/05 18:33:03]

I think changing the source is OK, but maybe one of these methods would work?

  algn_diff = 0 - (unsigned long)(src & 0xF);

  algn_diff = 0 - (uintptr_t)(src & 0xF);

  unsigned long algn_offset = src & 0xF;
  algn_diff = 0 - algn_offset;

  uintptr_t algn_offset = src & 0xF;
  algn_diff = 0 - (unsigned long)algn_offset;

     if (algn_diff) {
         xmm_crc_part = _mm_loadu_si128((__m128i *)src);
         _mm_storeu_si128((__m128i *)dst, xmm_crc_part);
 
         dst += algn_diff;
         src += algn_diff;
         len -= algn_diff;
 
         partial_fold(s, algn_diff, &xmm_crc0, &xmm_crc1, &xmm_crc2, &xmm_crc3,
             &xmm_crc_part);
@@ skipping 187 matching lines @@
 
     xmm_crc2 = xmm_crc3;
     xmm_crc3 = _mm_clmulepi64_si128(xmm_crc3, crc_fold, 0x10);
     xmm_crc3 = _mm_xor_si128(xmm_crc3, xmm_crc2);
     xmm_crc3 = _mm_xor_si128(xmm_crc3, xmm_crc1);
 
     crc = _mm_extract_epi32(xmm_crc3, 2);
     return ~crc;
     CRC_SAVE(s)
 }