[libcxx] [test] Don't rerun supportsVerify for each individual test

[mstorsjo]

Summary

We can't just memoize _supportsVerify in place in format.py, as it
previously was executed in each of the individual processes.

Instead move the function to dsl.py and add a feature flag for it
instead, which can be used both by tests (that already have such
a flag, locally for one set of tests) and for the testing framework
itself.

[ldionne]

libcxx/utils/libcxx/test/dsl.py

180–185

If we move it here, I suggest we rewrite it as:

return hasCompileFlag(config, '-Xclang -verify-ignore-unexpected')

libcxx/utils/libcxx/test/format.py

219

I wonder why I didn't do it that way from the start. I guess it's because we didn't have features.py when I first introduced the new testing format.

[mstorsjo]

libcxx/utils/libcxx/test/dsl.py

180–185

Ok, will try. That actually would fix two other issues I was planning on fixing afterwards.

[mstorsjo]

Use hasCompileFlag() instead of the original code.

[ldionne]

This LGTM, but even better would be to actually remove def supportsVerify(config) altogether and inline the check in Feature(name='verify-support', ...). Sorry I didn't suggest that yesterday.

[mstorsjo]

Remove the superfluous intermediate function altogether

[mstorsjo]

Extra conclusion (I'll amend it to the commit message):

By reimplementing this based on hasCompileFlag(), it also implicitly fixes two other issues: Previously, _supportsVerify called subprocess.call() directly, which can interpret command line quoting differently than lit.TestRunner.

(In particular, TestRunner handles arguments quoted by a single quote, while launching Windows processes with subprocess.call() only supports double quotes. This allows using shlex.quote(), which uses single quotes, everywhere - as all commands now go through TestRunner.)

Secondly, the old _supportsVerify method didn't include either %{flags) or %{compile_flags}.

libcxx/test/libcxx/selftest/fail.cpp/lit.local.cfg

-import libcxx.test.format
-
-# The tests in this directory need to know whether Clang-verify is supported
-# to work properly.
-if libcxx.test.format._supportsVerify(config):
-    config.available_features.add('verify-support')

libcxx/utils/libcxx/test/dsl.py

 #===----------------------------------------------------------------------===##
 #
 # 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
 #
 #===----------------------------------------------------------------------===##
 
 import os
 import pickle
 import pipes
 import platform
 import re
 import shutil
+import subprocess
 import tempfile
 
 import libcxx.test.format
 import lit
 import lit.LitConfig
 import lit.Test
 import lit.TestRunner
 import lit.util
def hasCompileFlag(config, flag):
   checking whether that succeeds.
   """
   with _makeConfigTest(config) as test:
     out, err, exitCode, timeoutInfo = _executeScriptInternal(test, [
       "%{{cxx}} -xc++ {} -Werror -fsyntax-only %{{flags}} %{{compile_flags}} {}".format(os.devnull, flag)
     ])
     return exitCode == 0
 
+@_memoizeExpensiveOperation(lambda c: (c.substitutions, c.environment))
+def supportsVerify(config):
+    """
+    Determine whether clang-verify is supported by the given configuration.
+
+    This is done by checking whether the %{cxx} substitution in that
+    configuration supports certain compiler flags.
+    """
+    return hasCompileFlag(config, '-Xclang -verify-ignore-unexpected')
+
 @_memoizeExpensiveOperation(lambda c, s: (c.substitutions, c.environment, s))
 def runScriptExitCode(config, script):
   """
   Runs the given script as a Lit test, and returns the exit code of the execution.
 
   The script must be a list of commands, each of which being something that
   could appear on the right-hand-side of a `RUN:` keyword.
   """
   with _makeConfigTest(config) as test:
     _, _, exitCode, _ = _executeScriptInternal(test, script)
     return exitCode
 

libcxx/utils/libcxx/test/features.py

DEFAULT_FEATURES = [
   Feature(name='-fsized-deallocation',          when=lambda cfg: hasCompileFlag(cfg, '-fsized-deallocation')),
   Feature(name='-faligned-allocation',          when=lambda cfg: hasCompileFlag(cfg, '-faligned-allocation')),
   Feature(name='fdelayed-template-parsing',     when=lambda cfg: hasCompileFlag(cfg, '-fdelayed-template-parsing')),
   Feature(name='libcpp-no-concepts',            when=lambda cfg: featureTestMacros(cfg).get('__cpp_concepts', 0) < 201907),
   Feature(name='libcpp-no-coroutines',          when=lambda cfg: featureTestMacros(cfg).get('__cpp_impl_coroutine', 0) < 201902),
   Feature(name='has-fobjc-arc',                 when=lambda cfg: hasCompileFlag(cfg, '-xobjective-c++ -fobjc-arc') and
                                                                  sys.platform.lower().strip() == 'darwin'), # TODO: this doesn't handle cross-compiling to Apple platforms.
   Feature(name='objective-c++',                 when=lambda cfg: hasCompileFlag(cfg, '-xobjective-c++ -fobjc-arc')),
+  Feature(name='verify-support',                when=lambda cfg: supportsVerify(cfg)),
 
   Feature(name='non-lockfree-atomics',
           when=lambda cfg: sourceBuilds(cfg, """
             #include <atomic>
             struct Large { int storage[100]; };
             std::atomic<Large> x;
             int main(int, char**) { (void)x.load(); return 0; }
           """)),

libcxx/utils/libcxx/test/format.py

 #===----------------------------------------------------------------------===##
 #
 # 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
 #
 #===----------------------------------------------------------------------===##
 
 import lit
 import lit.formats
 import os
 import pipes
 import re
 import shutil
-import subprocess
-
-def _supportsVerify(config):
-    """
-    Determine whether clang-verify is supported by the given configuration.
-
-    This is done by checking whether the %{cxx} substitution in that
-    configuration supports certain compiler flags.
-    """
-    command = "%{{cxx}} -xc++ {} -Werror -fsyntax-only -Xclang -verify-ignore-unexpected".format(os.devnull)
-    command = lit.TestRunner.applySubstitutions([command], config.substitutions,
-                                                recursion_limit=config.recursiveExpansionLimit)[0]
-    devNull = open(os.devnull, 'w')
-    result = subprocess.call(command, shell=True, stdout=devNull, stderr=devNull)
-    return result == 0
 
 def _getTempPaths(test):
     """
     Return the values to use for the %T and %t substitutions, respectively.
 
     The difference between this and Lit's default behavior is that we guarantee
     that %T is a path unique to the test being run.
     """
class CxxStandardLibraryTest(lit.formats.TestFormat):
 
     def _disableWithModules(self, test):
         with open(test.getSourcePath(), 'rb') as f:
             contents = f.read()
         return b'#define _LIBCPP_ASSERT' in contents
 
     def execute(self, test, litConfig):
         VERIFY_FLAGS = '-Xclang -verify -Xclang -verify-ignore-unexpected=note -ferror-limit=0'
-        supportsVerify = _supportsVerify(test.config)
+        supportsVerify = 'verify-support' in test.config.available_features
         filename = test.path_in_suite[-1]
 
         # TODO(ldionne): We currently disable tests that re-define _LIBCPP_ASSERT
         #                when we run with modules enabled. Instead, we should
         #                split the part that does a death test outside of the
         #                test, and only disable that part when modules are
         #                enabled.
         if 'modules-build' in test.config.available_features and self._disableWithModules(test):