[LIT] Make `%T` unique for every test

[ldionne]

Summary

This allows libc++ to use the same temp directory created by LIT's sh test, because that path is now hermetic to the test.

[EricWF]

llvm/utils/lit/lit/TestRunner.py

1062

@ldionne Here's the relevant LIT change.

1500

This change probably isn't needed.

[ldionne]

I really like that lit could provide a unique-per-test temporary directory. This would simplify things quite a bit.

A few more comments:

1. The Lit documentation for %T should be updated
2. Does any lit test need to be added?
3. The ssh.py executor now needs to get passed --execdir %T, and it needs to copy the dependencies into the current execdir, and then tar up all the execdir.

libcxx/utils/libcxx/test/newformat.py

204

You can remove fileDependencies=None now.

209

And this becomes fileDependencies = [], unconditionally.

libcxx/utils/run.py

46

A few comments around this part of the change:

1. Must fix this comment -- instead we can say that we assume it to already exist.
2. I think you can remove that try-finally altogether now.

However, this change means that we won't be able to copy-paste a RUN line from a failed test and execute it as-is. Before we do that, we'll need to make sure that the --execdir in the command-line exists. This wouldn't be an issue if we kept this code as-is, which would be my preference.

llvm/utils/lit/lit/TestRunner.py

1500

I think it is, actually, cause we do need to make sure that tests either clean-up after themselves (which they did before in run.py due to shutil.rmtree(args.execdir)), or that they clean up before they run in case a previous run left something weird.

@EricWF As-is, the exec dir for each test will contain stuff between test runs, which takes up space and you have asked me to remove that IIRC . Instead, why don't we remove the directory after we're done running the test in run.py above? This would address this issue, in addition to making RUN lines in libc++ copy-pasteable.

[thakis]

Are you aware of https://llvm.org/docs/CommandGuide/lit.html#substitutions ("%T: parent directory of %t (not unique, deprecated, do not use)")? We've talked about this a bunch in the past, and the decision back then was to do mkdir %t in the tests that need a dir, and to remove %T over time.

[ldionne]

In D78245#1985201, @thakis wrote:

Are you aware of https://llvm.org/docs/CommandGuide/lit.html#substitutions ("%T: parent directory of %t (not unique, deprecated, do not use)")? We've talked about this a bunch in the past, and the decision back then was to do mkdir %t in the tests that need a dir, and to remove %T over time.

I was aware of it, and TBH I don't know why it's that way, but I've wished it would be different several times. Doing mkdir %t isn't an option for us, as all tests would need to do it. So we'd use the solution we used before this patch.

@thakis Could you link to previous discussions about %T? I'm sure there are reasons for the way things are today, but I'd like to understand them.

[EricWF]

In D78245#1985201, @thakis wrote:

Are you aware of https://llvm.org/docs/CommandGuide/lit.html#substitutions ("%T: parent directory of %t (not unique, deprecated, do not use)")? We've talked about this a bunch in the past, and the decision back then was to do mkdir %t in the tests that need a dir, and to remove %T over time.

As @ldionne said, this is less than ideal for the libc++ tests, which aren't structured as true ShTest's. I would be happy with a solution that allowed libc++ to twist these knobs without affecting everybody.

libcxx/utils/run.py

46

I would rather remove execdir and assume we run the binary in the same directory it's in. Then it's trivially true that the directory exists.
That's much less complicated than all this.

llvm/utils/lit/lit/TestRunner.py

1500

I actually really do like having the failing executables still on the filesystem, because they're a pain to reproduce manually.

[ldionne]

libcxx/utils/run.py

46

The problem is that we must then cd <directory> before we can execute the test when copy-pasting a RUN line from a failed test. The thing I *really* love right now with the new format is that you can always just copy-paste the compiler command line (after COMPILED AS:) and then the execution line (after EXECUTED AS:) from any directory, and it all just works.

Perhaps we could make it clearer by adding the directory where a test is executed to lit's output? Otherwise, we have to basically infer it from the compiler command-line with the location of -o, and that's just annoying to do every time.

llvm/utils/lit/lit/TestRunner.py

1500

They are not anymore -- you can just copy-paste the COMPILED AS: command line and it just works. It does with the new format currently, at least. This was one of the explicit design goals, because I was really annoyed by that specific shortcoming of the old format.

