[lit] Detect Inconsistent File Access Times

[stuij]

Summary

I was seeing failing tests in my builds due to another process (in my
case, antivirus) changing the access time of files during the tests,
so tests using touch -a were failing.

This change modifies how we check if a system has consistent atime. We
have a list of platforms where we know these tests don't work (NetBSD,
Windows) with their existing reasons documented.

On other platforms, we do the same check that some of the tests
themselves end up doing: set the atime using touch, and then check it
was preserved with ls. I have found this check to be robust on my
system, but it's relying on the process updating the atime to race
with the process reading the atime, so it may not be as robust as we
want. Only if the check is successful, then you get a consistent-atime
feature which you can make your test REQUIRE.

This updates all existing tests which mention atime or use touch -a to
REQUIRE: consistent-atime, fixing their flakiness.

[lenary]

Adding more reviewers. @mgorny you disabled some tests on NetBSD in the past for the same reason (rG92dc7dce4a6f117a497ced1650bc48e5b658f0ea), this just updates how they were disabled.

[jhenderson]

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?

llvm/utils/lit/lit/llvm/config.py

165

Is "hopefully" really needed here?

[lenary]

In 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.

llvm/utils/lit/lit/llvm/config.py

165

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.

[int3]

llvm/utils/lit/lit/llvm/config.py

165

if we know for sure that NetBSD and Windows don't support this, why not hardcode those values in, and only try the experimental check otherwise?

[michaelplatings]

Thanks very much for looking into this, the flakiness of these tests has been bugging me for ages.

llvm/utils/lit/lit/llvm/config.py

171

It looks like this command will be run on Windows. I think it will fail and cause False to be returned, which is the desired result, but this appears to be by accident rather than design. Therefore I'm inclined to agree with @int3 that a hard-coded check would be preferable.

[lenary]

Updates incoming to add a specific check for netbsd and windows, which are currently excluding the affected tests anyway.

llvm/utils/lit/lit/llvm/config.py

165

Yeah, this seems pragmatic, I will add it back in again - excluding NetBSD and Windows.

171

I am going to add the hardcoded checks, but I think touch is available in windows, it should be in the same directory as all the git binaries.

[michaelplatings]

llvm/utils/lit/lit/llvm/config.py

171

This is definitely tangential to the change, but in case it's useful to know: conventionally only C:\Program Files\Git\bin is added to the path on Windows, not C:\Program Files\Git\usr\bin. C:\Program Files\Git\bin only contains bash.exe, git.exe and sh.exe.

[lenary]

llvm/utils/lit/lit/llvm/config.py

171

Something weirder is happening in _find_git_windows_unix_tools (from https://reviews.llvm.org/D84380), but I think it's probably right just to early exit with false on Windows.

[lenary]

Hopefully this addresses the feedback so far.

[michaelplatings]

I see some of these tests previously had UNSUPPORTED: system-netbsd but not UNSUPPORTED: system-windows - do you know why?

llvm/utils/lit/lit/llvm/config.py

190

This could end up matching the wrong part of the string, for example if the temporary file happened to have 1995 in its name. A regex match for '\b1995\b' would be more reliable.

[jhenderson]

llvm/utils/lit/lit/llvm/config.py

171

I've got no objection to explicitly omitting Windows, but just wanted to point out that LLVM requires various Unix-like tools to be available according to https://llvm.org/docs/GettingStarted.html#software. Whilst touch isn't explicitly specified, I'd be surprised if you managed to get access to that explicit list without including touch inadvertently (it's also worth noting that there are several other tests that use touch and other utilities that are intended to work on Windows). I thought that Git\usr\bin was recommended to be included in people's paths for Windows, as the way to get access to these tools, but I couldn't find that recommendation, so maybe that's just our downstream recommendation.

[lenary]

In D144638#4155217, @michaelplatings wrote:

I see some of these tests previously had UNSUPPORTED: system-netbsd but not UNSUPPORTED: system-windows - do you know why?

I'm not entirely sure why, but I have some ideas:

• rGa680ea2c720751a3d724fd8282515924acb3bd32 disabled the llvm-objdump test - I don't quite know why it didn't disable the ThinLTO/x86/cache.ll test (which did exist at the time).
• rG929ce27cc77dd1fbf909ef4d42614eee9d11880e, rG29935acdb10679f27f028cf9dff1971c839dc51d, and rG92dc7dce4a6f117a497ced1650bc48e5b658f0ea are the netbsd disables, which were done all approximately at the same time, across clang, llvm and lld.

It looks like the pruning logic for both LTO and Clang modules is the same, using llvm::sys::fs::file_status::getLastAccessedTime() (but, this of course has to use the underlying system information), which is:

