Follow-up for the root cause of bug 777579 and bug 777741
We should: - get the crashpad tests covering this running on cq / main waterfall bots - figure out the clang-cl bug and make a reduced repro (and fix)
The following revision refers to this bug: https://chromium.googlesource.com/crashpad/crashpad.git/+/6d5bd1d04d5f50d9f4a58d528ccfb38c46423f23 commit 6d5bd1d04d5f50d9f4a58d528ccfb38c46423f23 Author: Mark Mentovai <mark@chromium.org> Date: Tue Oct 24 19:07:38 2017 win: Go back to using ml.exe for SafeTerminateProcess() This reverts 55133d332b6c and adds a broken dummy SafeTerminateProcess() for cross builds instead. It’s similar to 2f4516f93838, which was for CaptureContext(). This upstreams https://chromium.googlesource.com/chromium/src/+/af5f31ed615bdbf0ad246cff643a2802a1e0d702 (slightly modified). The dummy implementation in the “broken” file affords no protection against third-party code patching TerminateProcess() badly. The “broken” file is not used by Crashpad anywhere at all, and is only used by Crashpad in Chromium during a cross build targeting Windows without the benefit of Microsoft’s ml.exe assembler. Strictly speaking, this file does not need to be checked in to the Crashpad repository, but since Chromium needs it to unblock its not-production-ready cross build for Windows, it’s being landed here to avoid Chromium’s copy of Crashpad appearing as modified or “dirty” relative to this upstream copy. Bug: chromium:762167, chromium:777924 Change-Id: Iba68c0cab142fbe9541ea254a9a856b8263e4c70 Reviewed-on: https://chromium-review.googlesource.com/735078 Reviewed-by: Mark Mentovai <mark@chromium.org> [modify] https://crrev.com/6d5bd1d04d5f50d9f4a58d528ccfb38c46423f23/util/util.gyp [add] https://crrev.com/6d5bd1d04d5f50d9f4a58d528ccfb38c46423f23/util/win/safe_terminate_process.asm [delete] https://crrev.com/025455e77af4eb0cffad42dda61aff0151ba1bfa/util/win/safe_terminate_process.cc [add] https://crrev.com/6d5bd1d04d5f50d9f4a58d528ccfb38c46423f23/util/win/safe_terminate_process_broken.cc
The following revision refers to this bug: https://chromium.googlesource.com/crashpad/crashpad.git/+/2f481590112b42aa21010abceaa3742e50775875 commit 2f481590112b42aa21010abceaa3742e50775875 Author: Nico Weber <thakis@chromium.org> Date: Wed Oct 25 19:02:47 2017 Make crashpad_util_test build without warnings with clang-cl on Windows. This upstreams https://chromium-review.googlesource.com/c/chromium/src/+/735820/ Bug: chromium:777924 Change-Id: I9fe76b839442d73a6c2836ccfe6cbe41acd67fad Reviewed-on: https://chromium-review.googlesource.com/738394 Reviewed-by: Mark Mentovai <mark@chromium.org> Commit-Queue: Nico Weber <thakis@chromium.org> [modify] https://crrev.com/2f481590112b42aa21010abceaa3742e50775875/test/scoped_temp_dir_win.cc [modify] https://crrev.com/2f481590112b42aa21010abceaa3742e50775875/test/win/win_child_process.cc [modify] https://crrev.com/2f481590112b42aa21010abceaa3742e50775875/util/stdlib/strlcpy_test.cc [modify] https://crrev.com/2f481590112b42aa21010abceaa3742e50775875/util/win/capture_context_test.cc [modify] https://crrev.com/2f481590112b42aa21010abceaa3742e50775875/util/win/command_line_test.cc [modify] https://crrev.com/2f481590112b42aa21010abceaa3742e50775875/util/win/exception_handler_server_test.cc [modify] https://crrev.com/2f481590112b42aa21010abceaa3742e50775875/util/win/process_info_test.cc [modify] https://crrev.com/2f481590112b42aa21010abceaa3742e50775875/util/win/safe_terminate_process_test.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ef647b4938a6c0b5fccf740ef44e1654d80320a4 commit ef647b4938a6c0b5fccf740ef44e1654d80320a4 Author: Nico Weber <thakis@chromium.org> Date: Wed Oct 25 19:16:33 2017 Make crashpad_util_test buildable in gn builds. Fix all warnings emitted in cl.exe and clang-cl Windows builds. I verified that crashpad_util_test works in those two configurations. This omits the tests in process/, because that code is linux-only and .gn files don't hook up linux code yet. Some new tests are only added on non-Windows since they fail on Windows (see util/BUILD.gn). Bug: 777924 Change-Id: I5378085f78908af2aefb93991087e0190938641f Reviewed-on: https://chromium-review.googlesource.com/735820 Reviewed-by: Mark Mentovai <mark@chromium.org> Cr-Commit-Position: refs/heads/master@{#511547} [add] https://crrev.com/ef647b4938a6c0b5fccf740ef44e1654d80320a4/build/secondary/third_party/crashpad/crashpad/test/BUILD.gn [modify] https://crrev.com/ef647b4938a6c0b5fccf740ef44e1654d80320a4/build/secondary/third_party/crashpad/crashpad/util/BUILD.gn [modify] https://crrev.com/ef647b4938a6c0b5fccf740ef44e1654d80320a4/third_party/crashpad/crashpad/test/scoped_temp_dir_win.cc [modify] https://crrev.com/ef647b4938a6c0b5fccf740ef44e1654d80320a4/third_party/crashpad/crashpad/test/win/win_child_process.cc [modify] https://crrev.com/ef647b4938a6c0b5fccf740ef44e1654d80320a4/third_party/crashpad/crashpad/util/stdlib/strlcpy_test.cc [modify] https://crrev.com/ef647b4938a6c0b5fccf740ef44e1654d80320a4/third_party/crashpad/crashpad/util/win/capture_context_test.cc [modify] https://crrev.com/ef647b4938a6c0b5fccf740ef44e1654d80320a4/third_party/crashpad/crashpad/util/win/command_line_test.cc [modify] https://crrev.com/ef647b4938a6c0b5fccf740ef44e1654d80320a4/third_party/crashpad/crashpad/util/win/exception_handler_server_test.cc [modify] https://crrev.com/ef647b4938a6c0b5fccf740ef44e1654d80320a4/third_party/crashpad/crashpad/util/win/process_info_test.cc [modify] https://crrev.com/ef647b4938a6c0b5fccf740ef44e1654d80320a4/third_party/crashpad/crashpad/util/win/safe_terminate_process_test.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c10b28fde4d96b3cb9d9672c08c9d8a940e062a5 commit c10b28fde4d96b3cb9d9672c08c9d8a940e062a5 Author: Nico Weber <thakis@chromium.org> Date: Wed Oct 25 20:23:10 2017 Fix 64-bit build of crashpad_util_test. crashpad/util/win/process_info_test.cc(47,6): error: unused function 'IsProcessWow64' [-Werror,-Wunused-function] bool IsProcessWow64(HANDLE process_handle) { ^ And a bunch of -Wsign-compare. TBR=mark Bug: 777924 Change-Id: Ibbb4b05343647abafb5972dcb57157d4bcba87e3 Reviewed-on: https://chromium-review.googlesource.com/738402 Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#511557} [modify] https://crrev.com/c10b28fde4d96b3cb9d9672c08c9d8a940e062a5/third_party/crashpad/crashpad/util/win/capture_context_test.cc [modify] https://crrev.com/c10b28fde4d96b3cb9d9672c08c9d8a940e062a5/third_party/crashpad/crashpad/util/win/process_info_test.cc
Copying some stuff over from bug 777579 comment 30. Clang (from stp_clang.asm, https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=308768): mov eax, dword ptr [__imp__TerminateProcess@8] […] call dword ptr [eax] MSVC (from stp_msvc.asm, https://bugs.chromium.org/p/chromium/issues/attachmentText?aid=308769): call DWORD PTR __imp__TerminateProcess@8 Looks like Clang version is doing a double-dereference. For reference, here’s the code that we were compiling (assembling): https://chromium.googlesource.com/chromium/src/+/912c9907d5636ec45d59f95cd0902f4215e027d9/third_party/crashpad/crashpad/util/win/safe_terminate_process.cc#25: call TerminateProcess The pure asm version, https://chromium.googlesource.com/chromium/src/+/af5f31ed615bdbf0ad246cff643a2802a1e0d702/third_party/crashpad/crashpad/util/win/safe_terminate_process.asm#53, uses extern __imp__TerminateProcess@8:proc […] call dword ptr [__imp__TerminateProcess@8]
The following revision refers to this bug: https://chromium.googlesource.com/crashpad/crashpad.git/+/9bc5989125b82a7d10e594cf0402f732aaf0a6fe commit 9bc5989125b82a7d10e594cf0402f732aaf0a6fe Author: Nico Weber <thakis@chromium.org> Date: Wed Oct 25 20:53:51 2017 crashpad_util_test warning fixes for clang-cl, 64-bit edition. This upstreams https://chromium-review.googlesource.com/c/chromium/src/+/738402 Bug: chromium:777924 Change-Id: Ib3c8f4f77631da45a2911029e8925c1afad1c244 Reviewed-on: https://chromium-review.googlesource.com/738553 Commit-Queue: Nico Weber <thakis@chromium.org> Commit-Queue: Scott Graham <scottmg@chromium.org> Reviewed-by: Scott Graham <scottmg@chromium.org> Reviewed-by: Mark Mentovai <mark@chromium.org> [modify] https://crrev.com/9bc5989125b82a7d10e594cf0402f732aaf0a6fe/util/win/capture_context_test.cc [modify] https://crrev.com/9bc5989125b82a7d10e594cf0402f732aaf0a6fe/util/win/process_info_test.cc
Upstream: https://bugs.llvm.org/show_bug.cgi?id=35056
The following revision refers to this bug: https://chromium.googlesource.com/crashpad/crashpad.git/+/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323 commit 5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323 Author: Mark Mentovai <mark@chromium.org> Date: Thu Oct 26 18:31:57 2017 win: Dynamically disable WoW64 tests absent explicit 32-bit build output Rather than having the 64-bit build assume that it lives in out\{Debug,Release}_x64 and that it can find 32-bit build output in out\{Debug,Release}, require the location of 32-bit build output to be provided explicitly via the CRASHPAD_TEST_32_BIT_OUTPUT environment variable. If this variable is not set, 64-bit tests that require 32-bit test build output will dynamically disable themselves at runtime. In order for this to work, a new DISABLED_TEST() macro is added to support dynamically disabled tests. gtest does not have its own first-class support for this (https://groups.google.com/d/topic/googletestframework/Nwh3u7YFuN4, https://github.com/google/googletest/issues/490) so this local solution is used instead. For tests via Crashpad’s own build\run_tests.py, which is how Crashpad’s own buildbots and trybots invoke tests, CRASHPAD_TEST_32_BIT_OUTPUT is set to a locaton compatible with the paths expected for the GYP-based build. No test coverage is lost on Crashpad’s own buildbots and trybots. For Crashpad tests in Chromium’s buildbots and trybots, this environment variable will not be set, causing these tests to be dynamically disabled. Bug: crashpad:203 , chromium:743139, chromium:777924 Change-Id: I3c0de2bf4f835e13ed5a4adda5760d6fed508126 Reviewed-on: https://chromium-review.googlesource.com/739795 Commit-Queue: Mark Mentovai <mark@chromium.org> Reviewed-by: Scott Graham <scottmg@chromium.org> [modify] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/build/run_tests.py [modify] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/snapshot/win/end_to_end_test.py [modify] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/snapshot/win/exception_snapshot_win_test.cc [modify] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/snapshot/win/extra_memory_ranges_test.cc [modify] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/snapshot/win/pe_image_annotations_reader_test.cc [modify] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/snapshot/win/process_snapshot_win_test.cc [modify] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/test/gmock_main.cc [add] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/test/gtest_disabled.cc [add] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/test/gtest_disabled.h [modify] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/test/gtest_main.cc [modify] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/test/test.gyp [modify] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/test/test_paths.cc [modify] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/test/test_paths.h [modify] https://crrev.com/5e9ed4cb9f69ffc60e47d7e5d8312f6417dcb323/util/win/process_info_test.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/00a0654929787f70b0cd81f30aa14e81c5e11b2f commit 00a0654929787f70b0cd81f30aa14e81c5e11b2f Author: Nico Weber <thakis@chromium.org> Date: Fri Oct 27 19:20:10 2017 Build and run crashpad_util_test on Windows bots. Ran python tools\mb\mb.py run out\gn crashpad_util_test to locally verify data and data_deps entries for swarming. Swarming apparently requires the use of base::TestLauncher, so use that in Crashpad-in-Chromium builds. Disable a bunch of tests that fail in that setup. Bug: 777924 , 778672 Change-Id: I4aa828040d488755582e5559d5f117e1ed8d1c86 Reviewed-on: https://chromium-review.googlesource.com/737439 Commit-Queue: Nico Weber <thakis@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Reviewed-by: Mark Mentovai <mark@chromium.org> Cr-Commit-Position: refs/heads/master@{#512262} [modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/build/secondary/third_party/crashpad/crashpad/test/BUILD.gn [modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/build/secondary/third_party/crashpad/crashpad/util/BUILD.gn [modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/testing/buildbot/chromium.clang.json [modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/testing/buildbot/chromium.fyi.json [modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/testing/buildbot/chromium.win.json [modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/testing/buildbot/gn_isolate_map.pyl [modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/third_party/crashpad/crashpad/test/gmock_main.cc [modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/third_party/crashpad/crashpad/test/gtest_main.cc [modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/third_party/crashpad/crashpad/util/win/exception_handler_server_test.cc [modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/third_party/crashpad/crashpad/util/win/process_info_test.cc [modify] https://crrev.com/00a0654929787f70b0cd81f30aa14e81c5e11b2f/third_party/crashpad/crashpad/util/win/scoped_process_suspend_test.cc
Figuring out and adding tests that auto-catch this is done. The root cause isn't fixed yet, but let's close out this bug here.
Comment 1 by thakis@chromium.org
, Oct 24 2017