[PATCH] D129004: [Support] Fix Windows dump file hang with multi-threaded crashes

[Andrew Ng via Phabricator via llvm-commits]

andrewng added inline comments.


================
Comment at: llvm/unittests/Support/WindowsDumpFileTest.cpp:41
+
+  if (getenv("LLVM_WINDOWS_DUMP_FILE_TEST_MULTI_THREAD")) {
+    ASSERT_FALSE(sys::Process::AreCoreFilesPrevented());
----------------
rnk wrote:
> I appreciate the effort and thoroughness that went into writing this test, but I don't think we should commit it as is.
> 
> If the critical section is removed, this test will nondeterministically fail with a timeout due to the deadlock. That's not something we can use to bisect problems or debug why things went wrong. I don't think it's worth spending resources to run this test if it can't produce actionable results from a buildbot.
I'm not sure that there's a "better" way to test this issue. Do you have any suggestions? Or are you suggesting to just discard the testing completely? Which I think could be acceptable in this case. I added testing because that's usually a requirement for any change to be accepted.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129004/new/

https://reviews.llvm.org/D129004