Narrow the CRC scope in file_test #5043
Conversation
48e598c to
7a33be3
Compare
testing/file_test/file_test_base.cpp
Outdated
| } | ||
| }); | ||
| if (thread_crashed) { | ||
| // Guard access to both `llvm::errs` and `crashed`. |
There was a problem hiding this comment.
I was wondering while reading if GUARDED_BY and REQUIRES would be useful here. I've not actually written code with them before though.
There was a problem hiding this comment.
We don't own the things we're trying to lock, many different systems touch output, so it's an awkward fit for what we're trying to lock here. We'd basically need to add helpers around things like llvm::errs(), and even then we should expect incomplete coverage because there are uses that are too indirect to capture.
That said, we also don't enable -fsanitize=thread so I think annotations would probably be brittle. If you think this would be worthwhile to change, it seems out of the scope of this PR, so perhaps discuss it on #infra?
Co-authored-by: Geoff Romer <gromer@google.com>
This narrows the scope of the CRC to try to get better behavior around mutex lock releasing on crash. Closes #5042.
This breaks apart
ProcessTestFileAndRunbecause we need to process the test file forSET-CAPTURE-CONSOLE-OUTPUT. The test file processing should more reliably not crash than the coreRunlogic though, so should be reasonably safe to put outside the CRC.Also support --threads=1 for disabling threading. This is the flipside for me of reducing how much is in the CRC: make it easier to run on a single thread if the CRC gets in the way of debugging. This also means a typical copy-paste execution of a single test will be single-threaded.