• On windows, atime will be zero if unsupported: If the underlying file system does not support last access time, this member is zero. https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-win32_find_dataw which should be a long time in the past.
• The mac, netbsd and linux docs for noatime just say something to the effect of "the access time is never updated", not the value that it reads as if you have noatime.

I've actually just done a more thorough search, and there are other tests that use touch -a (a few in clang/test/PCH, some others in clang/test/Modules). I presume this comes down to logic as to whether the code uses getLastAccessedTime which seems to be used:

• in the modules cache prune logic (pruneModuleCache in clang/lib/Frontend/CompilerInstance.cpp)
• in the LTO cache pruning logic (llvm::pruneCache)
• in FileCollector, as used by Modules (llvm/include/llvm/Support/FileCollector.h)
• in FilePermissionsApplier, as used by llvm-dwarfutil and llvm-objcopy (declared in llvm/include/llvm/Support/FileUtilities.h)

I cannot really see any correlation between the logic under test, and the use of touch -a or touch -a -m vs just touch. I'm not sure what the right "fix" should be any more. I guess I'm just happy for everyone who is not running into this issue.

[lenary]

llvm/utils/lit/lit/llvm/config.py

190

Done.

[michaelplatings]

LGTM

[lenary]

@jhenderson @int3 I think I have addressed your feedback - are you happy for me to land this?

[jhenderson]

Related to my inline comment, your changes will result in some tests being disabled on Windows that weren't before (at least one of the tests pass for me even on my machine where atime is disabled). I think we need to understand why these tests pass on Windows before losing test coverage by disabling them.

llvm/utils/lit/lit/llvm/config.py

172

As mentioned, I don't think this is a fair statement: many tests use touch or ls, and work fine on Windows. That being said, the top Google result for how to detect if access time is enabled on Windows yields the following command: fsutil behavior query disablelastaccess, which prints some information about its state that could be easily queried in python to give a more correct answer for Windows.

[lenary]

To be clear: I've not yet addressed @jhenderson's comments above, yet, which may require changes to the patch.

clang/test/Modules/prune.m

-// NetBSD: noatime mounts currently inhibit 'touch -a' updates
-// UNSUPPORTED: system-netbsd
+// REQUIRES: consistent-atime
 
 // Test the automatic pruning of module cache entries.
 #ifdef IMPORT_DEPENDS_ON_MODULE
 @import DependsOnModule;
 #else
 @import Module;
 #endif
 

lld/test/COFF/lto-cache.ll

 ; REQUIRES: x86
-; NetBSD: noatime mounts currently inhibit 'touch' from updating atime
-; UNSUPPORTED: system-netbsd
+; REQUIRES: consistent-atime
 
 ; RUN: opt -module-hash -module-summary %s -o %t.o
 ; RUN: opt -module-hash -module-summary %p/Inputs/lto-cache.ll -o %t2.o
 
 ; RUN: rm -Rf %t.cache && mkdir %t.cache
 ; Create two files that would be removed by cache pruning due to age.
 ; We should only remove files matching "llvmcache-*" or "Thin-*".
 ; RUN: touch -t 197001011200 %t.cache/llvmcache-foo %t.cache/Thin-123.tmp.o %t.cache/foo

lld/test/ELF/lto/cache.ll

 ; REQUIRES: x86
-; NetBSD: noatime mounts currently inhibit 'touch' from updating atime
-; UNSUPPORTED: system-netbsd
+; REQUIRES: consistent-atime
 
 ; RUN: opt -module-hash -module-summary %s -o %t.o
 ; RUN: opt -module-hash -module-summary %p/Inputs/cache.ll -o %t2.o
 
 ; RUN: rm -Rf %t.cache && mkdir %t.cache
 ; Create two files that would be removed by cache pruning due to age.
 ; We should only remove files matching the pattern "llvmcache-*".
 ; RUN: touch -t 197001011200 %t.cache/llvmcache-foo %t.cache/foo

lld/test/MachO/lto-cache.ll

 ; REQUIRES: x86
-; NetBSD: noatime mounts currently inhibit 'touch' from updating atime
-; UNSUPPORTED: system-netbsd
+; REQUIRES: consistent-atime
 
 ; RUN: rm -rf %t; split-file %s %t
 ; RUN: opt -module-hash -module-summary %t/foo.ll -o %t/foo.o
 ; RUN: opt -module-hash -module-summary %t/bar.ll -o %t/bar.o
 
 ; RUN: rm -Rf %t/cache && mkdir %t/cache
 ;; Create two files that would be removed by cache pruning due to age.
 ;; We should only remove files matching the pattern "llvmcache-*".