[ldionne]

In D78245#1986587, @ldionne wrote:

In D78245#1985201, @thakis wrote:

Are you aware of https://llvm.org/docs/CommandGuide/lit.html#substitutions ("%T: parent directory of %t (not unique, deprecated, do not use)")? We've talked about this a bunch in the past, and the decision back then was to do mkdir %t in the tests that need a dir, and to remove %T over time.

I was aware of it, and TBH I don't know why it's that way, but I've wished it would be different several times. Doing mkdir %t isn't an option for us, as all tests would need to do it. So we'd use the solution we used before this patch.

@thakis Could you link to previous discussions about %T? I'm sure there are reasons for the way things are today, but I'd like to understand them.

Gentle ping -- are there previous discussions on why %T was deprecated? I'm sure there is, but in the absence of any documentation supporting otherwise, it seems to be a useful feature.

[thakis]

@thakis Could you link to previous discussions about %T? I'm sure there are reasons for the way things are today, but I'd like to understand them.

I remember reading the discussion, but I failed to find it just now. @dsanders added the docs that say "deprecated"; maybe he remembers where the thread on moving off %T was?

[thakis]

Ah, I did find it after all, by git log --grep '%T' llvm/test. The discussion was at https://reviews.llvm.org/D35396.

[thakis]

(Thanks @dblaikie for the good commit message :) )

[probinson]

D35396 is pretty long, so as a public service, summarizing the objections to %T from D35396, having lit always pre-create a %T directory would slow down test runs. Considering %T has been deprecated and work done to remove it from llvm, clang, etc, there are not many tests that use it and it would pretty much be an unnecessary penalty for everyone (except libcxx, who needs to create the directory anyway). (I think I got that right.)

If lit would do this directory pre-creation only if it actually saw %T in the test, that I think would answer the objection.

[dsanders]

In D78245#2010879, @thakis wrote:

@thakis Could you link to previous discussions about %T? I'm sure there are reasons for the way things are today, but I'd like to understand them.

I remember reading the discussion, but I failed to find it just now. @dsanders added the docs that say "deprecated"; maybe he remembers where the thread on moving off %T was?

I only added more whitespace to the line to make %{/s:regex_replacement} fit in the column. The bit about %T being deprecated was added in:
commit a2e0c2462a5ed4ddabab03dfddb97efc54682bde
Author: Kuba Mracek <[e-mail]>
Date: Sat Aug 25 01:27:48 2018 +0000

[llvm] Document "%T" as deprecated in CommandGuide/lit.rst

Differential Revision: https://reviews.llvm.org/D48842

llvm-svn: 340677

[ldionne]

I went ahead and committed 4e813bbdf335626a741398314709a535ef52fe8e, which basically implements this functionality for libc++ only. A followup patch will cleanup the need for tracking file dependencies everywhere.

I think this review can be closed.

[thakis]

rG82f3c5d4a66 cross-ref so i can find this faster next time

[MaskRay]

In D78245#2255317, @thakis wrote:

rG82f3c5d4a66 cross-ref so i can find this faster next time

@EricWF Seems that the revision can be abandoned now?

[ldionne]

Commandeering so I can abandon to clean up the review queue.

[ldionne]

Abandoning since this was implemented in 4e813bbdf335626a741398314709a535ef52fe8e. Thanks @EricWF for the improvement, which turned out to simplify a bunch of things!

libcxx/test/libcxx/atomics/atomics.align/align.pass.sh.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // UNSUPPORTED: libcpp-has-no-threads, c++98, c++03
 // REQUIRES: libatomic
-// FILE_DEPENDENCIES: %t.exe
+
 // RUN: %{build} -latomic
 // RUN: %{run}
 //
 // GCC currently fails because it needs -fabi-version=6 to fix mangling of
 // std::atomic when used with __attribute__((vector(X))).
 // XFAIL: gcc
 
 // <atomic>

libcxx/test/libcxx/double_include.sh.cpp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // Test that we can include each header in two TU's and link them together.
 
