[PATCH] D144638: [lit] Detect Consistent File Access Times

Thu Feb 23 06:16:26 PST 2023

lenary added a comment.

In D144638#4147133 <https://reviews.llvm.org/D144638#4147133>, @jhenderson wrote:

> This looks reasonable to me, with the caveat that I don't know a huge amount about how the different OSes access time systems work. One question though: if your antivirus was causing flakiness (as opposed to outright always-fails), won't it just move that flakiness into whether the REQUIRES calculation returns true or not (i.e. it could spuriously do so, causing the tests to be enabled but then potential still be flaky?

I'm definitely not an expert in OS atimes either, beyond the comments that have accumulated in the repo so far, and knowing that some filesystems prefer to have it switched off for performance reasons.

The flakiness I (and colleagues) have seen seems to correspond to computer load. In my case, I think the underlying issue is a race between the `touch -a -t <timestamp> <file>` (which in these test creates `<file>`), then the `ls -ul <file>`, and the antivirus, which is watching for file creation, and will also access the file. This is also one reason why I put "hopefully" in the comment - we're hoping to get the same race during lit configuration. One reason I think we're more likely to is that this code is run before we spin up lots of parallel threads to start testing, so the test machine should be under less load for this check, which should allow the antivirus to get in before the `ls -ul`, if all goes to plan (it's a race, so this is hard to be sure about).

I can see your point about this moving the flakiness, but I think that this check is more likely to fail in the other direction: marking the tests as unsupported when they might be ok. In that case, we lose coverage when we didn't need to.

================
Comment at: llvm/utils/lit/lit/llvm/config.py:165
+    #
+    # This check hopefully detects both cases, and disables tests that require
+    # consistent atime.
----------------
jhenderson wrote:
> Is "hopefully" really needed here?
I'm hedging in this comment, in part because we're trying to find a race condition experimentally, and also because this will cause lit to fatal error before running any tests if `touch` exits non-zero for any reason. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144638