Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1422773008:
Fixing remaining VC++ 2015 64-bit build breaks (Closed)
|
Created:
5 years, 1 month ago by brucedawson Modified:
5 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, cbentzel+watch_chromium.org, szager+layoutwatch_chromium.org, leviw+renderwatch, Peter Beverloo, wfh+watch_chromium.org, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, jam, rickyz+watch_chromium.org, eae+blinkwatch, blink-reviews-events_chromium.org, zoltan1, dglazkov+blink, darin-cc_chromium.org, jchaffraix+rendering, blink-reviews, mkwst+moarreviews-shell_chromium.org, blink-reviews-wtf_chromium.org, jochen+watch_chromium.org, Mikhail Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixing remaining VC++ 2015 64-bit build breaks
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them (4311 and 4312). Others seem
to find some real bugs so I am leaving them enabled for now (4302 and
4334), fixing the bugs, and adjusting the code to suppress the other
warnings.
The changes in shell_web_contents_view_delegate_win.cc,
ipc_perftest_support.cc, test_root_certs_win.cc, TracedLayoutObject.*
and TraceEvent.h appear to fix real truncation bugs.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic. The warnings in the
generated code were suppressed because they aren't bugs and because
fixing gperf.exe (untouched since 2004) does not seem worthwhile.
The warnings from gperf generated code look like:
out\release_x64\gen\blink\core\csspropertynames.cpp(2914): warning C4302:
'type cast': truncation from 'char (*)[28]' to 'long'
out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302:
'type cast': truncation from 'char (*)[4]' to 'long'
Note that Windows HANDLE types are a wrapper around a pointer, but due
to WOW64 (32-bit process on 64-bit Windows) compatibility they are
always 32-bit values that can safely be truncated, then sign-extended
back. Unfortunately Microsoft does not supply a safe-truncation macro.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%29.aspx
Also a few uncommented warning-disables in common.gypi were commented.
Setting NOTRY=true because the change is hitting timeouts. See
crbug.com/567377 and comment #119 for details.
NOTRY=true
BUG=440500
Committed: https://crrev.com/d3beca7e8267b3db4e8245eb588147639a1b1bd2
Cr-Commit-Position: refs/heads/master@{#364264}
Patch Set 1 #Patch Set 2 : Updated to latest origin/master #Patch Set 3 : Use uintptr_t instead of size_t #Patch Set 4 : More tweaks to the fixes. #Patch Set 5 : Disable C4302 for two generated files. #Patch Set 6 : Fixing test comment #Patch Set 7 : Sync to latest #
Total comments: 26
Patch Set 8 : Some CR tweaks #
Total comments: 6
Patch Set 9 : Switch to HandleToUint32 #Patch Set 10 : Adding #ifdefs #
Total comments: 2
Patch Set 11 : Spot suppression of C4311 and global suppression of C4312 #
Total comments: 2
Patch Set 12 : Supply reference for handles-are-32-bits claim #
Total comments: 13
Patch Set 13 : Comment and formatting tweaks #Patch Set 14 : git pull to latest code #Patch Set 15 : More use of HandleToUint32 #
Total comments: 6
Patch Set 16 : Removing unnecessary #if defined(s) #
Total comments: 3
Patch Set 17 : Remove more unneeded #ifs #Patch Set 18 : Pull to latest #Patch Set 19 : git pull #Patch Set 20 : Removing unneeded include #Messages
Total messages: 126 (43 generated)
Description was changed from
==========
First pass at fixing VC++ 2015 64-bit builds
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit shift
implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
Lots of these still need fixing:
out\release_x64\gen\blink\core\csspropertynames.cpp(2468): warning C4302: 'type
cast': truncation from 'char (*)[2]' to 'long'
out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302: 'type
cast': truncation from 'char (*)[4]' to 'long'
Plus a third_party\webrtc fix that needs landing.
BUG=440500
==========
to
==========
First pass at fixing VC++ 2015 64-bit builds
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them. Others seem to find some
real bugs so I am leaving them enabled for now, fixing the bugs, and
adjusting the code to suppress the other warnings.
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
Lots of these still need fixing:
warning C4302: 'type cast': truncation from 'char (*)[2]' to 'long'
These are occurring in these two generated files:
out\release_x64\gen\blink\core\csspropertynames.cpp
out\release_x64\gen\blink\core\cssvaluekeywords.cpp
There is also a third_party\webrtc fix that needs landing.
BUG=440500
==========
Description was changed from
==========
First pass at fixing VC++ 2015 64-bit builds
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them. Others seem to find some
real bugs so I am leaving them enabled for now, fixing the bugs, and
adjusting the code to suppress the other warnings.
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
Lots of these still need fixing:
warning C4302: 'type cast': truncation from 'char (*)[2]' to 'long'
These are occurring in these two generated files:
out\release_x64\gen\blink\core\csspropertynames.cpp
out\release_x64\gen\blink\core\cssvaluekeywords.cpp
There is also a third_party\webrtc fix that needs landing.
BUG=440500
==========
to
==========
Fixing most VC++ 2015 64-bit build breaks
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them (4311 and 4312). Others seem
to find some real bugs so I am leaving them enabled for now (4302 and
4334), fixing the bugs, and adjusting the code to suppress the other
warnings.
The changes in shell_web_contents_view_delegate_win.cc,
ipc_perftest_support.cc, test_root_certs_win.cc, and TraceEvent.h appear
to fix real bugs.
The change in handle_inheritance_test.cc might fix a real bug.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic. The warnings in the
generated code were suppressed because they aren't bugs and because
fixing gperf.exe (untouched since 2004) does not seem worthwhile.
Note that Windows HANDLE types are a wrapper around a pointer, but due
to WOW64 (32-bit process on 64-bit Windows) compatibility they are
always 32-bit values that can safely be truncated, then sign-extended
back. Unfortunately Microsoft does not supply a safe-truncation macro.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2...
Also a few uncommented warning-disables in common.gypi were commented.
BUG=440500
==========
Description was changed from ========== Fixing most VC++ 2015 64-bit build breaks VC++ 2015 64-bit builds trigger many new warnings about possibly unintended truncation bugs. Some of the bugs have a very low signal-to- noise ratio and this change suppresses them (4311 and 4312). Others seem to find some real bugs so I am leaving them enabled for now (4302 and 4334), fixing the bugs, and adjusting the code to suppress the other warnings. The changes in shell_web_contents_view_delegate_win.cc, ipc_perftest_support.cc, test_root_certs_win.cc, and TraceEvent.h appear to fix real bugs. The change in handle_inheritance_test.cc might fix a real bug. Typical warnings are: ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast': truncation from 'LPCSTR' to 'uint32_t' The main pattern is that VC++ 2015 is suspicious of code that converts a 64-bit pointer to a 32-bit integer. If the code converts from a pointer to a 64-bit integer, and then to a 32-bit integer, separating the type change from the truncation, then it assumes that the truncation is intentional. This seems like a reasonable heuristic. The warnings in the generated code were suppressed because they aren't bugs and because fixing gperf.exe (untouched since 2004) does not seem worthwhile. Note that Windows HANDLE types are a wrapper around a pointer, but due to WOW64 (32-bit process on 64-bit Windows) compatibility they are always 32-bit values that can safely be truncated, then sign-extended back. Unfortunately Microsoft does not supply a safe-truncation macro. https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2... Also a few uncommented warning-disables in common.gypi were commented. BUG=440500 ========== to ========== Fixing most VC++ 2015 64-bit build breaks VC++ 2015 64-bit builds trigger many new warnings about possibly unintended truncation bugs. Some of the bugs have a very low signal-to- noise ratio and this change suppresses them (4311 and 4312). Others seem to find some real bugs so I am leaving them enabled for now (4302 and 4334), fixing the bugs, and adjusting the code to suppress the other warnings. The changes in shell_web_contents_view_delegate_win.cc, ipc_perftest_support.cc, test_root_certs_win.cc, and TraceEvent.h appear to fix real bugs. Typical warnings are: ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast': truncation from 'LPCSTR' to 'uint32_t' The main pattern is that VC++ 2015 is suspicious of code that converts a 64-bit pointer to a 32-bit integer. If the code converts from a pointer to a 64-bit integer, and then to a 32-bit integer, separating the type change from the truncation, then it assumes that the truncation is intentional. This seems like a reasonable heuristic. The warnings in the generated code were suppressed because they aren't bugs and because fixing gperf.exe (untouched since 2004) does not seem worthwhile. Note that Windows HANDLE types are a wrapper around a pointer, but due to WOW64 (32-bit process on 64-bit Windows) compatibility they are always 32-bit values that can safely be truncated, then sign-extended back. Unfortunately Microsoft does not supply a safe-truncation macro. https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2... Also a few uncommented warning-disables in common.gypi were commented. BUG=440500 ==========
brucedawson@chromium.org changed reviewers: + wfh@chromium.org
PTAL when you get a chance. Together with a v8 fix and a webrtc fix (both pending) this gets 64-bit Chrome building with VC++ 2015 Update 1. I think the balance between suppressing warnings and adding casts is correct - the bugs found tend to support that. But I don't like adding casts. A HANDLE-to-int macro is tempting.
On 2015/11/07 00:57:35, brucedawson wrote: > PTAL when you get a chance. > > Together with a v8 fix and a webrtc fix (both pending) this gets 64-bit Chrome > building with VC++ 2015 Update 1. > > I think the balance between suppressing warnings and adding casts is correct - > the bugs found tend to support that. But I don't like adding casts. A > HANDLE-to-int macro is tempting. yes a handle to int macro sounds tempting too, having those double casts all over the place isn't ideal.
https://codereview.chromium.org/1422773008/diff/120001/components/startup_met... File components/startup_metric_utils/browser/startup_metric_utils.cc (right): https://codereview.chromium.org/1422773008/diff/120001/components/startup_met... components/startup_metric_utils/browser/startup_metric_utils.cc:139: if (reinterpret_cast<uintptr_t>(proc_info->UniqueProcessId) == proc_id) { UniqueProcessId is a HANDLE, shouldn't this be a double cast similar to others, or is there an implicit cast from uintptr_t -> DWORD happening? No warnings for that? https://codereview.chromium.org/1422773008/diff/120001/content/common/pepper_... File content/common/pepper_file_util.cc (right): https://codereview.chromium.org/1422773008/diff/120001/content/common/pepper_... content/common/pepper_file_util.cc:28: return static_cast<int>(reinterpret_cast<intptr_t>(socket_handle)); Can HANDLE values go above INT_MAX (signed int vs unsigned int)? Shouldn't this function return unsigned int? https://codereview.chromium.org/1422773008/diff/120001/content/shell/browser/... File content/shell/browser/shell_web_contents_view_delegate_win.cc (right): https://codereview.chromium.org/1422773008/diff/120001/content/shell/browser/... content/shell/browser/shell_web_contents_view_delegate_win.cc:85: AppendMenu(menu, MF_STRING | MF_POPUP, (UINT_PTR)sub_menu, L""); nit: do not use C-style cast here. https://codereview.chromium.org/1422773008/diff/120001/ipc/ipc_perftest_suppo... File ipc/ipc_perftest_support.cc (right): https://codereview.chromium.org/1422773008/diff/120001/ipc/ipc_perftest_suppo... ipc/ipc_perftest_support.cc:351: const DWORD_PTR thread_mask = static_cast<DWORD_PTR>(1) << cpu_number; will 1U work instead? seems bizarre to cast a literal. https://codereview.chromium.org/1422773008/diff/120001/net/cert/test_root_cer... File net/cert/test_root_certs_win.cc (right): https://codereview.chromium.org/1422773008/diff/120001/net/cert/test_root_cer... net/cert/test_root_certs_win.cc:103: uintptr_t store_as_uint = reinterpret_cast<uintptr_t>(store_provider); store_as_uintptr now :) https://codereview.chromium.org/1422773008/diff/120001/sandbox/win/src/interc... File sandbox/win/src/interception_unittest.cc (right): https://codereview.chromium.org/1422773008/diff/120001/sandbox/win/src/interc... sandbox/win/src/interception_unittest.cc:96: const size_t kAlignment = static_cast<size_t>(1) << kAlignmentBits; same as before, does 1U or 1UL work? https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_css_property_names.py (right): https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_names.py:93: #pragma warning(disable : 4302) where are these warnings generated from? do you have some sample compiler output? https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp (right): https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:30: // Cast through uintptr_t and then unsigned int to make the truncation looks like m_address should be a different type here, &object will not always be < 32-bits long. it should perhaps be a uintptr_t? https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/TraceEvent.h (right): https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/TraceEvent.h:273: : m_data(reinterpret_cast<uintptr_t>(id)) this looks like a behavior change, before the id was truncated to 32-bit even on 64-bit, now it's not. https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/wtf/BitVector.cpp (right): https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/wtf/BitVector.cpp:81: numBits = (numBits + bitsInPointer() - 1) & ~(bitsInPointer() - static_cast<size_t>(1)); same as others
Description was changed from ========== Fixing most VC++ 2015 64-bit build breaks VC++ 2015 64-bit builds trigger many new warnings about possibly unintended truncation bugs. Some of the bugs have a very low signal-to- noise ratio and this change suppresses them (4311 and 4312). Others seem to find some real bugs so I am leaving them enabled for now (4302 and 4334), fixing the bugs, and adjusting the code to suppress the other warnings. The changes in shell_web_contents_view_delegate_win.cc, ipc_perftest_support.cc, test_root_certs_win.cc, and TraceEvent.h appear to fix real bugs. Typical warnings are: ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast': truncation from 'LPCSTR' to 'uint32_t' The main pattern is that VC++ 2015 is suspicious of code that converts a 64-bit pointer to a 32-bit integer. If the code converts from a pointer to a 64-bit integer, and then to a 32-bit integer, separating the type change from the truncation, then it assumes that the truncation is intentional. This seems like a reasonable heuristic. The warnings in the generated code were suppressed because they aren't bugs and because fixing gperf.exe (untouched since 2004) does not seem worthwhile. Note that Windows HANDLE types are a wrapper around a pointer, but due to WOW64 (32-bit process on 64-bit Windows) compatibility they are always 32-bit values that can safely be truncated, then sign-extended back. Unfortunately Microsoft does not supply a safe-truncation macro. https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2... Also a few uncommented warning-disables in common.gypi were commented. BUG=440500 ========== to ========== Fixing most VC++ 2015 64-bit build breaks VC++ 2015 64-bit builds trigger many new warnings about possibly unintended truncation bugs. Some of the bugs have a very low signal-to- noise ratio and this change suppresses them (4311 and 4312). Others seem to find some real bugs so I am leaving them enabled for now (4302 and 4334), fixing the bugs, and adjusting the code to suppress the other warnings. The changes in shell_web_contents_view_delegate_win.cc, ipc_perftest_support.cc, test_root_certs_win.cc, and TraceEvent.h appear to fix real bugs. Typical warnings are: ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast': truncation from 'LPCSTR' to 'uint32_t' The main pattern is that VC++ 2015 is suspicious of code that converts a 64-bit pointer to a 32-bit integer. If the code converts from a pointer to a 64-bit integer, and then to a 32-bit integer, separating the type change from the truncation, then it assumes that the truncation is intentional. This seems like a reasonable heuristic. The warnings in the generated code were suppressed because they aren't bugs and because fixing gperf.exe (untouched since 2004) does not seem worthwhile. The warnings from gperf generated code look like: out\release_x64\gen\blink\core\csspropertynames.cpp(2914): warning C4302: 'type cast': truncation from 'char (*)[28]' to 'long' out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302: 'type cast': truncation from 'char (*)[4]' to 'long' Note that Windows HANDLE types are a wrapper around a pointer, but due to WOW64 (32-bit process on 64-bit Windows) compatibility they are always 32-bit values that can safely be truncated, then sign-extended back. Unfortunately Microsoft does not supply a safe-truncation macro. https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2... Also a few uncommented warning-disables in common.gypi were commented. BUG=440500 ==========
Thanks for the feedback. I'm thinking of adding an inline helper function to
win_util.h, looking like this:
inline uint32_t HandleToUint32(HANDLE h) {
// Cast through uintptr_t and then unsigned int to make the truncation to
// 32 bits explicit. Handles are size of-pointer but are always 32-bit values.
return static_cast<uint32_t>(reinterpret_cast<uintptr_t>(h));
}
Usage (before reformatting for length) looks like this:
std::string command_line = "HandleInheritanceTests_ValidInheritedHandle " +
base::UintToString(base::win::HandleToUint32(shared_handle));
The namespaces means it doesn't save much code space, but it avoids duplicating
the comments and the subtle truncation rules in ~8-10 places.
Thoughts?
https://codereview.chromium.org/1422773008/diff/120001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):
https://codereview.chromium.org/1422773008/diff/120001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:139: if
(reinterpret_cast<uintptr_t>(proc_info->UniqueProcessId) == proc_id) {
On 2015/11/10 18:05:14, Will Harris wrote:
> UniqueProcessId is a HANDLE, shouldn't this be a double cast similar to
others,
> or is there an implicit cast from uintptr_t -> DWORD happening? No warnings
for
> that?
There's no truncation because proc_id is extended to uintptr_t size to do the
comparison. But that means this code is wrong if the sign bit is set, so I
should add another cast. It probably only affects the INVALID_HANDLE value case,
but still.
Yet another reason to have a truncation function or macro.
https://codereview.chromium.org/1422773008/diff/120001/content/common/pepper_...
File content/common/pepper_file_util.cc (right):
https://codereview.chromium.org/1422773008/diff/120001/content/common/pepper_...
content/common/pepper_file_util.cc:28: return
static_cast<int>(reinterpret_cast<intptr_t>(socket_handle));
On 2015/11/10 18:05:14, Will Harris wrote:
> Can HANDLE values go above INT_MAX (signed int vs unsigned int)? Shouldn't
this
> function return unsigned int?
INVALID_HANDLE_VALUE (0xFFFFFFFF) is probably the only signed handle value,
although I don't know that for sure. MSDN says that sign extension should be
used when converting handles from 32-bit to 64-bit, but if comparing two 32-bit
handles it doesn't matter if they are signed or unsigned, so I'm inclined to
leave this alone. Although, having a truncation macro will force standardizing
which adds some temporary complication.
https://codereview.chromium.org/1422773008/diff/120001/content/shell/browser/...
File content/shell/browser/shell_web_contents_view_delegate_win.cc (right):
https://codereview.chromium.org/1422773008/diff/120001/content/shell/browser/...
content/shell/browser/shell_web_contents_view_delegate_win.cc:85:
AppendMenu(menu, MF_STRING | MF_POPUP, (UINT_PTR)sub_menu, L"");
On 2015/11/10 18:05:14, Will Harris wrote:
> nit: do not use C-style cast here.
I thought I could get grandfathered in :-)
https://codereview.chromium.org/1422773008/diff/120001/ipc/ipc_perftest_suppo...
File ipc/ipc_perftest_support.cc (right):
https://codereview.chromium.org/1422773008/diff/120001/ipc/ipc_perftest_suppo...
ipc/ipc_perftest_support.cc:351: const DWORD_PTR thread_mask =
static_cast<DWORD_PTR>(1) << cpu_number;
On 2015/11/10 18:05:14, Will Harris wrote:
> will 1U work instead? seems bizarre to cast a literal.
I asked this on c-users. I could use 1LLU but that is particularly bad in this
case because it is always a 64-bit value and we want sizeof(void*) value.
Their best suggestion was actually DWORD_PTR{1} (uniform initialization syntax)
but that is currently prohibited in Chrome, and not everyone agreed that that
was a great idea.
https://codereview.chromium.org/1422773008/diff/120001/net/cert/test_root_cer...
File net/cert/test_root_certs_win.cc (right):
https://codereview.chromium.org/1422773008/diff/120001/net/cert/test_root_cer...
net/cert/test_root_certs_win.cc:103: uintptr_t store_as_uint =
reinterpret_cast<uintptr_t>(store_provider);
On 2015/11/10 18:05:14, Will Harris wrote:
> store_as_uintptr now :)
D'oh!
https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/make_css_property_names.py (right):
https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_css_property_names.py:93: #pragma
warning(disable : 4302)
On 2015/11/10 18:05:14, Will Harris wrote:
> where are these warnings generated from? do you have some sample compiler
> output?
They show up in two generated files and I've added sample warnings from each
file to the CL description.
https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp (right):
https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:30: // Cast through
uintptr_t and then unsigned int to make the truncation
On 2015/11/10 18:05:14, Will Harris wrote:
> looks like m_address should be a different type here, &object will not always
be
> < 32-bits long. it should perhaps be a uintptr_t?
Yes, ideally, but the setNumber function that they use takes a double so it
can't handle a 64-bit pointer. Apparently it doesn't really matter and the code
may be deleted and... Sigh...
I've suggested this to the authors. I could change it or leave it - since it's
currently unused it doesn't matter much as long as it compiles.
Note that on Linux and OSX m_address is already always pointer sized, so
uintptr_t would be more consistent.
https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/TraceEvent.h (right):
https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/TraceEvent.h:273: :
m_data(reinterpret_cast<uintptr_t>(id))
On 2015/11/10 18:05:14, Will Harris wrote:
> this looks like a behavior change, before the id was truncated to 32-bit even
on
> 64-bit, now it's not.
Yep, it's a behavior change because the previous code was buggy. They copied
from base\trace_event.h which now does this correctly.
Description was changed from
==========
Fixing most VC++ 2015 64-bit build breaks
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them (4311 and 4312). Others seem
to find some real bugs so I am leaving them enabled for now (4302 and
4334), fixing the bugs, and adjusting the code to suppress the other
warnings.
The changes in shell_web_contents_view_delegate_win.cc,
ipc_perftest_support.cc, test_root_certs_win.cc, and TraceEvent.h appear
to fix real bugs.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic. The warnings in the
generated code were suppressed because they aren't bugs and because
fixing gperf.exe (untouched since 2004) does not seem worthwhile.
The warnings from gperf generated code look like:
out\release_x64\gen\blink\core\csspropertynames.cpp(2914): warning C4302:
'type cast': truncation from 'char (*)[28]' to 'long'
out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302:
'type cast': truncation from 'char (*)[4]' to 'long'
Note that Windows HANDLE types are a wrapper around a pointer, but due
to WOW64 (32-bit process on 64-bit Windows) compatibility they are
always 32-bit values that can safely be truncated, then sign-extended
back. Unfortunately Microsoft does not supply a safe-truncation macro.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2...
Also a few uncommented warning-disables in common.gypi were commented.
BUG=440500
==========
to
==========
Fixing remaining VC++ 2015 64-bit build breaks
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them (4311 and 4312). Others seem
to find some real bugs so I am leaving them enabled for now (4302 and
4334), fixing the bugs, and adjusting the code to suppress the other
warnings.
The changes in shell_web_contents_view_delegate_win.cc,
ipc_perftest_support.cc, test_root_certs_win.cc, and TraceEvent.h appear
to fix real bugs.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic. The warnings in the
generated code were suppressed because they aren't bugs and because
fixing gperf.exe (untouched since 2004) does not seem worthwhile.
The warnings from gperf generated code look like:
out\release_x64\gen\blink\core\csspropertynames.cpp(2914): warning C4302:
'type cast': truncation from 'char (*)[28]' to 'long'
out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302:
'type cast': truncation from 'char (*)[4]' to 'long'
Note that Windows HANDLE types are a wrapper around a pointer, but due
to WOW64 (32-bit process on 64-bit Windows) compatibility they are
always 32-bit values that can safely be truncated, then sign-extended
back. Unfortunately Microsoft does not supply a safe-truncation macro.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2...
Also a few uncommented warning-disables in common.gypi were commented.
BUG=440500
==========
Long term, we'd mostly like to enable even warnings with a low signal-to-noise ratio unless they result in egregious problems with code (for example, we disabled "warn on using 'this' in a constructor initializer list" because we had to annotate an incredibly large number of places in the codebase). How big a change would it be to enable 4311 and 4312?
Regarding behavior changes, I listed the changes that I think fix bugs, which is another way of saying "behavior changing". I guess TracedLayoutObject.* should be added to that list: The changes in shell_web_contents_view_delegate_win.cc, ipc_perftest_support.cc, test_root_certs_win.cc, and TraceEvent.h appear to fix real bugs. I added a HANDLE truncation function and used it in one place (handle_inheritance_test.cc). Let me know what you think. If it's good I'll use it everywhere. PTAL.
Description was changed from
==========
Fixing remaining VC++ 2015 64-bit build breaks
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them (4311 and 4312). Others seem
to find some real bugs so I am leaving them enabled for now (4302 and
4334), fixing the bugs, and adjusting the code to suppress the other
warnings.
The changes in shell_web_contents_view_delegate_win.cc,
ipc_perftest_support.cc, test_root_certs_win.cc, and TraceEvent.h appear
to fix real bugs.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic. The warnings in the
generated code were suppressed because they aren't bugs and because
fixing gperf.exe (untouched since 2004) does not seem worthwhile.
The warnings from gperf generated code look like:
out\release_x64\gen\blink\core\csspropertynames.cpp(2914): warning C4302:
'type cast': truncation from 'char (*)[28]' to 'long'
out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302:
'type cast': truncation from 'char (*)[4]' to 'long'
Note that Windows HANDLE types are a wrapper around a pointer, but due
to WOW64 (32-bit process on 64-bit Windows) compatibility they are
always 32-bit values that can safely be truncated, then sign-extended
back. Unfortunately Microsoft does not supply a safe-truncation macro.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2...
Also a few uncommented warning-disables in common.gypi were commented.
BUG=440500
==========
to
==========
Fixing remaining VC++ 2015 64-bit build breaks
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them (4311 and 4312). Others seem
to find some real bugs so I am leaving them enabled for now (4302 and
4334), fixing the bugs, and adjusting the code to suppress the other
warnings.
The changes in shell_web_contents_view_delegate_win.cc,
ipc_perftest_support.cc, test_root_certs_win.cc, TracedLayoutObject.*
and TraceEvent.h appear to fix real truncation bugs.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic. The warnings in the
generated code were suppressed because they aren't bugs and because
fixing gperf.exe (untouched since 2004) does not seem worthwhile.
The warnings from gperf generated code look like:
out\release_x64\gen\blink\core\csspropertynames.cpp(2914): warning C4302:
'type cast': truncation from 'char (*)[28]' to 'long'
out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302:
'type cast': truncation from 'char (*)[4]' to 'long'
Note that Windows HANDLE types are a wrapper around a pointer, but due
to WOW64 (32-bit process on 64-bit Windows) compatibility they are
always 32-bit values that can safely be truncated, then sign-extended
back. Unfortunately Microsoft does not supply a safe-truncation macro.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2...
Also a few uncommented warning-disables in common.gypi were commented.
BUG=440500
==========
https://codereview.chromium.org/1422773008/diff/120001/content/common/pepper_... File content/common/pepper_file_util.cc (right): https://codereview.chromium.org/1422773008/diff/120001/content/common/pepper_... content/common/pepper_file_util.cc:28: return static_cast<int>(reinterpret_cast<intptr_t>(socket_handle)); seems it's hard to change pepper api to have this return unsigned, sounds better to leave this alone. while looking at this code, I did find PlatformFileToInt in platform_file.h which I think does what your helper function will do. https://codereview.chromium.org/1422773008/diff/120001/ipc/ipc_perftest_suppo... File ipc/ipc_perftest_support.cc (right): https://codereview.chromium.org/1422773008/diff/120001/ipc/ipc_perftest_suppo... ipc/ipc_perftest_support.cc:351: const DWORD_PTR thread_mask = static_cast<DWORD_PTR>(1) << cpu_number; okay. makes sense. https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_css_property_names.py (right): https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_names.py:93: #pragma warning(disable : 4302) On 2015/11/10 18:43:26, brucedawson wrote: > On 2015/11/10 18:05:14, Will Harris wrote: > > where are these warnings generated from? do you have some sample compiler > > output? > > They show up in two generated files and I've added sample warnings from each > file to the CL description. Acknowledged. https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp (right): https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:30: // Cast through uintptr_t and then unsigned int to make the truncation this code is crazy. I agree with not complicating the craziness but there really should be a TODO or something to remove this code if it's not being used. https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/TraceEvent.h (right): https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/TraceEvent.h:273: : m_data(reinterpret_cast<uintptr_t>(id)) On 2015/11/10 18:43:26, brucedawson wrote: > On 2015/11/10 18:05:14, Will Harris wrote: > > this looks like a behavior change, before the id was truncated to 32-bit even > on > > 64-bit, now it's not. > > Yep, it's a behavior change because the previous code was buggy. They copied > from base\trace_event.h which now does this correctly. trace_event.h does data_(static_cast<unsigned long long>(reinterpret_cast<uintptr_t>(id))) can't you just match this by changing unsigned long to uintptr_t? It seems strange to be inconsistent here.
On 2015/11/10 19:40:01, Peter Kasting wrote: > Long term, we'd mostly like to enable even warnings with a low signal-to-noise > ratio unless they result in egregious problems with code (for example, we > disabled "warn on using 'this' in a constructor initializer list" because we had > to annotate an incredibly large number of places in the codebase). How big a > change would it be to enable 4311 and 4312? The C4311 and C4312 warnings were going to require a daunting number of changes, but I don't know exactly how many that would be. I'd be happy to try again after I land this change in order to check that disabling them is the right decision. My first build with them enabled hit 64 unique warnings before it failed - I would guess hundreds of code changes to suppress them. It might be possible to suppress them on projects with patterns that trigger them frequently. v8 triggers the warnings in what appears to be correct code. libxml triggers them in suspicious looking code that is misguided at best, but could take a while to land fixes for.
On 2015/11/10 19:47:30, brucedawson wrote: > On 2015/11/10 19:40:01, Peter Kasting wrote: > > Long term, we'd mostly like to enable even warnings with a low signal-to-noise > > ratio unless they result in egregious problems with code (for example, we > > disabled "warn on using 'this' in a constructor initializer list" because we > had > > to annotate an incredibly large number of places in the codebase). How big a > > change would it be to enable 4311 and 4312? > > The C4311 and C4312 warnings were going to require a daunting number of changes, > but > I don't know exactly how many that would be. I'd be happy to try again after I > land > this change in order to check that disabling them is the right decision. My > first > build with them enabled hit 64 unique warnings before it failed - I would guess > hundreds of code changes to suppress them. It might be possible to suppress them > on projects with patterns that trigger them frequently. It would definitely be better to only suppress these on some projects rather than globally. I have a background task I've been doing for a couple years now to enable warnings like C4244 that were originally triggering on thousands of cases, so dozens or hundreds doesn't seem as daunting :D
See specific comments. And just to clarify, does the truncation function I've added to win_util.h look like it will pass muster? https://codereview.chromium.org/1422773008/diff/120001/content/common/pepper_... File content/common/pepper_file_util.cc (right): https://codereview.chromium.org/1422773008/diff/120001/content/common/pepper_... content/common/pepper_file_util.cc:28: return static_cast<int>(reinterpret_cast<intptr_t>(socket_handle)); On 2015/11/10 19:45:45, Will Harris wrote: > seems it's hard to change pepper api to have this return unsigned, sounds better > to leave this alone. > > while looking at this code, I did find PlatformFileToInt in platform_file.h > which I think does what your helper function will do. Good find. I'll use this in this context, and my other function elsewhere. Using the base function elsewhere makes sense because the result sign is different and because using ppapi code elsewhere would be bad. https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp (right): https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:30: // Cast through uintptr_t and then unsigned int to make the truncation On 2015/11/10 19:45:46, Will Harris wrote: > this code is crazy. I agree with not complicating the craziness but there really > should be a TODO or something to remove this code if it's not being used. I changed it to uintptr_t so it's at least portably weird. I'll follow-up with the devs. https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/TraceEvent.h (right): https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/TraceEvent.h:273: : m_data(reinterpret_cast<uintptr_t>(id)) On 2015/11/10 19:45:46, Will Harris wrote: > On 2015/11/10 18:43:26, brucedawson wrote: > > On 2015/11/10 18:05:14, Will Harris wrote: > > > this looks like a behavior change, before the id was truncated to 32-bit > even > > on > > > 64-bit, now it's not. > > > > Yep, it's a behavior change because the previous code was buggy. They copied > > from base\trace_event.h which now does this correctly. > > trace_event.h does > > data_(static_cast<unsigned long long>(reinterpret_cast<uintptr_t>(id))) > > can't you just match this by changing unsigned long to uintptr_t? It seems > strange to be inconsistent here. I guess, although the whole idea of casting from uintptr_t to ulonglong is very odd - why are we casting to an equal or larger type?
https://codereview.chromium.org/1422773008/diff/140001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1422773008/diff/140001/build/common.gypi#newc... build/common.gypi:5708: 4311, # type cast: pointer truncation from pointer to smaller integer as per pkasting suggestion, please try and scope these down to just the areas required e.g. libxml and v8 rather than entirety of Chrome https://codereview.chromium.org/1422773008/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp (right): https://codereview.chromium.org/1422773008/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:30: : m_address(reinterpret_cast<uintptr_t>(&object)) is a cast even needed here now? https://codereview.chromium.org/1422773008/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:90: json->setNumber("address", m_address); how does this code compile when m_address is now a uintptr_t and setNumber takes a double?
https://codereview.chromium.org/1422773008/diff/140001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1422773008/diff/140001/build/common.gypi#newc... build/common.gypi:5708: 4311, # type cast: pointer truncation from pointer to smaller integer On 2015/11/10 23:01:30, Will Harris wrote: > as per pkasting suggestion, please try and scope these down to just the areas > required e.g. libxml and v8 rather than entirety of Chrome I'll try that, and see how messy it gets. https://codereview.chromium.org/1422773008/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp (right): https://codereview.chromium.org/1422773008/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:30: : m_address(reinterpret_cast<uintptr_t>(&object)) On 2015/11/10 23:01:31, Will Harris wrote: > is a cast even needed here now? The cast is still needed because we're changing from a pointer to an integer type. https://codereview.chromium.org/1422773008/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:90: json->setNumber("address", m_address); On 2015/11/10 23:01:30, Will Harris wrote: > how does this code compile when m_address is now a uintptr_t and setNumber takes > a double? Converting from uintptr_t to double is totally legal. Whether the compiler warns is up to it. On Linux and OSX the code is identical to what we had before (since unsigned long is the same size as uintptr_t) so it's just a matter of whether VC++ warns about the possible loss of precision. And it doesn't.
I think I've addressed all of the comments except for disabling 4311/4312 on a project-specific base (ongoing). Of course the changes may raise further questions. In particular: 1) I added a function to win_util.h and the comment does say that you probably shouldn't. 2) To use that function I have to include win_util.h from a half-dozen source files 3) win_util.h can only be included on Windows builds (why isn't it a NOP on other platforms?) so I had to add some #ifdefs, which I dislike. I think the try failures are because I need to sync to the latest code - therefore they can safely be ignored for now. Anyway, PTAL.
On 2015/11/11 18:56:13, brucedawson wrote: > I think I've addressed all of the comments except for disabling 4311/4312 on a > project-specific base (ongoing). > > Of course the changes may raise further questions. In particular: > 1) I added a function to win_util.h and the comment does say that you probably > shouldn't. > 2) To use that function I have to include win_util.h from a half-dozen source > files > 3) win_util.h can only be included on Windows builds (why isn't it a NOP on > other platforms?) so I had to add some #ifdefs, which I dislike. > > I think the try failures are because I need to sync to the latest code - > therefore they can safely be ignored for now. > > Anyway, PTAL. You'll want to start adding base/ owners to get their view on base/win/win_util.h In general, include files ending in _win.h or _linux.h should be wrapped in #ifdef OS_WIN around the #include in the compilation unit, which is what you're doing (correctly) here.
brucedawson@chromium.org changed reviewers: + danakj@chromium.org
danakj@, can you weigh in on whether the change to base/win/win_util.h that I'm proposing is a good idea? This change resolves some pointer-to-32-bit-int conversion warnings that are new to VC++ 2015. Some of them are bugs, and some are just warnings. In particular, we have several places where we convert a HANDLE (void*) to a 32-bit int. This is legal, but to avoid a warning you need a double cast to make your intentions explicit. The reason behind the double cast and the legality of the truncation is not obvious - hence the new function. The benefits can be seen in the two different possible changes to one file - one that does the double-cast and explanatory comment inline, and the other that uses the support function: https://codereview.chromium.org/1422773008/diff/140001/chrome/app/chrome_watc... https://codereview.chromium.org/1422773008/diff/180001/chrome/app/chrome_watc...
lgtm https://codereview.chromium.org/1422773008/diff/180001/content/common/pepper_... File content/common/pepper_file_util.cc (right): https://codereview.chromium.org/1422773008/diff/180001/content/common/pepper_... content/common/pepper_file_util.cc:27: return ppapi::PlatformFileToInt(socket_handle); looks like this could call down to ppapi::PlatformFileToInt for all platforms.
There are three conversion warnings that are showing up on VC++ 2015 64-bit
builds:
C4302: 'conversion' : truncation from 'type 1' to 'type 2'
C4311: 'variable' : pointer truncation from 'type' to 'type'
C4312: 'operation' : conversion from 'type1' to 'type2' of greater size
I initially fixed C4302 and had C4311 and C4312 disabled. Having done a (mostly)
full build of Chromium with warnAsErrors off I think that the correct thing to
do is to spot-suppress C4311 (and fix the three remaining warnings later) and
globally suppress C4312.
I don't think that C4312 indicates a pattern which we want to avoid. It triggers
on test code like this:
ContextID dummy_context_id = reinterpret_cast<ContextID>(0x87654321);
void* dev_channel_iface_addr = (void*)0xdeadbeef;
and on real code like this:
return reinterpret_cast<ImageHandle>(i);
static void constructDeletedValue(Node& slot, bool) { slot.m_next =
reinterpret_cast<Node*>(deletedValue); }
base::win::ScopedHandle pipe(reinterpret_cast<HANDLE>(pipe_handle));
Fixing these would require a lot of casts that I think would make the code
worse. Suppressing C4312 in individual projects requires about 20-25 .gypi
modifications and the same number of .gn modifications. I don't think the
benefits justify this.
Fixing the pipe_handle example above might make sense, by adding a
Uint32ToHandle function in win_util.h, which could be used to ensure that the
sign extension is done correctly.
TL;DR C4311 has value, C4312 has little to none.
On 2015/11/11 23:53:14, brucedawson wrote:
> I don't think that C4312 indicates a pattern which we want to avoid. It
triggers
> on test code like this:
>
> ContextID dummy_context_id = reinterpret_cast<ContextID>(0x87654321);
> void* dev_channel_iface_addr = (void*)0xdeadbeef;
>
> and on real code like this:
>
> return reinterpret_cast<ImageHandle>(i);
> static void constructDeletedValue(Node& slot, bool) { slot.m_next =
> reinterpret_cast<Node*>(deletedValue); }
> base::win::ScopedHandle pipe(reinterpret_cast<HANDLE>(pipe_handle));
>
> Fixing these would require a lot of casts that I think would make the code
> worse.
It seems like these are fixable without casts by doing something like this:
OLD CODE
ImageHandle PlatformImageData::HandleFromInt(int32_t i) {
return reinterpret_cast<ImageHandle>(i);
}
NEW CODE
ImageHandle PlatformImageData::HandleFromInt(int64_t i) {
return reinterpret_cast<ImageHandle>(i);
}
...unless this generates a narrowing conversion warning on a 32-bit build?
Otherwise, the upconversion happens at the spot of the function call and we
avoid casts entirely.
(Of course, according to code search, HandleFromInt() is never called at all so
we should fix by deleting it entirely, but whatever.)
The reason I kind of like fixing this way instead of just disabling the warning
is that I'm concerned about sign-vs.-zero-extension going wrong; someone does a
static_cast and sign-extends, while someone else compares against a
reinterpret_casted value, which gets implementation-defined behavior. I've
already found that many places in our codebase with this sort of code are quite
arbitrary in terms of whether people use the same type of cast everywhere, so
this seems like it might not be theoretical.
> It seems like these are fixable without casts by doing something like this: ... Your NEW CODE should use intptr_t instead of int64_t, and with that it works in that particular case, but not (for instance) in the cases where a numeric constant is used. There a converter function is needed, or double casts. Sign extension is the big worry. In many cases it doesn't matter, but I can't claim that it never matters. Having done a full build I can now say that there are a total of 1,387 of these warnings. After subtracting off those from two generated files (csspropertynames.cpp and cssvaluekeywords.cpp again) and libxml/libxslt there are 74 C4312 warnings and 2 C4311 warnings. So, not too many. I looked through most of them and there are three main categories, roughly equally distributed: 1) Assigning hard-coded integers to pointers, mostly for test purposes, which is benign. 2) Storing integer values in pointers and extracting them out to integers later, which is benign. 3) Converting from HANDLE to integer and back to HANDLE, which is risky, and should be fixed. So, they should all be fixed. However, I would like to globally suppress 4312 for this change simply for the pragmatic reason that suppressing it on a project-by-project basis is messy. I can do it (it's mostly done) but getting approval for another 20-25 changes to .gyp files is going to make this CL drag on. I'd like to get this landed to unblock VC++ 2015 64-bit builds, which currently don't work. I don't think we should require fixing (or individually suppressing) the remaining 64-bit bugs/warnings before we can move to the next step with VC++ 2015. Thoughts? I can do the per-project suppressions but I'm not seeing the value.
The latest patch does spot suppression of C4311 (it just took two lines of code and three .gyp changes) and bulk suppression of C4312 (spot suppression would require ~20-25 .gyp changes). I think this is the right balance. Eventually the necessary changes - many upstream - to fix all of these warnings should be landed (I'm convinced) tracked by bug 554200. https://codereview.chromium.org/1422773008/diff/180001/content/common/pepper_... File content/common/pepper_file_util.cc (right): https://codereview.chromium.org/1422773008/diff/180001/content/common/pepper_... content/common/pepper_file_util.cc:27: return ppapi::PlatformFileToInt(socket_handle); On 2015/11/11 23:49:35, Will Harris wrote: > looks like this could call down to ppapi::PlatformFileToInt for all platforms. Good point.
General principle sounds fine to me.
danakj@chromium.org changed reviewers: + grt@chromium.org
+grt for base/win/ ownership (also sorry for being slow here, was blinkon week) https://codereview.chromium.org/1422773008/diff/200001/base/win/win_util.h File base/win/win_util.h (right): https://codereview.chromium.org/1422773008/diff/200001/base/win/win_util.h#ne... base/win/win_util.h:8: // In general, you should not be adding stuff to this file. This reads a lot like the base/OWNERS comment. This new function seems to be needed in a lot of places (chrome, content, components, sandbox) and so this seems like a good place for it. I also think it's a very good idea to use helpers instead of casts for something like this. I think you should probably + a base/win/ owner specifically for this though, to be fair. https://codereview.chromium.org/1422773008/diff/200001/base/win/win_util.h#ne... base/win/win_util.h:61: // 32 bits explicit. Handles are size of-pointer but are always 32-bit values. Can you point to some documentation verifying this assumption?
> Can you point to some documentation verifying this assumption? Done - reference added to the comment. Note that there are currently three pre-submit warnings but I believe they can be ignored because the only changes are to our .gyp files: ** Presubmit Warnings ** When updating or adding third party code the appropriate 'README.chromium' file should also be updated with the correct version and package information. third_party\libxml\libxml.gyp \ third_party\libxslt\libxslt.gyp \ third_party\mesa\mesa.gyp
base/win/win_util.h lgtm
thakis@chromium.org changed reviewers: + thakis@chromium.org
Basically lgtm for these files: build\common.gyp chrome\app\chrome_watcher_client_unittest_win.cc chrome\app\chrome_watcher_command_line_win.cc chrome\app\close_handle_hook_win.cc chrome\browser\hang_monitor\hung_plugin_action.cc chrome\browser\hang_monitor\hung_window_detector.cc third_party\WebKit\Source\core\layout\TracedLayoutObject.cpp third_party\WebKit\Source\core\layout\TracedLayoutObject.h third_party\WebKit\Source\platform\TraceEvent.h third_party\WebKit\Source\wtf\BitVector.cpp third_party\libxml\libxml.gyp third_party\libxslt\libxslt.gyp third_party\mesa\mesa.gyp https://codereview.chromium.org/1422773008/diff/220001/chrome/app/close_handl... File chrome/app/close_handle_hook_win.cc (right): https://codereview.chromium.org/1422773008/diff/220001/chrome/app/close_handl... chrome/app/close_handle_hook_win.cc:145: #pragma warning(disable : 4311 4302) why the space in front of :? clang-format? https://codereview.chromium.org/1422773008/diff/220001/chrome/browser/hang_mo... File chrome/browser/hang_monitor/hung_plugin_action.cc (right): https://codereview.chromium.org/1422773008/diff/220001/chrome/browser/hang_mo... chrome/browser/hang_monitor/hung_plugin_action.cc:126: #pragma warning(disable : 4311 4302) should these warning pragmas have a comment that explains why they're safe to do? https://codereview.chromium.org/1422773008/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/build/scripts/make_css_property_names.py (right): https://codereview.chromium.org/1422773008/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/build/scripts/make_css_property_names.py:91: // Disable the warnings from casting a 64-bit pointer to 32-bit long Can you say why that's safe here too? Some of these make_ scripts have tests, but it looks like this one maybe doesn't. If the try bots are happy, I suppose it's alright. https://codereview.chromium.org/1422773008/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp (right): https://codereview.chromium.org/1422773008/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:30: : m_address(reinterpret_cast<uintptr_t>(&object)) Wow, how did this ever work? https://codereview.chromium.org/1422773008/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/TraceEvent.h (right): https://codereview.chromium.org/1422773008/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/TraceEvent.h:273: : m_data(static_cast<unsigned long long>(reinterpret_cast<uintptr_t>(id))) either cast to unsigned long long or change m_data to uintptr_t (which would make it 32-bit on 32-bit systems) https://codereview.chromium.org/1422773008/diff/220001/third_party/libxml/lib... File third_party/libxml/libxml.gyp (right): https://codereview.chromium.org/1422773008/diff/220001/third_party/libxml/lib... third_party/libxml/libxml.gyp:266: # 2015 64-bit warning for pointer truncation Is the TODO to fix this warning? Not clear what the TODO is asking for (also earlier)
I made some comment and formatting tweaks to address your comments. No code changes. PTAL Nico. https://codereview.chromium.org/1422773008/diff/220001/chrome/app/close_handl... File chrome/app/close_handle_hook_win.cc (right): https://codereview.chromium.org/1422773008/diff/220001/chrome/app/close_handl... chrome/app/close_handle_hook_win.cc:145: #pragma warning(disable : 4311 4302) On 2015/11/16 21:31:07, Nico wrote: > why the space in front of :? clang-format? Correct. I had to double-check, but I verified that the change was put in by git cl format. I very rarely reformat code manually anymore, and I certainly avoid doing it just 'cause I think the alternative is 'prettier'. But always worth checking. https://codereview.chromium.org/1422773008/diff/220001/chrome/browser/hang_mo... File chrome/browser/hang_monitor/hung_plugin_action.cc (right): https://codereview.chromium.org/1422773008/diff/220001/chrome/browser/hang_mo... chrome/browser/hang_monitor/hung_plugin_action.cc:126: #pragma warning(disable : 4311 4302) On 2015/11/16 21:31:07, Nico wrote: > should these warning pragmas have a comment that explains why they're safe to > do? Yes. I copied over the warning from the previous file, adding the word 'truncation' to make the type of warning more explicit. I also modified the layout of the #pragma warning on line 136 to make it consistent. https://codereview.chromium.org/1422773008/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp (right): https://codereview.chromium.org/1422773008/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/TracedLayoutObject.cpp:30: : m_address(reinterpret_cast<uintptr_t>(&object)) On 2015/11/16 21:31:07, Nico wrote: > Wow, how did this ever work? You'll be happier not knowing... It's oddly designed code, that is not currently used (I believe), that may go away, where perfect results are not required... I've shared my concerns with the owner but I don't want the compiler upgrade blocked on those issues. https://codereview.chromium.org/1422773008/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/TraceEvent.h (right): https://codereview.chromium.org/1422773008/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/TraceEvent.h:273: : m_data(static_cast<unsigned long long>(reinterpret_cast<uintptr_t>(id))) On 2015/11/16 21:31:07, Nico wrote: > either cast to unsigned long long or change m_data to uintptr_t (which would > make it 32-bit on 32-bit systems) The reason for leaving it as-is is that TraceID can be initialized with an unsigned long long (so m_id can't be uintptr_t) and the double cast matches the canonical version in base\trace_event.h (which this had diverged from). I'd initially cast to uintptr_t and let the conversion to unsigned long long be implicit, but wfh@ complained: https://codereview.chromium.org/1422773008/diff/120001/third_party/WebKit/Sou... At least all of the variants should be correct. https://codereview.chromium.org/1422773008/diff/220001/third_party/libxml/lib... File third_party/libxml/libxml.gyp (right): https://codereview.chromium.org/1422773008/diff/220001/third_party/libxml/lib... third_party/libxml/libxml.gyp:266: # 2015 64-bit warning for pointer truncation On 2015/11/16 21:31:07, Nico wrote: > Is the TODO to fix this warning? Not clear what the TODO is asking for (also > earlier) The TODO is to fix the code so that the warning suppression is not needed. libxml uses 'long' as if it was intptr_t. It uses it in such a way that it is often correct, but I'd rather have always correct with no warnings. The fixes will be easy but time consuming given the challenges of working on libxml. I have clarified the comments.
thanks, lgtm. i still don't get that one comment, but it's not all that important https://codereview.chromium.org/1422773008/diff/220001/chrome/browser/hang_mo... File chrome/browser/hang_monitor/hung_plugin_action.cc (right): https://codereview.chromium.org/1422773008/diff/220001/chrome/browser/hang_mo... chrome/browser/hang_monitor/hung_plugin_action.cc:126: #pragma warning(disable : 4311 4302) On 2015/11/17 00:24:39, brucedawson wrote: > On 2015/11/16 21:31:07, Nico wrote: > > should these warning pragmas have a comment that explains why they're safe to > > do? > > Yes. I copied over the warning from the previous file, adding the word > 'truncation' to make the type of warning more explicit. > > I also modified the layout of the #pragma warning on line 136 to make it > consistent. Hm, this now says that it's a truncation, but not why that's ok. Is this snippet only built in 32-bit builds?
> Hm, this now says that it's a truncation, but not why that's ok. Is this snippet > only built in 32-bit builds? I just realized that copying the comment over was not valid. The code has a similar pattern but is different. In this case the "truncation" is fine because the value being stored is fundamentally an integer, not a pointer. You know, I should probably fix the pattern to suppress them 'normally', using the dual casts instead of warning disables. Once I get my Visual Studio install working again...
I'm back, after reinstalling Windows and Visual Studio. I had been leaving alone the hang-detection code on the assumption that the existing #pragma warnings were well considered, but Nico@'s comments made me rethink that. It turns out that they are perfect candidates for using HandleToUint32, so now these source files have fewer #pragma warnings: chrome/browser/hang_monitor/hung_plugin_action.cc chrome/browser/hang_monitor/hung_window_detector.cc The SetProp calls will be fixed as part of bug 554200. https://codereview.chromium.org/1422773008/diff/220001/chrome/browser/hang_mo... File chrome/browser/hang_monitor/hung_plugin_action.cc (right): https://codereview.chromium.org/1422773008/diff/220001/chrome/browser/hang_mo... chrome/browser/hang_monitor/hung_plugin_action.cc:126: #pragma warning(disable : 4311 4302) Okay, better fix now. TetProp/GetProp take/return a HANDLE, so HandleToUint32 can be used, and some #pragmas can be removed. There is no *actual* truncation because an int is being stored and retrieved, so HandleToUint32 is safe. The warnings on the reverse process (int to HANDLE) can wait for bug 554200 (I just added a reminder).
brucedawson@chromium.org changed reviewers: + ananta@chromium.org
ananta@ - can you look at these files? They are the only unreviewed ones: components\startup_metric_utils\browser\startup_metric_utils.cc content\child\npapi\webplugin_delegate_impl_win.cc content\common\pepper_file_util.cc content\shell\browser\shell_web_contents_view_delegate_win.cc ipc\ipc_perftest_support.cc net\cert\test_root_certs_win.cc Although these files were changed since the last review: chrome/browser/hang_monitor/hung_plugin_action.cc chrome/browser/hang_monitor/hung_window_detector.cc Thanks.
my files still lgtm, but every time i look at them i find something new i don't understand, sorry https://codereview.chromium.org/1422773008/diff/280001/chrome/app/close_handl... File chrome/app/close_handle_hook_win.cc (right): https://codereview.chromium.org/1422773008/diff/280001/chrome/app/close_handl... chrome/app/close_handle_hook_win.cc:148: reinterpret_cast<DWORD>(module); I guess we don't run this code in 64-bit?
https://codereview.chromium.org/1422773008/diff/280001/chrome/app/close_handl... File chrome/app/close_handle_hook_win.cc (right): https://codereview.chromium.org/1422773008/diff/280001/chrome/app/close_handl... chrome/app/close_handle_hook_win.cc:148: reinterpret_cast<DWORD>(module); On 2015/11/18 02:02:35, Nico wrote: > I guess we don't run this code in 64-bit? Correct. UseHooks (line 234) unconditionally returns false in 64-bit builds. I think a case could be made for wrapping this code in the same #ifdef, but I didn't quite convince myself to do it. It's tempting because it avoids the #pragmas, but it requires more #ifdefs, but they're pretty safe #ifdefs, but...
https://codereview.chromium.org/1422773008/diff/280001/chrome/app/close_handl... File chrome/app/close_handle_hook_win.cc (right): https://codereview.chromium.org/1422773008/diff/280001/chrome/app/close_handl... chrome/app/close_handle_hook_win.cc:148: reinterpret_cast<DWORD>(module); On 2015/11/18 02:02:35, Nico wrote: > I guess we don't run this code in 64-bit? looks like these should look more like the EAT hooks in sandbox https://code.google.com/p/chromium/codesearch#chromium/src/sandbox/win/src/ea...
https://codereview.chromium.org/1422773008/diff/280001/chrome/app/close_handl... File chrome/app/close_handle_hook_win.cc (right): https://codereview.chromium.org/1422773008/diff/280001/chrome/app/close_handl... chrome/app/close_handle_hook_win.cc:148: reinterpret_cast<DWORD>(module); On 2015/11/18 02:06:04, brucedawson wrote: > On 2015/11/18 02:02:35, Nico wrote: > > I guess we don't run this code in 64-bit? > > Correct. UseHooks (line 234) unconditionally returns false in 64-bit builds. I > think a case could be made for wrapping this code in the same #ifdef, but I > didn't quite convince myself to do it. It's tempting because it avoids the > #pragmas, but it requires more #ifdefs, but they're pretty safe #ifdefs, but... Yeah, if this isn't called on 64-bit then it should only be compiled there. If this didn't go through the UseHooks() indirection, -Wunused-function would've even complained about this at compile time. Doesn't have to be in this CL though, or doesn't even have to be done by you. Thanks for explaining!
lgtm https://codereview.chromium.org/1422773008/diff/280001/chrome/browser/hang_mo... File chrome/browser/hang_monitor/hung_plugin_action.cc (right): https://codereview.chromium.org/1422773008/diff/280001/chrome/browser/hang_mo... chrome/browser/hang_monitor/hung_plugin_action.cc:18: The OS_WIN ifdef is not needed. This file is windows specific https://codereview.chromium.org/1422773008/diff/280001/chrome/browser/hang_mo... File chrome/browser/hang_monitor/hung_window_detector.cc (right): https://codereview.chromium.org/1422773008/diff/280001/chrome/browser/hang_mo... chrome/browser/hang_monitor/hung_window_detector.cc:15: #include "base/win/win_util.h" Remove the OS_WIN ifdef.
lgtm
brettw@chromium.org changed reviewers: + brettw@chromium.org
lgtm https://codereview.chromium.org/1422773008/diff/300001/chrome/app/chrome_watc... File chrome/app/chrome_watcher_client_unittest_win.cc (right): https://codereview.chromium.org/1422773008/diff/300001/chrome/app/chrome_watc... chrome/app/chrome_watcher_client_unittest_win.cc:26: #if defined(OS_WIN) This file is named "_win.cc". I don't think we need this conditional. https://codereview.chromium.org/1422773008/diff/300001/chrome/app/chrome_watc... File chrome/app/chrome_watcher_command_line_win.cc (right): https://codereview.chromium.org/1422773008/diff/300001/chrome/app/chrome_watc... chrome/app/chrome_watcher_command_line_win.cc:17: #if defined(OS_WIN) Ditto https://codereview.chromium.org/1422773008/diff/300001/content/common/sandbox... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/1422773008/diff/300001/content/common/sandbox... content/common/sandbox_win.cc:41: #if defined(OS_WIN) Ditto
Thanks ananta@/brettw@ for pointing out the excess #ifdefs - I'll happily remove those. And we're good to go, which means I am not aware of any blocking issues with VC++ 2015 Update 1.
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org, grt@chromium.org, thakis@chromium.org, ananta@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1422773008/#ps320001 (title: "Remove more unneeded #ifs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2015/11/19 05:35:41, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) The tests "fast/forms/text/text-font-height-mismatch.html" and "fast/text/emphasis.html" are failing half the time, which has been just enough to stop this change from making it through. I'm going to try again, while following up on why these tests are flaky. The failures are on 32-bit builds, and my changes have zero affect on such builds.
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2015/11/20 01:26:14, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) seems like a legitimate flake - the same issue is happening on other CLs (not yours) e.g. http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...
On 2015/11/20 02:30:44, Will Harris wrote: > On 2015/11/20 01:26:14, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) > > seems like a legitimate flake - the same issue is happening on other CLs (not > yours) > > e.g. > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_... Thanks Will. I filed 558574 and then found 521124. 521124 discusses different but similar tests failing on try bots but not on the waterfall. I will push for a resolution to those.
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2015/11/25 13:41:58, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) I disabled the two failing tests but now it looks like transforms/3d/point-mapping/3d-point-mapping-preserve-3d.html is flaky also - at least two spurious failures. I'm sheriff today so I have an extra justification for disabling it and tracking down someone to investigate.
This CL is rejected by the cq very often, much more often than all other CLs I've seen in the last few days. Maybe something is actually amiss? On Wed, Nov 25, 2015 at 11:44 AM, <brucedawson@chromium.org> wrote: > On 2015/11/25 13:41:58, commit-bot: I haz the power wrote: > >> Try jobs failed on following builders: >> win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, >> > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_... > ) > > I disabled the two failing tests but now it looks like > transforms/3d/point-mapping/3d-point-mapping-preserve-3d.html is flaky > also - at > least two spurious failures. I'm sheriff today so I have an extra > justification > for disabling it and tracking down someone to investigate. > > https://codereview.chromium.org/1422773008/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
This CL is rejected by the cq very often, much more often than all other CLs I've seen in the last few days. Maybe something is actually amiss? On Wed, Nov 25, 2015 at 11:44 AM, <brucedawson@chromium.org> wrote: > On 2015/11/25 13:41:58, commit-bot: I haz the power wrote: > >> Try jobs failed on following builders: >> win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, >> > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_... > ) > > I disabled the two failing tests but now it looks like > transforms/3d/point-mapping/3d-point-mapping-preserve-3d.html is flaky > also - at > least two spurious failures. I'm sheriff today so I have an extra > justification > for disabling it and tracking down someone to investigate. > > https://codereview.chromium.org/1422773008/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/11/25 17:47:01, Nico wrote: > This CL is rejected by the cq very often, much more often than all other > CLs I've seen in the last few days. Maybe something is actually amiss? I did consider that possibility but I don't think it is the case for a few reasons: 1) The tests that have caused most of the rejections (now disabled) were failing for other CLs as well. 2) The failures were on a 32-bit build and I've triple checked that the code should be completely unchanged in 32-bit builds (in 64-bit builds there are some bug fixes). My suspicion is that this CL hits the failures more often because by modifying common.gypi it forces a full rebuild and a run of all tests, whereas (I believe) smaller changes might trigger fewer tests. I'll try to validate this before I disable the new failing test. Any other thoughts are appreciated.
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2015/11/26 21:18:51, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_rel_ng failed twice. First time message_center_unittests (with patch) failed with no explanation. Second time it cruised past that test and then failed in Upload to test-results [gl_tests on ATI GPU on Windows (with patch) on Windows] with WARNING:root:Received HTTP status 409 loading "http://test-results.appspot.com/testfile/upload". I'm confused as to why I'm hitting so many random failures - there is no longer any pattern. I'm clicking commit again.
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2015/11/27 03:35:12, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_rel_ng failed twice, as usual. First time webkit_tests (with patch) failed with editing/spelling/spellcheck-editable-on-focus.html Second time sbox_integration_tests (with patch) failed with no explanation The gl_tests on ATI GPU on Windows stage, and various other stages, hit numerous 409 errors but eventually uploaded their results. Two runs, four different failures.
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2015/12/01 03:43:19, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) Two more random failures: fatal: unable to access 'https://chrome-internal.googlesource.com/chrome/tools/build_limited/scripts/slave.git/': Failed connect to chrome-internal.googlesource.com:443; No error error: Could not fetch origin and on webkit_tests (with patch): command timed out: 7200 seconds elapsed, attempting to kill program finished with exit code 1 elapsedTime=7200.418000
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org, thakis@chromium.org, brettw@chromium.org, ananta@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1422773008/#ps340001 (title: "Pull to latest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by brucedawson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wfh@chromium.org, thakis@chromium.org, brettw@chromium.org, ananta@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1422773008/#ps380001 (title: "Removing unneeded include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
No CL is this unlucky. I'd recommend splitting this in several CLs and landing each through the CQ. Then you'll see which part of this causes all these problems. On Wed, Dec 9, 2015 at 4:26 PM, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Try jobs failed on following builders: > cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu... > ) > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r... > ) > > https://codereview.chromium.org/1422773008/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
No CL is this unlucky. I'd recommend splitting this in several CLs and landing each through the CQ. Then you'll see which part of this causes all these problems. On Wed, Dec 9, 2015 at 4:26 PM, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Try jobs failed on following builders: > cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu... > ) > linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r... > ) > > https://codereview.chromium.org/1422773008/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The spurious failures lately have all been networking glitches - goma failed to initialize this time. The non-spurious failure is well understood: crbug.com/567377. The problem is that a full compile on win_chromium_rel_ng takes about 85 minutes. The WebKit tests take about 25 minutes, so it only needs ~ten minutes from the other steps and the 120 minute timeout for the entire process is hit and the build fails. I'm talking to the infra/goma teams about either raising the time limit or (better) speeding up the goma build. Current recommendation from others who have hit this issue is to skip the try bots when landing this CL as it is becoming a blocker. This is relatively safe because there have been no non-spurious test failures attributed to this change.
I've never seen this happen iirc, and I send CLs that rebuild all files (by tweaking warning flags or whatnot) relatively frequently... On Wed, Dec 9, 2015 at 4:53 PM, <brucedawson@chromium.org> wrote: > The spurious failures lately have all been networking glitches - goma > failed to > initialize this time. > > The non-spurious failure is well understood: crbug.com/567377. The > problem is > that a full compile on win_chromium_rel_ng takes about 85 minutes. The > WebKit > tests take about 25 minutes, so it only needs ~ten minutes from the other > steps > and the 120 minute timeout for the entire process is hit and the build > fails. > > I'm talking to the infra/goma teams about either raising the time limit or > (better) speeding up the goma build. Current recommendation from others > who have > hit this issue is to skip the try bots when landing this CL as it is > becoming a > blocker. This is relatively safe because there have been no non-spurious > test > failures attributed to this change. > > > https://codereview.chromium.org/1422773008/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I've never seen this happen iirc, and I send CLs that rebuild all files (by tweaking warning flags or whatnot) relatively frequently... On Wed, Dec 9, 2015 at 4:53 PM, <brucedawson@chromium.org> wrote: > The spurious failures lately have all been networking glitches - goma > failed to > initialize this time. > > The non-spurious failure is well understood: crbug.com/567377. The > problem is > that a full compile on win_chromium_rel_ng takes about 85 minutes. The > WebKit > tests take about 25 minutes, so it only needs ~ten minutes from the other > steps > and the 120 minute timeout for the entire process is hit and the build > fails. > > I'm talking to the infra/goma teams about either raising the time limit or > (better) speeding up the goma build. Current recommendation from others > who have > hit this issue is to skip the try bots when landing this CL as it is > becoming a > blocker. This is relatively safe because there have been no non-spurious > test > failures attributed to this change. > > > https://codereview.chromium.org/1422773008/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/12/09 22:01:50, Nico wrote: > I've never seen this happen iirc, and I send CLs that rebuild all files (by > tweaking warning flags or whatnot) relatively frequently... Can you share some example CLs? The more recent the better. It would be good to understand why this CL is hitting the timeout. I agree that it is weird.
On 2015/12/09 22:01:50, Nico wrote: > I've never seen this happen iirc, and I send CLs that rebuild all files (by > tweaking warning flags or whatnot) relatively frequently... We had this happen a few times when we were originally standing up the Win64 builds. We ended up raising the time limits at the time. > On Wed, Dec 9, 2015 at 4:53 PM, <mailto:brucedawson@chromium.org> wrote: > > > The spurious failures lately have all been networking glitches - goma > > failed to > > initialize this time. > > > > The non-spurious failure is well understood: crbug.com/567377. The > > problem is > > that a full compile on win_chromium_rel_ng takes about 85 minutes. The > > WebKit > > tests take about 25 minutes, so it only needs ~ten minutes from the other > > steps > > and the 120 minute timeout for the entire process is hit and the build > > fails. > > > > I'm talking to the infra/goma teams about either raising the time limit or > > (better) speeding up the goma build. Current recommendation from others > > who have > > hit this issue is to skip the try bots when landing this CL as it is > > becoming a > > blocker. This is relatively safe because there have been no non-spurious > > test > > failures attributed to this change. > > > > > > https://codereview.chromium.org/1422773008/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
message: On 2015/12/09 23:44:05, jschuh (very slow) wrote: > On 2015/12/09 22:01:50, Nico wrote: > > I've never seen this happen iirc, and I send CLs that rebuild all files (by > > tweaking warning flags or whatnot) relatively frequently... > > We had this happen a few times when we were originally standing up the Win64 > builds. We ended up raising the time limits at the time. This builder is just broken. It is doing too much and is failing frequently. Presumably the time to run the build and tests has been increasing and has now crossed a critical threshold. I just documented a few timeout failures on other CLs on the tracking bug (crbug.com/567377). Even doing a webkit only change is no guarantee of avoiding timeouts because those tests can take a *really* long time to run. And with the compile sometimes taking ~85 minutes to run it's pretty easy to hit disaster. I also created four test CLs to test this: https://codereview.chromium.org/1514703002 - common.gypi change only - 71:08 compile, but no webkit_tests https://codereview.chromium.org/1510383002 - common.gypi and base\win_util.h change only - 80:14 compile, but no webkit_tests https://codereview.chromium.org/1512133002 - same as above but with a webkit source change - 78:55 compile *and* webkit_tests https://codereview.chromium.org/1514723002 - webkit source change only - 12:28 compile, but webkit_tests The results are: Test 1: completed Test 2: hit a spurious failure shortly before finishing Test 3: timed out Test 4: still running, 90 minutes in, it might succeed, but similar changes have timed out I'm going to touch base with the sheriff and then force this through, tonight or tomorrow morning.
Description was changed from
==========
Fixing remaining VC++ 2015 64-bit build breaks
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them (4311 and 4312). Others seem
to find some real bugs so I am leaving them enabled for now (4302 and
4334), fixing the bugs, and adjusting the code to suppress the other
warnings.
The changes in shell_web_contents_view_delegate_win.cc,
ipc_perftest_support.cc, test_root_certs_win.cc, TracedLayoutObject.*
and TraceEvent.h appear to fix real truncation bugs.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic. The warnings in the
generated code were suppressed because they aren't bugs and because
fixing gperf.exe (untouched since 2004) does not seem worthwhile.
The warnings from gperf generated code look like:
out\release_x64\gen\blink\core\csspropertynames.cpp(2914): warning C4302:
'type cast': truncation from 'char (*)[28]' to 'long'
out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302:
'type cast': truncation from 'char (*)[4]' to 'long'
Note that Windows HANDLE types are a wrapper around a pointer, but due
to WOW64 (32-bit process on 64-bit Windows) compatibility they are
always 32-bit values that can safely be truncated, then sign-extended
back. Unfortunately Microsoft does not supply a safe-truncation macro.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2...
Also a few uncommented warning-disables in common.gypi were commented.
BUG=440500
==========
to
==========
Fixing remaining VC++ 2015 64-bit build breaks
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them (4311 and 4312). Others seem
to find some real bugs so I am leaving them enabled for now (4302 and
4334), fixing the bugs, and adjusting the code to suppress the other
warnings.
The changes in shell_web_contents_view_delegate_win.cc,
ipc_perftest_support.cc, test_root_certs_win.cc, TracedLayoutObject.*
and TraceEvent.h appear to fix real truncation bugs.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic. The warnings in the
generated code were suppressed because they aren't bugs and because
fixing gperf.exe (untouched since 2004) does not seem worthwhile.
The warnings from gperf generated code look like:
out\release_x64\gen\blink\core\csspropertynames.cpp(2914): warning C4302:
'type cast': truncation from 'char (*)[28]' to 'long'
out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302:
'type cast': truncation from 'char (*)[4]' to 'long'
Note that Windows HANDLE types are a wrapper around a pointer, but due
to WOW64 (32-bit process on 64-bit Windows) compatibility they are
always 32-bit values that can safely be truncated, then sign-extended
back. Unfortunately Microsoft does not supply a safe-truncation macro.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2...
Also a few uncommented warning-disables in common.gypi were commented.
Setting NOTRY=true because the change is hitting timeouts. See
crbug.com/567377 and comment #119 for details.
NOTRY=true
BUG=440500
==========
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1422773008/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1422773008/380001
Message was sent while issue was closed.
Description was changed from
==========
Fixing remaining VC++ 2015 64-bit build breaks
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them (4311 and 4312). Others seem
to find some real bugs so I am leaving them enabled for now (4302 and
4334), fixing the bugs, and adjusting the code to suppress the other
warnings.
The changes in shell_web_contents_view_delegate_win.cc,
ipc_perftest_support.cc, test_root_certs_win.cc, TracedLayoutObject.*
and TraceEvent.h appear to fix real truncation bugs.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic. The warnings in the
generated code were suppressed because they aren't bugs and because
fixing gperf.exe (untouched since 2004) does not seem worthwhile.
The warnings from gperf generated code look like:
out\release_x64\gen\blink\core\csspropertynames.cpp(2914): warning C4302:
'type cast': truncation from 'char (*)[28]' to 'long'
out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302:
'type cast': truncation from 'char (*)[4]' to 'long'
Note that Windows HANDLE types are a wrapper around a pointer, but due
to WOW64 (32-bit process on 64-bit Windows) compatibility they are
always 32-bit values that can safely be truncated, then sign-extended
back. Unfortunately Microsoft does not supply a safe-truncation macro.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2...
Also a few uncommented warning-disables in common.gypi were commented.
Setting NOTRY=true because the change is hitting timeouts. See
crbug.com/567377 and comment #119 for details.
NOTRY=true
BUG=440500
==========
to
==========
Fixing remaining VC++ 2015 64-bit build breaks
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them (4311 and 4312). Others seem
to find some real bugs so I am leaving them enabled for now (4302 and
4334), fixing the bugs, and adjusting the code to suppress the other
warnings.
The changes in shell_web_contents_view_delegate_win.cc,
ipc_perftest_support.cc, test_root_certs_win.cc, TracedLayoutObject.*
and TraceEvent.h appear to fix real truncation bugs.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic. The warnings in the
generated code were suppressed because they aren't bugs and because
fixing gperf.exe (untouched since 2004) does not seem worthwhile.
The warnings from gperf generated code look like:
out\release_x64\gen\blink\core\csspropertynames.cpp(2914): warning C4302:
'type cast': truncation from 'char (*)[28]' to 'long'
out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302:
'type cast': truncation from 'char (*)[4]' to 'long'
Note that Windows HANDLE types are a wrapper around a pointer, but due
to WOW64 (32-bit process on 64-bit Windows) compatibility they are
always 32-bit values that can safely be truncated, then sign-extended
back. Unfortunately Microsoft does not supply a safe-truncation macro.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2...
Also a few uncommented warning-disables in common.gypi were commented.
Setting NOTRY=true because the change is hitting timeouts. See
crbug.com/567377 and comment #119 for details.
NOTRY=true
BUG=440500
==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from
==========
Fixing remaining VC++ 2015 64-bit build breaks
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them (4311 and 4312). Others seem
to find some real bugs so I am leaving them enabled for now (4302 and
4334), fixing the bugs, and adjusting the code to suppress the other
warnings.
The changes in shell_web_contents_view_delegate_win.cc,
ipc_perftest_support.cc, test_root_certs_win.cc, TracedLayoutObject.*
and TraceEvent.h appear to fix real truncation bugs.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic. The warnings in the
generated code were suppressed because they aren't bugs and because
fixing gperf.exe (untouched since 2004) does not seem worthwhile.
The warnings from gperf generated code look like:
out\release_x64\gen\blink\core\csspropertynames.cpp(2914): warning C4302:
'type cast': truncation from 'char (*)[28]' to 'long'
out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302:
'type cast': truncation from 'char (*)[4]' to 'long'
Note that Windows HANDLE types are a wrapper around a pointer, but due
to WOW64 (32-bit process on 64-bit Windows) compatibility they are
always 32-bit values that can safely be truncated, then sign-extended
back. Unfortunately Microsoft does not supply a safe-truncation macro.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2...
Also a few uncommented warning-disables in common.gypi were commented.
Setting NOTRY=true because the change is hitting timeouts. See
crbug.com/567377 and comment #119 for details.
NOTRY=true
BUG=440500
==========
to
==========
Fixing remaining VC++ 2015 64-bit build breaks
VC++ 2015 64-bit builds trigger many new warnings about possibly
unintended truncation bugs. Some of the bugs have a very low signal-to-
noise ratio and this change suppresses them (4311 and 4312). Others seem
to find some real bugs so I am leaving them enabled for now (4302 and
4334), fixing the bugs, and adjusting the code to suppress the other
warnings.
The changes in shell_web_contents_view_delegate_win.cc,
ipc_perftest_support.cc, test_root_certs_win.cc, TracedLayoutObject.*
and TraceEvent.h appear to fix real truncation bugs.
Typical warnings are:
ipc\ipc_perftest_support.cc(351): warning C4334: '<<': result of 32-bit
shift implicitly converted to 64 bits (was 64-bit shift intended?)
net\cert\test_root_certs_win.cc(103): warning C4302: 'reinterpret_cast':
truncation from 'LPCSTR' to 'uint32_t'
The main pattern is that VC++ 2015 is suspicious of code that converts a
64-bit pointer to a 32-bit integer. If the code converts from a pointer
to a 64-bit integer, and then to a 32-bit integer, separating the type
change from the truncation, then it assumes that the truncation is
intentional. This seems like a reasonable heuristic. The warnings in the
generated code were suppressed because they aren't bugs and because
fixing gperf.exe (untouched since 2004) does not seem worthwhile.
The warnings from gperf generated code look like:
out\release_x64\gen\blink\core\csspropertynames.cpp(2914): warning C4302:
'type cast': truncation from 'char (*)[28]' to 'long'
out\release_x64\gen\blink\core\cssvaluekeywords.cpp(3409): warning C4302:
'type cast': truncation from 'char (*)[4]' to 'long'
Note that Windows HANDLE types are a wrapper around a pointer, but due
to WOW64 (32-bit process on 64-bit Windows) compatibility they are
always 32-bit values that can safely be truncated, then sign-extended
back. Unfortunately Microsoft does not supply a safe-truncation macro.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa384203%28v=vs.85%2...
Also a few uncommented warning-disables in common.gypi were commented.
Setting NOTRY=true because the change is hitting timeouts. See
crbug.com/567377 and comment #119 for details.
NOTRY=true
BUG=440500
Committed: https://crrev.com/d3beca7e8267b3db4e8245eb588147639a1b1bd2
Cr-Commit-Position: refs/heads/master@{#364264}
==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/d3beca7e8267b3db4e8245eb588147639a1b1bd2 Cr-Commit-Position: refs/heads/master@{#364264} |
