C4334 and C4595 warnings from VS 2015 Update 2 should be investigated |
|
Issue descriptionVC++ 2015 Update 2 emits lots of C4334 and C4595 warnings when compiling Chromium that are going to be temporarily suppressed to allow for forward progress. For example: third_party\skia\src\core\skedge.cpp(231): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) third_party\icu\source\common\unicode\utypes.h(415): warning C4595: 'operator new': non-member operator new or delete functions may not be declared inline Some of these warnings are debug only and some are 64-bit only and some appear (I believe) only in gn builds, due to different warning suppression settings. None of the shift related warnings have yet proven to indicate an actual bug of any sort. 32-bit release builds compile and the remaining warnings appear to be debug code (skia) or code where the destination is a size_t. The warnings should be investigated and fixed as appropriate in the future, but as these are new warnings not a new problem there is no requirement that these block the VS 2015 upgrade. What steps will reproduce the problem? 1. Install VS 2015 Update 2 latest 2. set GYP_MSVS_VERSION=2015&set GYP_DEFINES=target_arch=x64& set DEPOT_TOOLS_WIN_TOOLCHAIN=0 3. Remove 4311 and 4312 warning disables from common.gypi 4. Build a debug 64-bit configuration to expose maximum warnings
,
Mar 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/58552f9fcee335a8704f0c19472fde26ade8a7b9 commit 58552f9fcee335a8704f0c19472fde26ade8a7b9 Author: brucedawson <brucedawson@chromium.org> Date: Thu Mar 10 02:28:47 2016 Suppress new C4334 and C4595 warnings These two warnings are new in VS 2015 Update 2 (C4334 isn't necessarily brand new, but it fires much more frequently now). Therefore they are preventing a switch to VS 2015, without actually indicating any new issues. The possible issues pointed to by C4334, in particular, have been benign in every case that I have investigated. Therefore the pragmatic thing to do is to suppress them for now, and resolve the warnings individually after VS 2015 is the default. The 4595 warning suppression is only needed in third_party\icu but it is placed globally temporarily. These warnings are also blocking PGO builds - another reason why a timely fix is important. BUG= 440500 ,593448, 591920 Review URL: https://codereview.chromium.org/1781593004 Cr-Commit-Position: refs/heads/master@{#380312} [modify] https://crrev.com/58552f9fcee335a8704f0c19472fde26ade8a7b9/build/common.gypi [modify] https://crrev.com/58552f9fcee335a8704f0c19472fde26ade8a7b9/build/config/compiler/BUILD.gn
,
Mar 30 2016
Bug http://bugs.icu-project.org/trac/ticket/12443 filed for the C4595 warnings.
,
Mar 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/angle/angle/+/e2fcf5c319ba9418403ab1569a5f3728bf628314 commit e2fcf5c319ba9418403ab1569a5f3728bf628314 Author: Bruce Dawson <brucedawson@chromium.org> Date: Wed Mar 30 18:27:12 2016 Fix C4434 warnings about 32-bit shift assigned to 64-bits VS 2015 has a new or louder warning about 32-bit shifts that are then assigned to a 64-bit target. This code triggers it: int64_t size = 1 << shift_amount; Because the '1' being shifted is a 32-bit int the result of the shift will be a 32-bit result, so assigning it to a 64-bit variable is just misleading. In other cases the destination is a size_t which means that the warning only shows up on 64-bit builds. However in this case the size_t was later cast to a 32-bit type, so the warnings can be suppressed by selecting more natural types and *deleting* some casts. The two warnings were: C4334: '<<': result of 32-bit shift implicitly converted to 64 bits third_party\angle\src\tests\gl_tests\framebufferrendermipmaptest.cpp(90) third_party\angle\src\tests\gl_tests\framebufferrendermipmaptest.cpp(156) BUG=593448 Change-Id: Ice9f67096b155fbb5fa3247ad04ac41acffa36a5 Reviewed-on: https://chromium-review.googlesource.com/336332 Reviewed-by: Jamie Madill <jmadill@chromium.org> Commit-Queue: Jamie Madill <jmadill@chromium.org> [modify] https://crrev.com/e2fcf5c319ba9418403ab1569a5f3728bf628314/src/tests/gl_tests/FramebufferRenderMipmapTest.cpp
,
Mar 31 2016
The following revision refers to this bug: https://skia.googlesource.com/skia.git/+/cce19c821ce0a76886078c6df24fba57fd2f12de commit cce19c821ce0a76886078c6df24fba57fd2f12de Author: brucedawson <brucedawson@chromium.org> Date: Thu Mar 31 12:53:44 2016 Fix C4334 warning about 32-bit shift assigned to 64-bits VS 2015 has a new or louder warning about 32-bit shifts that are then assigned to a 64-bit target. This type of code triggers it: int64_t size = 1 << shift_amount; Because the '1' being shifted is a 32-bit int the result of the shift will be a 32-bit result, so assigning it to a 64-bit variable is just misleading. In this case the code that triggers it is this: size_t alloc = 1<<fLgSize++; The destination is a size_t so the warning only shows up on 64-bit builds and doesn't indicate a real bug. But, casting the '1' constant to size_t makes the behavior/intent more obvious and consistent and allows enabling C4334 in Chromium. BUG=593448 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1845123002 Review URL: https://codereview.chromium.org/1845123002 [modify] https://crrev.com/cce19c821ce0a76886078c6df24fba57fd2f12de/src/core/SkVarAlloc.cpp
,
Apr 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5565f0c9c54cf854319007378631b55b67aea488 commit 5565f0c9c54cf854319007378631b55b67aea488 Author: ochang <ochang@chromium.org> Date: Fri Apr 01 12:33:21 2016 Roll PDFium ac88953..b33dfdf https://pdfium.googlesource.com/pdfium.git/+log/ac88953..b33dfdf BUG=593448, 599526 TBR=tsepez@chromium.org Review URL: https://codereview.chromium.org/1850893002 Cr-Commit-Position: refs/heads/master@{#384562} [modify] https://crrev.com/5565f0c9c54cf854319007378631b55b67aea488/DEPS
,
Apr 2 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6a5b73fc8615892260d6f47242d1d6a642e3dcfc commit 6a5b73fc8615892260d6f47242d1d6a642e3dcfc Author: jmadill <jmadill@chromium.org> Date: Sat Apr 02 01:36:02 2016 Roll ANGLE 00f394e..0c0d800 https://chromium.googlesource.com/angle/angle.git/+log/00f394e..0c0d800 BUG=593448, 598944 , 595836 ,590870 TBR=geofflang@chromium.org TEST=bots CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.linux:linux_optional_gpu_tests_rel Review URL: https://codereview.chromium.org/1849013003 Cr-Commit-Position: refs/heads/master@{#384777} [modify] https://crrev.com/6a5b73fc8615892260d6f47242d1d6a642e3dcfc/DEPS
,
Apr 5 2016
http://bugs.icu-project.org/trac/ticket/12443 is a duplicate of http://bugs.icu-project.org/trac/ticket/11122 which was filed due to clang's no-inline-new-delete warning.
,
Apr 11 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c8615da7ebb48668de21b3e23edec64c1685ffbb commit c8615da7ebb48668de21b3e23edec64c1685ffbb Author: brucedawson <brucedawson@chromium.org> Date: Mon Apr 11 21:37:41 2016 Fix C4334 (32-bit shift converted to 64-bit) warnings VS 2015's warning about 32-bit shifts that are then assigned to a 64-bit target fires more frequently than in VS 2013. This type of code triggers it: int64_t size = 1 << shift_amount; Because the '1' being shifted is a 32-bit int the result of the shift will be a 32-bit result, so assigning it to a 64-bit variable is just misleading and can lead to undefined behavior and lost bits. During the port to VS 2015 this warning was globally suppressed. This removes that global suppression. There were ~nine C4334 warnings in Chromium with most of them being in external repos such as pdfium, skia, webrtc, and angle. The external repos have all been fixed. This fixes the Chromium repo warnings and enables C4334 in gn builds. In these cases the code that triggers it was assigning to a size_t so it only showed up on 64-bit builds. In some cases there was already a cast but it was after the shift, which is not as good as before the shift. In one case the 32-bit constant was completely superfluous. The Chromium specific warnings were: net\base\mime_sniffer_perftest.cc(93) third_party\webkit\source\platform\heap\heappage.cpp(738) net\spdy\hpack\hpack_huffman_table.cc(172) media\filters\vp8_parser.cc(157) warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits BUG=593448 Review URL: https://codereview.chromium.org/1858423002 Cr-Commit-Position: refs/heads/master@{#386480} [modify] https://crrev.com/c8615da7ebb48668de21b3e23edec64c1685ffbb/build/common.gypi [modify] https://crrev.com/c8615da7ebb48668de21b3e23edec64c1685ffbb/build/config/compiler/BUILD.gn [modify] https://crrev.com/c8615da7ebb48668de21b3e23edec64c1685ffbb/media/filters/vp8_parser.cc [modify] https://crrev.com/c8615da7ebb48668de21b3e23edec64c1685ffbb/net/base/mime_sniffer_perftest.cc [modify] https://crrev.com/c8615da7ebb48668de21b3e23edec64c1685ffbb/net/spdy/hpack/hpack_huffman_table.cc [modify] https://crrev.com/c8615da7ebb48668de21b3e23edec64c1685ffbb/third_party/WebKit/Source/platform/heap/HeapPage.cpp
,
Nov 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c6e883ee8cf3b20541b256b9a2154af03445a3fc commit c6e883ee8cf3b20541b256b9a2154af03445a3fc Author: brucedawson <brucedawson@chromium.org> Date: Thu Nov 10 21:51:53 2016 Stop disabling warning 4595 - inline operator new ICU used to define operator new/delete inline, but it no longer does so we don't need to suppress this VS 2015/clang warning anymore. http://bugs.icu-project.org/trac/ticket/11122 BUG=593448 Review-Url: https://codereview.chromium.org/2494793002 Cr-Commit-Position: refs/heads/master@{#431369} [modify] https://crrev.com/c6e883ee8cf3b20541b256b9a2154af03445a3fc/build/config/compiler/BUILD.gn |
|
►
Sign in to add a comment |
|
Comment 1 by brucedaw...@chromium.org
, Mar 9 2016