-// FILE_DEPENDENCIES: %t.exe
 // RUN: %{cxx} -c %s -o %t.first.o %{flags} %{compile_flags}
 // RUN: %{cxx} -c %s -o %t.second.o -DWITH_MAIN %{flags} %{compile_flags}
 // RUN: %{cxx} -o %t.exe %t.first.o %t.second.o %{flags} %{link_flags}
 // RUN: %{run}
 
 // Prevent <ext/hash_map> from generating deprecated warnings for this test.
 #if defined(__DEPRECATED)
 #undef __DEPRECATED

libcxx/test/libcxx/language.support/support.dynamic/libcpp_deallocate.sh.cpp

 // even when it is supported.
 // UNSUPPORTED: apple-clang-9
 
 // XFAIL: sanitizer-new-delete, ubsan
 
 // GCC doesn't support the aligned-allocation flags.
 // XFAIL: gcc
 
-// FILE_DEPENDENCIES: %t.exe
 // RUN: %{build} -faligned-allocation -fsized-deallocation
 // RUN: %{run}
 // RUN: %{build} -faligned-allocation -fno-sized-deallocation -DNO_SIZE
 // RUN: %{run}
 // RUN: %{build} -fno-aligned-allocation -fsized-deallocation -DNO_ALIGN
 // RUN: %{run}
 // RUN: %{build} -fno-aligned-allocation -fno-sized-deallocation -DNO_ALIGN -DNO_SIZE
 // RUN: %{run}

libcxx/test/libcxx/selftest/exec.sh.cpp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-// FILE_DEPENDENCIES: %t.exe
-
 // RUN: %{build}
 // RUN: %{exec} %t.exe "HELLO"
 
 #include <cassert>
 #include <string>
 
 int main(int argc, char** argv) {
   assert(argc == 2);
 
   std::string arg = argv[1];
   assert(arg == "HELLO");
   return 0;
 }

libcxx/test/libcxx/selftest/newformat/sh.cpp/remote-substitutions.sh.cpp

 // effectively. This somewhat difficult-to-understand test checks that when
 // we run with a remote executor, test-executables are copied to the remote
 // host and their path is fixed up (directly in the command-line) to their
 // path on the remote host instead of the local host.
 //
 // We also check that the path of test-executables is replaced whether they
 // appear first in the command-line or not.
 
-// FILE_DEPENDENCIES: %t.exe
 // RUN: %{cxx} %s %{flags} %{compile_flags} %{link_flags} -o %t.exe
 // RUN: %{exec} %t.exe 0
 // RUN: %{exec} ! %t.exe 1
 
 #include <cassert>
 #include <cstdlib>
 
 int main(int argc, char** argv) {
   assert(argc == 2);
   int ret = std::atoi(argv[1]);
   return ret;
 }

libcxx/test/libcxx/selftest/newformat/sh.cpp/substitutions.sh.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // Make sure we have access to the following substitutions at all times:
 // - %{cxx}
 // - %{flags}
 // - %{compile_flags}
 // - %{link_flags}
 // - %{exec}
 
-// FILE_DEPENDENCIES: %t.exe
 // RUN: %{cxx} %s %{flags} %{compile_flags} %{link_flags} -o %t.exe
 // RUN: %{exec} %t.exe "HELLO"
 
 #include <cassert>
 #include <string>
 
 int main(int argc, char** argv) {
   assert(argc == 2);
 
   std::string arg = argv[1];
   assert(arg == "HELLO");
   return 0;
 }

libcxx/test/libcxx/selftest/test.sh.cpp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-// FILE_DEPENDENCIES: %t.exe
-
 // RUN: %{build}
 // RUN: %{run}
 
 int main(int, char**)
 {
 
   return 0;
 }

libcxx/test/libcxx/strings/basic.string/PR42676.sh.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // Regression test for PR42676.
 
-// FILE_DEPENDENCIES: %t.exe
 // RUN: %{cxx} %{flags} %s -o %t.exe %{compile_flags} %{link_flags} -D_LIBCPP_HIDE_FROM_ABI_PER_TU
 // RUN: %{run}
 
 #include <memory>
 #include <string>
 
 int main() {
     std::string s1(10u, '-', std::allocator<char>()); (void)s1;
     std::string s2("hello", std::allocator<char>()); (void)s2;
     std::string s3("hello"); (void)s3;
 }

