Enable PRINTF_FORMAT in clang builds on Windows |
|||||
Issue description
midi_manager_win.cc contains this code:
void set_index(size_t index) {
index_ = index;
// TODO(toyoshim): Use hashed ID.
info_.id = base::StringPrintf("%s-%d", type_.c_str(), index_);
}
This is incorrect because index_ is a size_t so there is a size mismatch on 64-bit builds. This would previously have been noticed by the VC++ /analyze build but this is temporarily out of commission due to the switch to clang-as-default and is long-term out of commission because clang is taking over.
One might expect clang-l to detect this problem but it does not because PRINTF_FORMAT is only usefully defined in GCC builds. Once PRINTF_FORMAT is usefully defined for __clang__ builds this error is caught.
Unfortunately, in addition to this medium-value warning (and other high-value mismatch warnings) PRINTF_FORMAT catches many low-value warnings, for DWORD/SystemErrorCode versus unsigned long mismatches. These warnings are low value because the size and sign match. Here is some sample output:
> ninja -C out\release_64 obj/media/midi/midi/midi_manager_win.obj
ninja: Entering directory `out\release_64'
[283 processes, 342/952 @ 72.6/s : 4.709s ] CXX obj/base/base/native_library_win.obj
C:\src\chromium3\src\base\native_library_win.cc(150,29): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat]
return StringPrintf("%u", code);
~~ ^~~~
%lu
1 warning generated.
[41 processes, 898/952 @ 77.3/s : 11.624s ] CXX obj/base/base/logging.obj
C:\src\chromium3\src\base\logging.cc(875,39): warning: format specifies type 'unsigned int' but the argument has type 'logging::SystemErrorCode' (aka 'unsigned long') [-Wformat]
base::StringPrintf(" (0x%X)", error_code);
~~ ^~~~~~~~~~
%lX
C:\src\chromium3\src\base\logging.cc(878,29): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat]
GetLastError(), error_code);
^~~~~~~~~~~~~~
C:\src\chromium3\src\base\logging.cc(878,45): warning: format specifies type 'unsigned int' but the argument has type 'logging::SystemErrorCode' (aka 'unsigned long') [-Wformat]
GetLastError(), error_code);
^~~~~~~~~~
3 warnings generated.
[1 processes, 952/952 @ 34.4/s : 27.685s ] CXX obj/media/midi/midi/midi_manager_win.obj
C:\src\chromium3\src\media\midi\midi_manager_win.cc(292,59): warning: format specifies type 'int' but the argument has type 'size_t' (aka 'unsigned long long') [-Wformat]
info_.id = base::StringPrintf("%s-%d", type_.c_str(), index_);
~~ ^~~~~~
%zu
1 warning generated.
We could fix all of the places where DWORD is printed with %X instead of %lX but this adds no value. It would potentially be much better to allow modifying the clang-cl PRINTF_FORMAT warning so that it ignores these distinctions without any meaning, where the sign and size match.
I will land a fix for the midi_manager_win.cc mismatch.
,
Aug 1 2017
> Printing an unsigned long with %u is a bad idea for portable code. True. In that case the warning is complaining that the code may be incorrect on some other/future platform. There is the risk (coming true in this case) that the warning will fire for a condition that will never happen. Given a sufficient diversity of platforms on the try bots one could argue that PRINTF_FORMAT doesn't need to warn on hypothetical mismatches, but only on actual mismatches. But, maybe the number of warnings is manageable. It looks like it may be in the dozens or low hundreds, so fixable.
,
Aug 1 2017
The problem with portability warnings is that they are basically saying "this code might be wrong on some other platform" and it is unfortunate that those "theoretical" warnings end up mixed up with real/current/actual/guaranteed warnings. For example, here are all of the warnings when building 64-bit chrome with clang with this warning enabled: C:\src\chromium3\src\base\native_library_win.cc(150,29): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\base\logging.cc(875,39): warning: format specifies type 'unsigned int' but the argument has type 'logging::SystemErrorCode' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\base\logging.cc(878,29): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\base\logging.cc(878,45): warning: format specifies type 'unsigned int' but the argument has type 'logging::SystemErrorCode' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\components\browser_watcher\stability_paths.cc(75,57): warning: format specifies type 'unsigned int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\win\ntstatus_logging.cc(38,31): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\win\ntstatus_logging.cc(66,47): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\client\crashpad_client_win.cc(280,55): warning: format specifies type 'int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\net\http_transport_win.cc(51,42): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\net\http_transport_win.cc(52,42): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\net\http_transport_win.cc(53,42): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\net\http_transport_win.cc(54,42): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\net\http_transport_win.cc(60,42): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\net\http_transport_win.cc(61,42): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\net\http_transport_win.cc(62,42): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\net\http_transport_win.cc(63,42): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\net\http_transport_win.cc(101,31): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\net\http_transport_win.cc(102,31): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\net\http_transport_win.cc(104,61): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\util\net\http_transport_win.cc(374,56): warning: format specifies type 'int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\third_party\crashpad\crashpad\snapshot\win\system_snapshot_win.cc(109,50): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] C:\src\chromium3\src\services\service_manager\runner\init.cc(60,54): warning: format specifies type 'int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\chrome\installer\util\self_reg_work_item.cc(80,53): warning: format specifies type 'unsigned int' but the argument has type 'HRESULT' (aka 'long') [-Wformat] C:\src\chromium3\src\components\policy\core\common\preg_parser.cc(307,39): warning: format specifies type 'char *' but the argument has type 'const wchar_t *' [-Wformat] C:\src\chromium3\src\content\browser\tracing\etw_tracing_agent_win.cc(35,7): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] C:\src\chromium3\src\content\browser\tracing\etw_tracing_agent_win.cc(174,51): warning: format specifies type 'unsigned int' but the argument has type 'LONG' (aka 'long') [-Wformat] C:\src\chromium3\src\content\browser\tracing\etw_tracing_agent_win.cc(175,39): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\content\browser\tracing\etw_tracing_agent_win.cc(176,59): warning: format specifies type 'unsigned int' but the argument has type 'LONG' (aka 'long') [-Wformat] C:\src\chromium3\src\content\browser\tracing\etw_tracing_agent_win.cc(177,47): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\content\browser\tracing\etw_tracing_agent_win.cc(190,44): warning: format specifies type 'unsigned int' but the argument has type 'LONG' (aka 'long') [-Wformat] C:\src\chromium3\src\content\browser\tracing\etw_tracing_agent_win.cc(190,60): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\device\bluetooth\bluetooth_service_record_win.cc(77,38): warning: format specifies type 'unsigned int' but the argument has type 'ULONG' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\device\bluetooth\bluetooth_service_record_win.cc(82,11): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] C:\src\chromium3\src\device\bluetooth\bluetooth_task_manager_win.cc(232,9): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] C:\src\chromium3\src\gpu\ipc\service\gpu_vsync_provider_win.cc(156,51): warning: format specifies type 'int' but the argument has type 'gpu::SurfaceHandle' (aka 'HWND__ *') [-Wformat] C:\src\chromium3\src\media\midi\midi_manager_win.cc(298,59): warning: format specifies type 'int' but the argument has type 'size_t' (aka 'unsigned long long') [-Wformat] C:\src\chromium3\src\content\browser\devtools\service_worker_devtools_agent_host.cc(72,31): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] C:\src\chromium3\src\chrome\browser\extensions\api\messaging\native_process_launcher.cc(195,48): warning: format specifies type 'int' but the argument has type 'intptr_t' (aka 'long long') [-Wformat] Not too many, and totally manageable to fix, but hidden within those are three warnings that are much more serious than the others: C:\src\chromium3\src\components\policy\core\common\preg_parser.cc(307,39): warning: format specifies type 'char *' but the argument has type 'const wchar_t *' [-Wformat] C:\src\chromium3\src\gpu\ipc\service\gpu_vsync_provider_win.cc(156,51): warning: format specifies type 'int' but the argument has type 'gpu::SurfaceHandle' (aka 'HWND__ *') [-Wformat] C:\src\chromium3\src\media\midi\midi_manager_win.cc(298,59): warning: format specifies type 'int' but the argument has type 'size_t' (aka 'unsigned long long') [-Wformat] Anyway, if we fix all of them then the hidden warnings will have nowhere to hide so it becomes a theoretical problem.
,
Aug 1 2017
I feel like I've done DWORD-related warning fixes in the past somewhere. And we've done some -Wformat improvements for Windows (e.g. https://bugs.llvm.org/show_bug.cgi?id=20808). Maybe for regular printf()s? But I can't find the changes now. But I believe we just fixed the warnings for DWORDs too.
,
Aug 1 2017
Aha, here: https://codereview.chromium.org/543923002 So we should just fix these I think.
,
Aug 1 2017
I'll start landing some fixes.
BTW, to enable the warnings you just need to make this change (adding __clang__) in compiler_specific.h:
#if defined(COMPILER_GCC) || defined(__clang__)
#define PRINTF_FORMAT(format_param, dots_param) \
__attribute__((format(printf, format_param, dots_param)))
#else
#define PRINTF_FORMAT(format_param, dots_param)
#endif
,
Aug 1 2017
CCing Xi to fix the crashpad warnings for third_party practice. To enable the warnings just locally make the change listed in comment #6. Putting treat_warnings_as_errors = false in args.gn lets you get past other warnings to the ones you want.
,
Aug 2 2017
I did a full build with these settings and PRINTF_FORMAT enabled for clang: is_component_build = false is_debug = false target_cpu = "x64" treat_warnings_as_errors = false There were only a few more warnings, most of which now have fixes pending or somebody assigned. The last two sections are available if anybody wants to grab them, or else I'll get to them later this week. Here's the report: Fix pending in crrev.com/c/596612: base\logging.cc(875,39): warning: format specifies type 'unsigned int' but the argument has type 'logging::SystemErrorCode' (aka 'unsigned long') [-Wformat] base\logging.cc(878,29): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] base\logging.cc(878,45): warning: format specifies type 'unsigned int' but the argument has type 'logging::SystemErrorCode' (aka 'unsigned long') [-Wformat] base\native_library_win.cc(150,29): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] Fix pending in crrev.com/c/596618: components\policy\core\common\preg_parser.cc(307,39): warning: format specifies type 'char *' but the argument has type 'const wchar_t *' [-Wformat] Fix pending in crrev.com/c/596613: content\browser\tracing\etw_tracing_agent_win.cc(174,51): warning: format specifies type 'unsigned int' but the argument has type 'LONG' (aka 'long') [-Wformat] content\browser\tracing\etw_tracing_agent_win.cc(175,39): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] content\browser\tracing\etw_tracing_agent_win.cc(176,59): warning: format specifies type 'unsigned int' but the argument has type 'LONG' (aka 'long') [-Wformat] content\browser\tracing\etw_tracing_agent_win.cc(177,47): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] content\browser\tracing\etw_tracing_agent_win.cc(190,44): warning: format specifies type 'unsigned int' but the argument has type 'LONG' (aka 'long') [-Wformat] content\browser\tracing\etw_tracing_agent_win.cc(190,60): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] content\browser\tracing\etw_tracing_agent_win.cc(35,7): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] Assigned to chengx: third_party\crashpad\crashpad\client\crashpad_client_win.cc(280,55): warning: format specifies type 'int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] third_party\crashpad\crashpad\snapshot\win\system_snapshot_win.cc(109,50): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] third_party\crashpad\crashpad\util\net\http_transport_win.cc(101,31): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] third_party\crashpad\crashpad\util\net\http_transport_win.cc(102,31): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] third_party\crashpad\crashpad\util\net\http_transport_win.cc(104,61): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] third_party\crashpad\crashpad\util\net\http_transport_win.cc(374,56): warning: format specifies type 'int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] third_party\crashpad\crashpad\util\net\http_transport_win.cc(51,42): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] third_party\crashpad\crashpad\util\net\http_transport_win.cc(52,42): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] third_party\crashpad\crashpad\util\net\http_transport_win.cc(53,42): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] third_party\crashpad\crashpad\util\net\http_transport_win.cc(54,42): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] third_party\crashpad\crashpad\util\net\http_transport_win.cc(60,42): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] third_party\crashpad\crashpad\util\net\http_transport_win.cc(61,42): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] third_party\crashpad\crashpad\util\net\http_transport_win.cc(62,42): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] third_party\crashpad\crashpad\util\net\http_transport_win.cc(63,42): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] third_party\crashpad\crashpad\util\win\ntstatus_logging.cc(38,31): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] third_party\crashpad\crashpad\util\win\ntstatus_logging.cc(66,47): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] Assigned to stanisc: gpu\ipc\service\gpu_vsync_provider_win.cc(156,51): warning: format specifies type 'int' but the argument has type 'gpu::SurfaceHandle' (aka 'HWND__ *') [-Wformat] Minor bugs: chrome\browser\printing\cloud_print\test\cloud_print_proxy_process_browsertest.cc(473,38): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] chrome\installer\util\self_reg_work_item.cc(80,53): warning: format specifies type 'unsigned int' but the argument has type 'HRESULT' (aka 'long') [-Wformat] components\browser_watcher\stability_paths.cc(75,57): warning: format specifies type 'unsigned int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] components\policy\core\common\policy_loader_win_unittest.cc(314,43): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] content\browser\accessibility\accessibility_event_recorder_win.cc(255,35): warning: format specifies type 'int' but the argument has type 'long' [-Wformat] content\browser\accessibility\accessibility_event_recorder_win.cc(256,35): warning: format specifies type 'int' but the argument has type 'long' [-Wformat] content\browser\accessibility\accessibility_event_recorder_win.cc(264,35): warning: format specifies type 'int' but the argument has type 'long' [-Wformat] content\browser\accessibility\accessibility_event_recorder_win.cc(265,35): warning: format specifies type 'int' but the argument has type 'long' [-Wformat] content\browser\devtools\service_worker_devtools_agent_host.cc(72,31): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] content\shell\browser\layout_test\blink_test_controller.cc(533,56): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] device\bluetooth\bluetooth_service_record_win.cc(77,38): warning: format specifies type 'unsigned int' but the argument has type 'ULONG' (aka 'unsigned long') [-Wformat] device\bluetooth\bluetooth_service_record_win.cc(82,11): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] device\bluetooth\bluetooth_task_manager_win.cc(232,9): warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat] device\test\usb_test_gadget_impl.cc(212,47): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] media\midi\midi_manager_win.cc(292,59): warning: format specifies type 'int' but the argument has type 'size_t' (aka 'unsigned long long') [-Wformat] remoting\host\win\unprivileged_process_delegate.cc(159,7): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] services\service_manager\runner\init.cc(60,54): warning: format specifies type 'int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat] Serious bugs: chrome\browser\extensions\api\messaging\native_process_launcher.cc(195,48): warning: format specifies type 'int' but the argument has type 'intptr_t' (aka 'long long') [-Wformat] chrome\test\chromedriver\element_commands.cc(339,32): warning: format specifies type 'char *' but the argument has type 'const wchar_t *' [-Wformat] storage\browser\fileapi\dump_file_system.cc(109,13): warning: format specifies type 'char *' but the argument has type 'const wchar_t *' [-Wformat] tools\gn\command_clean.cc(130,28): warning: format specifies type 'char *' but the argument has type 'const wchar_t *' [-Wformat]
,
Aug 2 2017
I will finish my part. Thanks!
,
Aug 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19175845b01c7e3d2160e7801ffa35812ffa9664 commit 19175845b01c7e3d2160e7801ffa35812ffa9664 Author: Bruce Dawson <brucedawson@chromium.org> Date: Wed Aug 02 17:00:45 2017 Fix %u,%X/DWORD printf mismatches in base In order to enable clang-cl's printf format string mismatch checking we need to fix a few dozen existing errors. These are mostly places where DWORD (unsigned long) is printed with %X or %u - an 'l' is needed. R=thakis@chromium.org BUG= 751171 Change-Id: Ieb1aeeedb14c5725b51d460fc34c418abcafe68f Reviewed-on: https://chromium-review.googlesource.com/596612 Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#491407} [modify] https://crrev.com/19175845b01c7e3d2160e7801ffa35812ffa9664/base/logging.cc [modify] https://crrev.com/19175845b01c7e3d2160e7801ffa35812ffa9664/base/native_library_win.cc
,
Aug 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c8cd473e465cf5bc02e91ada001b26e62bcf4724 commit c8cd473e465cf5bc02e91ada001b26e62bcf4724 Author: Bruce Dawson <brucedawson@chromium.org> Date: Thu Aug 03 21:59:28 2017 Fix %x/DWORD printf mismatches in bluetooth In order to enable clang-cl's printf format string mismatch checking we need to fix a few dozen existing errors. These are mostly places where DWORD (unsigned long) is printed with %X - an 'l' is needed. This change fixes three of these and these files now build cleanly with clang-cl with PRINTF_FORMAT checking enabled. R=scheib@chromium.org BUG= 751171 Change-Id: I6321593fb579f9c70f8adb5f885daffd0e5a6c74 Reviewed-on: https://chromium-review.googlesource.com/598827 Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Reviewed-by: Vincent Scheib <scheib@chromium.org> Cr-Commit-Position: refs/heads/master@{#491856} [modify] https://crrev.com/c8cd473e465cf5bc02e91ada001b26e62bcf4724/device/bluetooth/bluetooth_service_record_win.cc [modify] https://crrev.com/c8cd473e465cf5bc02e91ada001b26e62bcf4724/device/bluetooth/bluetooth_task_manager_win.cc
,
Aug 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/439caa972e11590a6a80374ba0076875060720ad commit 439caa972e11590a6a80374ba0076875060720ad Author: Bruce Dawson <brucedawson@chromium.org> Date: Fri Aug 04 00:56:43 2017 Fix %d/unsigned long printf mismatches found by clang In order to enable clang-cl's printf format string mismatch checking we need to fix a few dozen existing errors. These are mostly places where unsigned long is printed with %X or %u - an 'l' is needed. R=primiano@chromium.org BUG= 751171 Change-Id: I5276d31a80f1fc9391e81df656cfa1ce1debdaa5 Reviewed-on: https://chromium-review.googlesource.com/596613 Commit-Queue: Primiano Tucci <primiano@chromium.org> Reviewed-by: Primiano Tucci <primiano@chromium.org> Cr-Commit-Position: refs/heads/master@{#491897} [modify] https://crrev.com/439caa972e11590a6a80374ba0076875060720ad/content/browser/tracing/etw_tracing_agent_win.cc
,
Aug 4 2017
,
Aug 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d3e0a250a50feed42ba23ef581ab7e9f55fa831f commit d3e0a250a50feed42ba23ef581ab7e9f55fa831f Author: Bruce Dawson <brucedawson@chromium.org> Date: Fri Aug 04 01:15:04 2017 Fix char*/wchar_t* mismatch in StringPrintf call Building with clang-cl's PRINTF_FORMAT checking experimentally enabled found a few dozen warnings and a few real bugs, including this platform-specific format-string mismatch. Printing a wchar_t* string with %s will always give incorrect results so it's not clear why this was never noticed - I guess the debug_name is normally ignored. TBR=pastarmovj@chromium.org BUG= 751171 Change-Id: Ia2f7ca15a89c6d6a4606139b7d874c0aa1be8fc4 Reviewed-on: https://chromium-review.googlesource.com/596618 Reviewed-by: Bruce Dawson <brucedawson@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#491902} [modify] https://crrev.com/d3e0a250a50feed42ba23ef581ab7e9f55fa831f/components/policy/core/common/preg_parser.cc
,
Aug 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6c9a5dd740ec12e401045fdd22ae25b5435b3173 commit 6c9a5dd740ec12e401045fdd22ae25b5435b3173 Author: Bruce Dawson <brucedawson@chromium.org> Date: Fri Aug 04 17:57:45 2017 Fix five printf format-string mismatches This CL fixes the five most serious printf format-string mismatches found by clang-cl. These bugs slipped through because VC++ does not have native format-string checking for user-defined functions, because clang-cl had its checking disabled, because these files are not compiled on other platforms, and because VC++'s /analyze either didn't compile these files or I didn't notice the warnings appearing. Once all of these bugs/warnings are fixed we can enable PRINTF_FORMAT for clang-cl and these bugs will never return. One bug was from printing size_t sized integers with %d - the fix is to change to %zd. Another was from printing intptr_t with %d - the fix is to use "%" PRIdPTR. Three of the bugs were from printing file paths with %s which is incorrect on Windows - use "%" PRIsFP. TBR=toyoshim@chromium.org BUG= 751171 Change-Id: I2d337c77c56b5cffd6f25d9e59d49c91d73d2869 Reviewed-on: https://chromium-review.googlesource.com/599087 Reviewed-by: Bruce Dawson <brucedawson@chromium.org> Reviewed-by: Shuotao Gao <stgao@chromium.org> Reviewed-by: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Sergey Ulanov <sergeyu@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#492064} [modify] https://crrev.com/6c9a5dd740ec12e401045fdd22ae25b5435b3173/chrome/browser/extensions/api/messaging/native_process_launcher.cc [modify] https://crrev.com/6c9a5dd740ec12e401045fdd22ae25b5435b3173/chrome/test/chromedriver/element_commands.cc [modify] https://crrev.com/6c9a5dd740ec12e401045fdd22ae25b5435b3173/media/midi/midi_manager_win.cc [modify] https://crrev.com/6c9a5dd740ec12e401045fdd22ae25b5435b3173/storage/browser/fileapi/dump_file_system.cc
,
Aug 5 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ab51142795125c33d81880f6350b04ba352a1d58 commit ab51142795125c33d81880f6350b04ba352a1d58 Author: Bruce Dawson <brucedawson@chromium.org> Date: Sat Aug 05 19:33:01 2017 Fix %d/unsigned long printf mismatch found by clang In order to enable clang-cl's printf format string mismatch checking we need to fix existing warnings, even benign ones like this Windows- specific %d/unsigned long mismatch (benign because unsigned long is always int-sized on Windows). R=sky@chromium.org BUG= 751171 Change-Id: I8920d2e3e15a2e407dba10ed9f034b54cafd274c Reviewed-on: https://chromium-review.googlesource.com/602172 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#492245} [modify] https://crrev.com/ab51142795125c33d81880f6350b04ba352a1d58/services/service_manager/runner/init.cc
,
Aug 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bf1fe350e796481f5731800cbe5e1af638d0158d commit bf1fe350e796481f5731800cbe5e1af638d0158d Author: Bruce Dawson <brucedawson@chromium.org> Date: Mon Aug 07 15:01:25 2017 Fix %X/unsigned long printf mismatch found by clang In order to enable clang-cl's printf format string mismatch checking we need to fix existing warnings, even benign ones like this Windows- specific %X/unsigned long mismatch (benign because unsigned long is always int-sized on Windows). R=gab@chromium.org BUG= 751171 Change-Id: I185bb1f6c7fbaa5dbdbe3a4dd31036bb496fe68a Reviewed-on: https://chromium-review.googlesource.com/602708 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#492320} [modify] https://crrev.com/bf1fe350e796481f5731800cbe5e1af638d0158d/chrome/installer/util/self_reg_work_item.cc
,
Aug 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fb6b7d43431f6a142d53150d14f441cbf1f3e461 commit fb6b7d43431f6a142d53150d14f441cbf1f3e461 Author: Bruce Dawson <brucedawson@chromium.org> Date: Mon Aug 07 16:01:50 2017 Fix %u/unsigned long printf mismatch found by clang In order to enable clang-cl's printf format string mismatch checking we need to fix existing warnings, even benign ones like this Windows- specific %u/unsigned long mismatch (benign because unsigned long is always int-sized on Windows). R=siggi@chromium.org BUG= 751171 Change-Id: I85f0beacbd10526085bc152649e3093652141b8b Reviewed-on: https://chromium-review.googlesource.com/603028 Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org> Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org> Cr-Commit-Position: refs/heads/master@{#492332} [modify] https://crrev.com/fb6b7d43431f6a142d53150d14f441cbf1f3e461/components/browser_watcher/stability_paths.cc
,
Aug 7 2017
Of the remaining non-crashpad format-string bugs that remain most of them are for base::ProcessId. Most of these are in cross-platform code where %d has worked for years - apparently it is compatible with mx_koid_t and or pid_t. It gives a warning on Windows because ProcessID is of type DWORD which is of type unsigned long. It gives this warning even though unsigned long is 32-bit on Windows, always. So, two fixes come to mind. Either I can create another PRIsFP style #define to abstract away how to format a ProcessID type, or I can cast ProcessID to int or unsigned and print that. On the one hand, casts are evil and can hide bugs. On the other hand, #defines are evil and can hide bugs. Either way the process IDs are 32 bits on all platforms and will end up printed that way.I'd like to get input/agreement before landing changes because the fixes will require approval from ~six different developers and I don't want a diversity of opinions to lead to a diversity of fixes. I am inclined to regard #defines as more evil in this case so I would prefer adding casts. Thoughts? Here are the remaining ProcessID warnings: chrome\browser\printing\cloud_print\test\cloud_print_proxy_process_browsertest.cc(473,38): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] components\policy\core\common\policy_loader_win_unittest.cc(314,43): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] content\browser\devtools\service_worker_devtools_agent_host.cc(72,31): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] content\shell\browser\layout_test\blink_test_controller.cc(534,56): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] device\test\usb_test_gadget_impl.cc(212,47): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] remoting\host\win\unprivileged_process_delegate.cc(159,7): warning: format specifies type 'int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] These are the non-ProcessId warnings: content\browser\accessibility\accessibility_event_recorder_win.cc(255,35): warning: format specifies type 'int' but the argument has type 'long' [-Wformat] content\browser\accessibility\accessibility_event_recorder_win.cc(256,35): warning: format specifies type 'int' but the argument has type 'long' [-Wformat] content\browser\accessibility\accessibility_event_recorder_win.cc(264,35): warning: format specifies type 'int' but the argument has type 'long' [-Wformat] content\browser\accessibility\accessibility_event_recorder_win.cc(265,35): warning: format specifies type 'int' but the argument has type 'long' [-Wformat]
,
Aug 7 2017
Do you know how often we printf() ProcessIDs? Having a printing macro for ProcessIDs seems a bit cleaner and more future-proof to me, but if it means we'll have to update 30 files instead of 6, then a cast is probably the way to go.
,
Aug 7 2017
There are seven places where we printf a ProcessId. The six listed above and one that I fixed by modifying %d to %ld before I realized that that fix was Windows specific. Here is the seventh case: components\browser_watcher\stability_paths.cc(75,57): warning: format specifies type 'unsigned int' but the argument has type 'base::ProcessId' (aka 'unsigned long') [-Wformat] If there are any other places where we printf a ProcessId then they must be not compiled for Windows. Finding them would require changing the ProcessID typedef and then seeing what new warnings appear. > Having a printing macro for ProcessIDs seems a bit cleaner and more future-proof to me Yeah, except that as a macro we can't namespace it so it is simultaneously *less* future proof.
,
Aug 7 2017
CrPRIpidS (or something else prefixed by "Cr") should be fairly collision-resistant :-)
,
Aug 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/62f417485ce7bbc08f0ae44ce0eee80637fa28b2 commit 62f417485ce7bbc08f0ae44ce0eee80637fa28b2 Author: Xi Cheng <chengx@chromium.org> Date: Mon Aug 07 20:36:19 2017 Update Crashpad to 01110c0a3b3536f344224728ac524b30599fecc5 edf4dde8ae10 linux: Add ExceptionSnapshotLinux 01110c0a3b35 win: Fix %u, %d, %x/DWORD printf mismatches Bug: 751171 Change-Id: I932d9b20b49b56a4aa672eda6cce23f74dad4694 Reviewed-on: https://chromium-review.googlesource.com/603748 Commit-Queue: Xi Cheng <chengx@chromium.org> Reviewed-by: Mark Mentovai <mark@chromium.org> Cr-Commit-Position: refs/heads/master@{#492397} [modify] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/README.chromium [modify] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/client/crashpad_client_win.cc [modify] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/compat/android/sys/syscall.h [modify] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/compat/compat.gyp [add] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/compat/linux/signal.h [modify] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/snapshot/linux/cpu_context_linux.cc [modify] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/snapshot/linux/cpu_context_linux.h [add] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/snapshot/linux/exception_snapshot_linux.cc [add] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/snapshot/linux/exception_snapshot_linux.h [add] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/snapshot/linux/exception_snapshot_linux_test.cc [add] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/snapshot/linux/signal_context.h [modify] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/snapshot/snapshot.gyp [modify] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/snapshot/snapshot_test.gyp [modify] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/snapshot/win/system_snapshot_win.cc [add] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/util/linux/traits.h [modify] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/util/net/http_transport_win.cc [modify] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/util/util.gyp [modify] https://crrev.com/62f417485ce7bbc08f0ae44ce0eee80637fa28b2/third_party/crashpad/crashpad/util/win/ntstatus_logging.cc
,
Aug 7 2017
The CL in comment 23 contains the printf mismatch fix for third_party/crashpad. Its upstream fix is https://chromium-review.googlesource.com/c/598651
,
Aug 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3859fd8f1db944d4e83270a5c2784422a0e1c71d commit 3859fd8f1db944d4e83270a5c2784422a0e1c71d Author: Bruce Dawson <brucedawson@chromium.org> Date: Tue Aug 08 15:57:17 2017 Use CrPRIdPid to print process IDs Process IDs are int on most platforms but long on Windows. So, we need a macro (sigh...) to abstract away the correct format specifiers. This CL creates and uses CrPRIdPid. The mismatches were benign because sizeof(long) == sizeof(int) on Windows, but fixing these warnings will allow us to enable PRINTF_FORMAT checking on Windows which will prevent more serious errors. BUG= 751171 TBR=jamiewalch Change-Id: Ic56ec048a4fb38d90956bb686e1e1fbf86433237 Reviewed-on: https://chromium-review.googlesource.com/604629 Commit-Queue: Nico Weber <thakis@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org> Reviewed-by: Peter Beverloo <peter@chromium.org> Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org> Reviewed-by: Reilly Grant <reillyg@chromium.org> Cr-Commit-Position: refs/heads/master@{#492641} [modify] https://crrev.com/3859fd8f1db944d4e83270a5c2784422a0e1c71d/base/process/process_handle.h [modify] https://crrev.com/3859fd8f1db944d4e83270a5c2784422a0e1c71d/chrome/browser/printing/cloud_print/test/cloud_print_proxy_process_browsertest.cc [modify] https://crrev.com/3859fd8f1db944d4e83270a5c2784422a0e1c71d/components/browser_watcher/stability_paths.cc [modify] https://crrev.com/3859fd8f1db944d4e83270a5c2784422a0e1c71d/components/policy/core/common/policy_loader_win_unittest.cc [modify] https://crrev.com/3859fd8f1db944d4e83270a5c2784422a0e1c71d/content/browser/devtools/service_worker_devtools_agent_host.cc [modify] https://crrev.com/3859fd8f1db944d4e83270a5c2784422a0e1c71d/content/shell/browser/layout_test/blink_test_controller.cc [modify] https://crrev.com/3859fd8f1db944d4e83270a5c2784422a0e1c71d/device/test/usb_test_gadget_impl.cc [modify] https://crrev.com/3859fd8f1db944d4e83270a5c2784422a0e1c71d/remoting/host/win/unprivileged_process_delegate.cc
,
Aug 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b38e2d000fc51de3f573172c4912437acc628d4 commit 3b38e2d000fc51de3f573172c4912437acc628d4 Author: Bruce Dawson <brucedawson@chromium.org> Date: Tue Aug 08 17:37:45 2017 Fix format strings when printing long The start/end values in IA2TextSegment are of type long but were printed with %d, when %ld is needed. This is a benign error on Windows because the types are the same size but the warning must be fixed to let us enable PRINTF_FORMAT checking on Windows with clang-cl, which protects us from more serious errors. The warnings were: warning: format specifies type 'int' but the argument has type 'long' [-Wformat] And they happened four times: content\browser\accessibility\accessibility_event_recorder_win.cc(255,35) content\browser\accessibility\accessibility_event_recorder_win.cc(256,35) content\browser\accessibility\accessibility_event_recorder_win.cc(264,35) content\browser\accessibility\accessibility_event_recorder_win.cc(265,35) R=dtseng@chromium.org BUG= 751171 Change-Id: I7130f595f53dffbb426aabc1688b846bd89f7181 Reviewed-on: https://chromium-review.googlesource.com/604009 Reviewed-by: David Tseng <dtseng@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#492688} [modify] https://crrev.com/3b38e2d000fc51de3f573172c4912437acc628d4/content/browser/accessibility/accessibility_event_recorder_win.cc
,
Aug 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/73528d15795917856b9946165402c6a00339f08c commit 73528d15795917856b9946165402c6a00339f08c Author: Bruce Dawson <brucedawson@chromium.org> Date: Wed Aug 09 01:04:29 2017 Enable PRINTF_FORMAT for clang Because VC++ does not have format-string checking for user-defined functions during normal compiles, because clang-cl had its format-string checking disabled, because some files are only compiled on Windows, and because VC++'s /analyze doesn't build all targets, and because VC++'s format-string checking is more lenient than clang's... 50 warnings about format-string mismatches crept in to Chromium's build. Seven of these were somewhat serious, with four being wchar_t*/char* mismatches because of base::FilePath and the other three being size_t/%d mismatches. Now that all of the mismatches are corrected this change enables PRINTF_FORMAT checking with clang-cl so that these bugs never return. Bug: 751171 Change-Id: I50ab91cf1c9ba6d122d530fb6cf58f5d84d541a3 Reviewed-on: https://chromium-review.googlesource.com/599127 Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: Bruce Dawson <brucedawson@chromium.org> Cr-Commit-Position: refs/heads/master@{#492796} [modify] https://crrev.com/73528d15795917856b9946165402c6a00339f08c/base/compiler_specific.h
,
Aug 9 2017
Barring anything unexpected this is now resolved. 50 warnings/bugs resolved and the warning enabled so that they won't come back. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by h...@chromium.org
, Aug 1 2017