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

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

VC++ 2015 64-bit builds were giving this warning:
crc_folding.c(286): warning C4311: 'type cast': pointer truncation from
  'const unsigned char *' to 'unsigned long'

Converting from unsigned char* to long is normally dodgy but is safe in
this case because of the masking with 0xF. Casting through uintptr_t is
sufficient to allay VC++'s fears that we are making a mistake.

R=pkasting@chromium.org,gavinp@chromium.org
BUG=440500
Committed: https://crrev.com/b474ca01fcc5376eb538f71e524351e52319d396
Cr-Commit-Position: refs/heads/master@{#352900}

[brucedawson, 2015-10-02 22:17:00]

You made the last changes to zlib to suppress conversion warnings so I thought
you'd be a good choice to review this similar change.

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

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

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

https://codereview.chromium.org/1384773002/diff/1/third_party/zlib/crc_folding.c
File third_party/zlib/crc_folding.c (right):

https://codereview.chromium.org/1384773002/diff/1/third_party/zlib/crc_foldin...
third_party/zlib/crc_folding.c:288: algn_diff = 0 - (unsigned long)(size_t)src &
0xF;
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:24]

https://codereview.chromium.org/1384773002/diff/1/third_party/zlib/crc_folding.c
File third_party/zlib/crc_folding.c (right):

https://codereview.chromium.org/1384773002/diff/1/third_party/zlib/crc_foldin...
third_party/zlib/crc_folding.c:288: algn_diff = 0 - (unsigned long)(size_t)src &
0xF;
On 2015/10/05 03:56:04, Peter Kasting wrote:
> 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)?

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]

https://codereview.chromium.org/1384773002/diff/1/third_party/zlib/crc_folding.c
File third_party/zlib/crc_folding.c (right):

https://codereview.chromium.org/1384773002/diff/1/third_party/zlib/crc_foldin...
third_party/zlib/crc_folding.c:288: algn_diff = 0 - (unsigned long)(size_t)src &
0xF;
On 2015/10/05 16:03:23, brucedawson wrote:
> On 2015/10/05 03:56:04, Peter Kasting wrote:
> > 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)?
> 
> 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.

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;

[brucedawson, 2015-10-05 22:10:32]

None of those work as listed because src is a pointer and you can't and a
pointer with an integer.

There are two tidy options. This one is verbose and explicit:

    // Convert from pointer to integral type, and then truncate from pointer
    // size to unsigned long. Separating the two conversions hints to VC++
    // 2015 that the conversions are intentional and avoids warning C4311:
    // 'type cast': pointer truncation from 'const unsigned char *' to 'unsigned
    uintptr_t algn_offset = (uintptr_t)src;
    algn_diff = 0 - (unsigned long)algn_offset & 0xF;

This one is quiet and trivial:
    algn_diff = 0 - (uintptr_t)src & 0xF;

I didn't expect option #2 to work because I thought VC++ would warn on the
truncation from 64-bit integer to 32-bit integer, but it does work. I guess we
have that warning disabled.

The only reason to prefer option #1 would be that it is more likely to stay
warning free if we enable truncation warnings in the future, but I don't find
that very compelling. Option #2 FTW?

[Peter Kasting, 2015-10-05 22:38:51]

On 2015/10/05 22:10:32, brucedawson wrote:
> None of those work as listed because src is a pointer and you can't and a
> pointer with an integer.

Huh!  Good to know.

> There are two tidy options. This one is verbose and explicit:
> 
>     // Convert from pointer to integral type, and then truncate from pointer
>     // size to unsigned long. Separating the two conversions hints to VC++
>     // 2015 that the conversions are intentional and avoids warning C4311:
>     // 'type cast': pointer truncation from 'const unsigned char *' to
'unsigned
>     uintptr_t algn_offset = (uintptr_t)src;
>     algn_diff = 0 - (unsigned long)algn_offset & 0xF;
> 
> This one is quiet and trivial:
>     algn_diff = 0 - (uintptr_t)src & 0xF;
> 
> I didn't expect option #2 to work because I thought VC++ would warn on the
> truncation from 64-bit integer to 32-bit integer, but it does work. I guess we
> have that warning disabled.

I think Visual Studio is smart enough to realize that the mask is guaranteed to
make the value representable in the shorter type.  That's been the case when
I've been fixing "conversion truncates value" sorts of warnings thus far.

So yeah, option 2 is fine with me.

[Peter Kasting, 2015-10-05 22:39:07]

Also pre-emptive LGTM on that so you don't have to round-trip through me again.

[brucedawson, 2015-10-05 22:53:43]

brucedawson@chromium.org changed reviewers:
+ gavinp@chromium.org

[brucedawson, 2015-10-05 22:57:51]

Adding gavinp@ for OWNER approval

I updated the .cc file and the google.patch file (both to have the new diffs and
to remove CRLF line endings that had sullied the previous patch)

[gavinp, 2015-10-07 17:29:24]

lgtm.

I'm very sad here about the sheer quantity of undefined code here, but you're
certainly not making it any worse, so let's go for it. :D

[brucedawson, 2015-10-07 17:40:17]

The CQ bit was checked by brucedawson@chromium.org

[brucedawson, 2015-10-07 17:40:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1384773002/#ps20001
(title: "Simplify patch")

[commit-bot: I haz the power, 2015-10-07 17:40:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1384773002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1384773002/20001

[commit-bot: I haz the power, 2015-10-07 18:46:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-07 18:47:13]

Patchset 2 (id:??) landed as
https://crrev.com/b474ca01fcc5376eb538f71e524351e52319d396
Cr-Commit-Position: refs/heads/master@{#352900}

[scottmg, 2015-10-07 19:33:58]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2015-10-07 19:33:58]

I think this shouldn't have been added to google.patch because crc_folding.c
isn't part of the zlib distribution, it was added by @intel.com people to our
zlib copy.

[brucedawson, 2015-10-07 21:10:54]

On 2015/10/07 19:33:58, scottmg wrote:
> I think this shouldn't have been added to google.patch because crc_folding.c
> isn't part of the zlib distribution, it was added by @intel.com people to our
> zlib copy.

Good catch. gavinp@, should I remove the change from google.patch or just leave
well enough alone?

[gavinp, 2015-10-08 14:02:51]

On 2015/10/07 21:10:54, brucedawson wrote:
> On 2015/10/07 19:33:58, scottmg wrote:
> > I think this shouldn't have been added to google.patch because crc_folding.c
> > isn't part of the zlib distribution, it was added by @intel.com people to
our
> > zlib copy.
> 
> Good catch. gavinp@, should I remove the change from google.patch or just
leave
> well enough alone?

Woops. I think we should probably clean up the patch; it should apply cleanly.

[brucedawson, 2015-10-08 18:01:58]

I created https://codereview.chromium.org/1397813003 to address this.