libcxx/test/std/containers/associative/map/PR28469_undefined_behavior_segfault.sh.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-// FILE_DEPENDENCIES: %t.exe
 // RUN: %{build} -O2
 // RUN: %{run}
 
 // <map>
 
 // Previously this code caused a segfault when compiled at -O2 due to undefined
 // behavior in __tree. See https://bugs.llvm.org/show_bug.cgi?id=28469
 

libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_terminates.sh.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: libcpp-no-exceptions
 // UNSUPPORTED: libcpp-has-no-threads
 
 // <condition_variable>
 
 // class condition_variable_any;
 
-// FILE_DEPENDENCIES: %t.exe
 // RUN: %{build}
 // RUN: %{run} 1
 // RUN: %{run} 2
 // RUN: %{run} 3
 // RUN: %{run} 4
 // RUN: %{run} 5
 // RUN: %{run} 6
 

libcxx/utils/libcxx/test/config.py

             '--codesign_identity "{}"'.format(codesign_ident),
             '--dependencies %{file_dependencies}',
             '--env {}'.format(env_vars)
         ]
         if isinstance(self.executor, SSHExecutor):
             exec_args.append('--host {}'.format(self.executor.user_prefix + self.executor.host))
             executor = os.path.join(self.libcxx_src_root, 'utils', 'ssh.py')
         else:
-            exec_args.append('--execdir %t.execdir')
+            exec_args.append('--execdir %T')
             executor = os.path.join(self.libcxx_src_root, 'utils', 'run.py')
         sub.append(('%{exec}', '{} {} {} -- '.format(pipes.quote(sys.executable),
                                                      pipes.quote(executor),
                                                      ' '.join(exec_args))))
         sub.append(('%{run}', '%{exec} %t.exe'))
         if self.get_lit_conf('libcxx_gdb'):
             sub.append(('%{libcxx_gdb}', self.get_lit_conf('libcxx_gdb')))
 

libcxx/utils/libcxx/test/newformat.py

def execute(self, test, litConfig):
                 "%dbg(LINKED WITH) ! %{cxx} %t.o %{flags} %{link_flags} -o %t.exe"
             ]
             return self._executeShTest(test, litConfig, steps)
         elif filename.endswith('.run.fail.cpp'):
             steps = [
                 "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} %{link_flags} -o %t.exe",
                 "%dbg(EXECUTED AS) %{exec} ! %t.exe"
             ]
-            return self._executeShTest(test, litConfig, steps, fileDependencies=['%t.exe'])
+            return self._executeShTest(test, litConfig, steps)
         elif filename.endswith('.verify.cpp'):
             steps = [
                 "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} -fsyntax-only " + VERIFY_FLAGS
             ]
             return self._executeShTest(test, litConfig, steps)
         # Make sure to check these ones last, since they will match other
         # suffixes above too.
         elif filename.endswith('.pass.cpp') or filename.endswith('.pass.mm'):
             steps = [
                 "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} %{link_flags} -o %t.exe",
                 "%dbg(EXECUTED AS) %{exec} %t.exe"
             ]
-            return self._executeShTest(test, litConfig, steps, fileDependencies=['%t.exe'])
+            return self._executeShTest(test, litConfig, steps)
         # This is like a .verify.cpp test when clang-verify is supported,
         # otherwise it's like a .compile.fail.cpp test. This is only provided
         # for backwards compatibility with the test suite.
         elif filename.endswith('.fail.cpp'):
             if self._supportsVerify(test, litConfig):
                 steps = [
                     "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} -fsyntax-only " + VERIFY_FLAGS
                 ]
             else:
                 steps = [
                     "%dbg(COMPILED WITH) ! %{cxx} %s %{flags} %{compile_flags} -fsyntax-only"
                 ]
             return self._executeShTest(test, litConfig, steps)
         else:
             return lit.Test.Result(lit.Test.UNRESOLVED, "Unknown test suffix for '{}'".format(filename))
 
     # Utility function to add compile flags in lit.local.cfg files.
     def addCompileFlags(self, config, *flags):
         string = ' '.join(flags)
         config.substitutions = [(s, x + ' ' + string) if s == '%{compile_flags}' else (s, x) for (s, x) in config.substitutions]
 
     # Modified version of lit.TestRunner.executeShTest to handle custom parsers correctly.
     def _executeShTest(self, test, litConfig, steps, fileDependencies=None):
         if test.config.unsupported:
             return lit.Test.Result(lit.Test.UNSUPPORTED, 'Test is unsupported')
 
         additionalCompileFlags = []
         fileDependencies = fileDependencies or []
         parsers = [
             lit.TestRunner.IntegratedTestKeywordParser('FILE_DEPENDENCIES:',
                                                        lit.TestRunner.ParserKind.LIST,
                                                        initial_value=fileDependencies),
             lit.TestRunner.IntegratedTestKeywordParser('ADDITIONAL_COMPILE_FLAGS:',
                                                        lit.TestRunner.ParserKind.LIST,
                                                        initial_value=additionalCompileFlags)
         ]

