Remove warning pragmas in pe_image.cc

Remove warning pragmas in pe_image.cc

PEImage::GetProcAddress now returns consistent -1 on both 32-bit and 64-bit. Added a test to verify.

BUG=167707, 554200
Committed: https://crrev.com/a01e999b6c4c23533dca9cdc73ec11a01939fddd
Cr-Commit-Position: refs/heads/master@{#366836}

[Will Harris, 2015-09-26 01:57:39]

wfh@chromium.org changed reviewers:
+ thakis@chromium.org

[Will Harris, 2015-09-26 01:57:49]

PTAL - finish off this bug once and for all.

[Will Harris, 2015-09-26 01:58:18]

If this l-g-t-y then please CQ and close bug as Fixed.

[Will Harris, 2015-12-23 20:22:21]

necro-CL For Christmas PTAL. :)

[Nico, 2015-12-23 20:57:01]

lgtm

https://codereview.chromium.org/1370843003/diff/20001/base/win/pe_image.cc
File base/win/pe_image.cc (right):

https://codereview.chromium.org/1370843003/diff/20001/base/win/pe_image.cc#ne...
base/win/pe_image.cc:214: // Cast from 32-bit value to 64-bit pointer generates
C4312 on 64-bit.
It'd be nice if the comment also said why the warning doesn't point to a bug in
this case.

https://codereview.chromium.org/1370843003/diff/20001/base/win/pe_image.cc#ne...
base/win/pe_image.cc:457: // Casts from 32-bit values to 64-bit pointers
generates C4312 on 64-bit.
ditto

[Will Harris, 2015-12-24 05:32:03]

Description was changed from

==========
Cleanup comments and warning pragmas in pe_image.cc

BUG=167707
==========

to

==========
Remove warning pragmas in pe_image.cc

GetProcaddress now returns -1 on both 32-bit and 64-bit. Added a test to verify.

BUG=167707,554200
==========

[Will Harris, 2015-12-24 05:35:04]

Description was changed from

==========
Remove warning pragmas in pe_image.cc

GetProcaddress now returns -1 on both 32-bit and 64-bit. Added a test to verify.

BUG=167707,554200
==========

to

==========
Remove warning pragmas in pe_image.cc

PEImage::GetProcAddress now returns consistent -1 on both 32-bit and 64-bit.
Added a test to verify.

BUG=167707,554200
==========

[Will Harris, 2015-12-24 05:35:51]

I redid this CL, so it needs another look. Thanks.

https://codereview.chromium.org/1370843003/diff/20001/base/win/pe_image.cc
File base/win/pe_image.cc (right):

https://codereview.chromium.org/1370843003/diff/20001/base/win/pe_image.cc#ne...
base/win/pe_image.cc:214: // Cast from 32-bit value to 64-bit pointer generates
C4312 on 64-bit.
On 2015/12/23 20:57:01, Nico wrote:
> It'd be nice if the comment also said why the warning doesn't point to a bug
in
> this case.

I changed this function to return -1 on both 64-bit and 32-bit and added a test
to verify it.

https://codereview.chromium.org/1370843003/diff/20001/base/win/pe_image.cc#ne...
base/win/pe_image.cc:457: // Casts from 32-bit values to 64-bit pointers
generates C4312 on 64-bit.
On 2015/12/23 20:57:01, Nico wrote:
> ditto

Done. Actually just changed the cast instead of suppressing the warning.

[Nico, 2015-12-24 14:11:12]

Thanks, that's much better! One real comment below (the -1 one), lgtm with that
addressed in some form

https://codereview.chromium.org/1370843003/diff/60001/base/win/pe_image.cc
File base/win/pe_image.cc (right):

https://codereview.chromium.org/1370843003/diff/60001/base/win/pe_image.cc#ne...
base/win/pe_image.cc:214: return reinterpret_cast<FARPROC>(-1);
Does this need a static_cast<intptr_t>(-1) inside the reinterpret_cast? -1 is an
int literal (ie 32-bit), and I couldn't find if the standard that this is
sign-extended when converted to a pointer. (seems to work in practice, though)

