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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to