libcxx/utils/run.py

if args.codesign_identity:
         rc = subprocess.call(['xcrun', 'codesign', '-f', '-s', args.codesign_identity, exe], env={})
         if rc != 0:
             sys.stderr.write('Failed to codesign: ' + exe)
             return rc
 
     # Extract environment variables into a dictionary
     env = {k : v  for (k, v) in map(lambda s: s.split('=', 1), args.env)}
 
     # Create the execution directory, and make sure we remove it at the end.
     try:
-        os.makedirs(args.execdir)
-
         # Ensure the file dependencies exist and copy them to the execution directory.
         for dep in args.dependencies:
             if not os.path.exists(dep):
                 sys.stderr.write('Missing file or directory "{}" marked as a dependency of a test'.format(dep))
                 exit(1)
             if os.path.isdir(dep):
                 shutil.copytree(dep, os.path.join(args.execdir, os.path.basename(dep)), symlinks=True)
             else:
                 shutil.copy2(dep, args.execdir)
 
         # Run the executable with the given environment in the execution directory.
         return subprocess.call(' '.join(remaining), cwd=args.execdir, env=env, shell=True)
     finally:
-        shutil.rmtree(args.execdir)
+        pass
 
 
 if __name__ == '__main__':
     exit(main())

libcxxabi/test/incomplete_type.sh.cpp

 // addresses.
 
 // UNSUPPORTED: libcxxabi-no-exceptions
 
 // NOTE: Link libc++abi explicitly and before libc++ so that libc++ doesn't drag
 // in the system libc++abi installation on OS X. (DYLD_LIBRARY_PATH is ignored
 // for shell tests because of Apple security features).
 
-// FILE_DEPENDENCIES: %t.exe
 // RUN: %{cxx} %{flags} %{compile_flags} -c %s -o %t.one.o
 // RUN: %{cxx} %{flags} %{compile_flags} -c %s -o %t.two.o -DTU_ONE
 // RUN: %{cxx} %{flags} %t.one.o %t.two.o %{link_libcxxabi} %{link_flags} -o %t.exe
 // RUN: %{exec} %t.exe
 
 #include <stdio.h>
 #include <cstring>
 #include <cassert>

llvm/utils/lit/lit/TestRunner.py

     finally:
         f.close()
 
 def getTempPaths(test):
     """Get the temporary location, this is always relative to the test suite
     root, not test source root."""
     execpath = test.getExecPath()
     execdir,execbase = os.path.split(execpath)
-    tmpDir = os.path.join(execdir, 'Output')
+    tmpDir = os.path.join(execdir, 'Output', execbase + '.dir')
     tmpBase = os.path.join(tmpDir, execbase)
     return tmpDir, tmpBase
 
 def colonNormalizePath(path):
     if kIsWindows:
         return re.sub(r'^(.):', r'\1', path.replace('\\', '/'))
     else:
         assert path[0] == '/'

         else:
             if timeoutInfo is None:
                 status = Test.FAIL
             else:
                 status = Test.TIMEOUT
         return out,err,exitCode,timeoutInfo,status
 
     # Create the output directory if it does not already exist.
-    lit.util.mkdir_p(os.path.dirname(tmpBase))
+    tmpDir = os.path.dirname(tmpBase)
+    if os.path.isdir(tmpDir):
+        shutil.rmtree(tmpDir)
+    lit.util.mkdir_p(tmpDir)
 
     # Re-run failed tests up to test.allowed_retries times.
     execdir = os.path.dirname(test.getExecPath())
     attempts = test.allowed_retries + 1
     for i in range(attempts):
         res = runOnce(execdir)
         if isinstance(res, lit.Test.Result):
             return res