https://codereview.chromium.org/1370843003/diff/60001/base/win/pe_image_unitt...
File base/win/pe_image_unittest.cc (right):

https://codereview.chromium.org/1370843003/diff/60001/base/win/pe_image_unitt...
base/win/pe_image_unittest.cc:197: EXPECT_EQ(FARPROC(-1), addr);
same question

https://codereview.chromium.org/1370843003/diff/60001/base/win/pe_image_unitt...
base/win/pe_image_unittest.cc:205: FreeLibrary(module);
(i'm a bit surprised we don't have a ScopedFoo for this. Possibly used often
enough in this file that a ScopedGeneric <> with a FreeLibrary() trait might be
worth it

[Will Harris, 2015-12-24 19:22:04]

Merry Christmas

https://codereview.chromium.org/1370843003/diff/60001/base/win/pe_image.cc
File base/win/pe_image.cc (right):

https://codereview.chromium.org/1370843003/diff/60001/base/win/pe_image.cc#ne...
base/win/pe_image.cc:214: return reinterpret_cast<FARPROC>(-1);
On 2015/12/24 14:11:11, Nico wrote:
> Does this need a static_cast<intptr_t>(-1) inside the reinterpret_cast? -1 is
an
> int literal (ie 32-bit), and I couldn't find if the standard that this is
> sign-extended when converted to a pointer. (seems to work in practice, though)

FARPROC is actually signed :(

I manually verified this works and doesn't warn even with C4312 and C4311
manually enabled, and it also compiles on clang...

I also tried a little test here to verify it == 0xFFFFFFFF (32-bit) and
0xFFFFFFFFFFFFFFFF (64-bit) so existing behavior (at least, on 32-bit) is
maintained.

https://codereview.chromium.org/1370843003/diff/60001/base/win/pe_image_unitt...
File base/win/pe_image_unittest.cc (right):

https://codereview.chromium.org/1370843003/diff/60001/base/win/pe_image_unitt...
base/win/pe_image_unittest.cc:197: EXPECT_EQ(FARPROC(-1), addr);
On 2015/12/24 14:11:11, Nico wrote:
> same question

Acknowledged.

https://codereview.chromium.org/1370843003/diff/60001/base/win/pe_image_unitt...
base/win/pe_image_unittest.cc:205: FreeLibrary(module);
On 2015/12/24 14:11:11, Nico wrote:
> (i'm a bit surprised we don't have a ScopedFoo for this. Possibly used often
> enough in this file that a ScopedGeneric <> with a FreeLibrary() trait might
be
> worth it

yes, we do, it's base::ScopedNativeLibrary so I swapped it in.

[Will Harris, 2015-12-24 20:55:35]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2015-12-24 20:55:35]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1370843003/#ps80001
(title: "use ScopedNativeLibrary")

[commit-bot: I haz the power, 2015-12-24 20:55:45]

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

[commit-bot: I haz the power, 2015-12-24 21:43:14]

Description was changed from

==========
Remove warning pragmas in pe_image.cc

PEImage::GetProcAddress now returns consistent -1 on both 32-bit and 64-bit.
Added a test to verify.

BUG=167707,554200
==========

to

==========
Remove warning pragmas in pe_image.cc

PEImage::GetProcAddress now returns consistent -1 on both 32-bit and 64-bit.
Added a test to verify.

BUG=167707,554200
==========

[commit-bot: I haz the power, 2015-12-24 21:43:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-24 21:44:23]

Description was changed from

==========
Remove warning pragmas in pe_image.cc

PEImage::GetProcAddress now returns consistent -1 on both 32-bit and 64-bit.
Added a test to verify.

BUG=167707,554200
==========

to

==========
Remove warning pragmas in pe_image.cc

PEImage::GetProcAddress now returns consistent -1 on both 32-bit and 64-bit.
Added a test to verify.

BUG=167707,554200

Committed: https://crrev.com/a01e999b6c4c23533dca9cdc73ec11a01939fddd
Cr-Commit-Position: refs/heads/master@{#366836}
==========

[commit-bot: I haz the power, 2015-12-24 21:44:24]

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