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

Thu Jul 7 09:56:47 PDT 2022

rnk 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());
----------------
andrewng wrote:
> 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.
Yes, I think it's fine to land this code change without a test. It doesn't seem practical in this case.

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

https://reviews.llvm.org/D129004