lld/test/wasm/lto/cache.ll

 ; RUN: opt -module-hash -module-summary %s -o %t.o
 ; RUN: opt -module-hash -module-summary %p/Inputs/cache.ll -o %t2.o
-; NetBSD: noatime mounts currently inhibit 'touch' from updating atime
-; Windows: no 'touch' command.
-; UNSUPPORTED: system-netbsd, system-windows
+; REQUIRES: consistent-atime
 
 ; RUN: rm -Rf %t.cache && mkdir %t.cache
 ; Create two files that would be removed by cache pruning due to age.
 ; We should only remove files matching the pattern "llvmcache-*".
 ; RUN: touch -t 197001011200 %t.cache/llvmcache-foo %t.cache/foo
 ; RUN: wasm-ld --thinlto-cache-dir=%t.cache --thinlto-cache-policy prune_after=1h:prune_interval=0s -o %t.wasm %t2.o %t.o
 
 ; Two cached objects, plus a timestamp file and "foo", minus the file we removed.

llvm/test/ThinLTO/X86/cache.ll

-; NetBSD: noatime mounts currently inhibit 'touch -a' updates
-; UNSUPPORTED: system-netbsd
+; REQUIRES: consistent-atime
 
 ; The .noindex suffix for output dir is to prevent Spotlight on macOS from
 ; indexing it.
 
 ; Verify first that *without* hash, we don't use the cache.
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/cache.ll -o %t2.bc
 

llvm/test/tools/llvm-objcopy/ELF/strip-preserve-atime.test

 # Note: ls -lu prints the accessed timestamp
-# NetBSD: noatime mounts currently inhibit 'touch -a' updates
-# Windows: the last access time is disabled by default in the OS
-# UNSUPPORTED: system-netbsd, system-windows
+# REQUIRES: consistent-atime
 
 # Preserve dates when stripping to an output file.
 # RUN: yaml2obj %s -o %t.1.o
 # RUN: touch -a -t 199505050555.55 %t.1.o
 # RUN: llvm-strip -p %t.1.o -o %t-preserved.1.o
 # RUN: ls -lu %t-preserved.1.o | FileCheck %s --check-prefix=CHECK-PRESERVE-ATIME
 # Check that the stripped output is in fact a valid object file.
 # RUN: llvm-readobj %t-preserved.1.o

llvm/utils/lit/lit/llvm/config.py

 import itertools
 import os
 import platform
 import re
 import subprocess
 import sys
 import errno
+import tempfile
 
 import lit.util
 from lit.llvm.subst import FindTool
 from lit.llvm.subst import ToolSubst
 
 lit_path_displayed = False
 
 class LLVMConfig(object):
def __init__(self, lit_config, config):
             # Allow use of an explicit path for gmalloc library.
             # Will default to '/usr/lib/libgmalloc.dylib' if not set.
             gmalloc_path_str = lit_config.params.get(
                 'gmalloc_path', '/usr/lib/libgmalloc.dylib')
             if gmalloc_path_str is not None:
                 self.with_environment(
                     'DYLD_INSERT_LIBRARIES', gmalloc_path_str)
 
+        if self._has_consistent_atime():
+            features.add('consistent-atime')
+
+    # Some tests use `touch` to set the access time for a file, and expect no
+    # other processes to change this during the test run. This is not always the
+    # case, so we have a way to disable these tests on systems where the access
+    # might not be preserved.
+    def _has_consistent_atime(self):
+        # NetBSD: noatime mounts currently inhibit 'touch -a' updates.
+        if platform.system() == 'NetBSD':
+            return False
+
+        # Windows: the last access time is disabled by default in the OS, and
+        # the check below is written in terms of unix utilities (touch, ls),
+        # which will not work on this platform.
+        if platform.system() == 'Windows':
+            return False
+
+        # Other Platforms: Try to use `touch -a` to set the atime, and then to
+        # read it with `ls`. If they don't match, or either process errors, then
+        # don't claim that atime is consistent.
+        with tempfile.NamedTemporaryFile() as f:
+            # Specific date in the past on purpose, based on what is attempted
+            # in the tests that do the same thing.
+            (_, try_touch_err) = self.get_process_output(["touch", "-a", "-t", "199505050555.55", f.name])
+            if try_touch_err != "":
+                return False
+
+            (touch_res_out, touch_res_err) = self.get_process_output(["ls", "-lu", f.name])
+            if touch_res_err != "":
+                return False
+            if "1995" not in touch_res_out:
+                return False
+
+            return True
+
     def _find_git_windows_unix_tools(self, tools_needed):
         assert(sys.platform == 'win32')
         if sys.version_info.major >= 3:
             import winreg
         else:
             import _winreg as winreg
 
         # Search both the 64 and 32-bit hives, as well as HKLM + HKCU