[libcxx][lit] Fix incorrect lambda capture in hasLocale checks

[arichardson]

Summary

The lambda being used to check whether locales are supported was always
passing the value of alts from the last loop iteration due to the way that
python lambda captures work. Fix this by using functools.partial instead.

To help debug future similar issues I also added a prefix to the config
test binary indicating which locale is being tested.
I originally found this issue when implementing a new executor that simply
collects test binaries in a given directory and was surprised to see many
additional executables other than the expected test binaries. I therefore
added the locale prefix to the test binaries and noticed that they were all
checking for cs_CZ.ISO8859-2.

[ldionne]

Nice catch! This is definitely the corner of Python that causes me the most trouble, I keep getting bitten by it.

libcxx/utils/libcxx/test/dsl.py

55

I would use something like testSlug (in reference to git repo slugs, etc) instead of test_name. It's not really the name of the test, it's just part of it.

146–147

I'd rather keep this checking for a single locale.

libcxx/utils/libcxx/test/features.py

109

I think you can also use:

lambda cfg, alts=alts: any(hasLocale(cfg, alt) for alt in alts)

I think it's more idiomatic in Python, and it seems easier to understand.

[arichardson]

libcxx/utils/libcxx/test/features.py

109

I find functools.partial slightly more obvious than adding a default argument. But I don't really mind either way. Will change to the default argument solution and keep any().

[arichardson]

Address review comments

[ldionne]

libcxx/utils/libcxx/test/dsl.py

61–62

I think just prefix=testPrefix is fine -- if testPrefix is None, then we won't use a prefix at all, which is the intended behavior. Also, we know it's some kind of configuration-related test because of the directory it lives in.

[arichardson]

Apply suggested simplification

[arichardson]

rebase

[bjope]

libcxx/utils/libcxx/test/dsl.py

55

I get errors like this after this patch:

llvm-lit: /repo/llvm-upstream/llvm/build-all-builtins/bin/../../utils/lit/lit/TestingConfig.py:101: fatal: unable to parse config file '/repo/llvm-upstream/llvm/build-all-builtins/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/libunwind/test/lit.site.cfg', traceback: Traceback (most recent call last):
  File "/repo/llvm-upstream/llvm/build-all-builtins/bin/../../utils/lit/lit/TestingConfig.py", line 88, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/repo/llvm-upstream/llvm/build-all-builtins/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/libunwind/test/lit.site.cfg", line 58, in <module>
    configuration.configure()
  File "/repo/llvm-upstream/libcxx/utils/libcxx/test/config.py", line 148, in configure
    self.lit_config
  File "/repo/llvm-upstream/libcxx/utils/libcxx/test/newconfig.py", line 20, in configure
    feature = param.getFeature(config, lit_config.params)
  File "/repo/llvm-upstream/libcxx/utils/libcxx/test/dsl.py", line 387, in getFeature
    value = self._parse(param) if param is not None else getDefault()
  File "/repo/llvm-upstream/libcxx/utils/libcxx/test/dsl.py", line 386, in <lambda>
    getDefault = lambda: self._default(config) if callable(self._default) else self._default
  File "/repo/llvm-upstream/libcxx/utils/libcxx/test/params.py", line 17, in <lambda>
    default=lambda cfg: next(s for s in reversed(_allStandards) if hasCompileFlag(cfg, '-std='+s)),
  File "/repo/llvm-upstream/libcxx/utils/libcxx/test/params.py", line 17, in <genexpr>
    default=lambda cfg: next(s for s in reversed(_allStandards) if hasCompileFlag(cfg, '-std='+s)),
  File "/repo/llvm-upstream/libcxx/utils/libcxx/test/dsl.py", line 125, in hasCompileFlag
    with _makeConfigTest(config) as test:
  File "/repo/llvm-upstream/libcxx/utils/libcxx/test/dsl.py", line 62, in _makeConfigTest
    prefix=testPrefix)
  File "/usr/lib64/python2.7/tempfile.py", line 458, in NamedTemporaryFile
    (fd, name) = _mkstemp_inner(dir, prefix, suffix, flags)
  File "/usr/lib64/python2.7/tempfile.py", line 237, in _mkstemp_inner
    file = _os.path.join(dir, pre + name + suf)
TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'

}

So using prefix=None does not work, at least not on my system. But if I change this to testPrefix='' it seems to work better.

[bjope]

libcxx/utils/libcxx/test/dsl.py

55

Could this perhaps be related to using /usr/lib64/python2.7/tempfile.py.

No idea really why it picks tempfile.py from that place. I had expect it to use python 3.5.0, as earlier in the log I've got:

-- Found Python3: /app/vbuild/RHEL6-x86_64/python/3.5.0/bin/python3.5 (found version "3.5.0") found components: Interpreter

as well as

cd /repo/llvm-upstream/clang/bindings/python && /app/cmake/3.16.4/bin/cmake -E env CLANG_LIBRARY_PATH=/repo/llvm-upstream/llvm/build-all-builtins/lib /app/vbuild/RHEL7-x86_64/python/3.5.0/bin/python3.5 -m unittest discover

[arichardson]

libcxx/utils/libcxx/test/dsl.py

55

Maybe this is happening because llvm-lit defaults to using "python" which is usually python2. (https://github.com/llvm/llvm-project/blob/9b19400004dfee9d07a90aa11d448bade9ee71a2/llvm/utils/llvm-lit/llvm-lit.in#L1) rather than the binary that was detected at CMake time.

It appears to work if I use python3 ./bin/llvm-lit projects/libcxx/test but I get the same error with ./bin/llvm-lit projects/libcxx/test

libcxx/utils/libcxx/test/dsl.py

def _executeScriptInternal(test, commands):
   for d in (execDir, os.path.dirname(tmpBase)):
     if not os.path.exists(d):
       os.makedirs(d)
   res = lit.TestRunner.executeScriptInternal(test, litConfig, tmpBase, parsedCommands, execDir)
   if isinstance(res, lit.Test.Result):
     res = ('', '', 127, None)
   return res
 
-def _makeConfigTest(config):
+def _makeConfigTest(config, testPrefix=None):
   sourceRoot = os.path.join(config.test_exec_root, '__config_src__')
   execRoot = os.path.join(config.test_exec_root, '__config_exec__')
   suite = lit.Test.TestSuite('__config__', sourceRoot, execRoot, config)
   if not os.path.exists(sourceRoot):
     os.makedirs(sourceRoot)
-  tmp = tempfile.NamedTemporaryFile(dir=sourceRoot, delete=False, suffix='.cpp')
+  tmp = tempfile.NamedTemporaryFile(dir=sourceRoot, delete=False, suffix='.cpp',
+                                    prefix=testPrefix)
   tmp.close()
   pathInSuite = [os.path.relpath(tmp.name, sourceRoot)]
   class TestWrapper(lit.Test.Test):
     def __enter__(self):       return self
     def __exit__(self, *args): os.remove(tmp.name)
   return TestWrapper(suite, pathInSuite, config)
 
 def sourceBuilds(config, source):
   """
   Return whether the program in the given string builds successfully.
 
   This is done by compiling and linking a program that consists of the given
   source with the %{cxx} substitution, and seeing whether that succeeds.
   """
   with _makeConfigTest(config) as test:
     with open(test.getSourcePath(), 'w') as sourceFile:
       sourceFile.write(source)
     out, err, exitCode, timeoutInfo = _executeScriptInternal(test, [
       "%{cxx} %s %{flags} %{compile_flags} %{link_flags} -o %t.exe"
     ])
     _executeScriptInternal(test, ['rm %t.exe'])
     return exitCode == 0
 
-def programOutput(config, program, args=[]):
+def programOutput(config, program, args=[], testPrefix=None):
   """
   Compiles a program for the test target, run it on the test target and return
   the output.
 
   If the program fails to compile or run, None is returned instead. Note that
   execution of the program is done through the %{exec} substitution, which means
   that the program may be run on a remote host depending on what %{exec} does.
   """
-  with _makeConfigTest(config) as test:
+  with _makeConfigTest(config, testPrefix=testPrefix) as test:
     with open(test.getSourcePath(), 'w') as source:
       source.write(program)
     try:
       _, _, exitCode, _ = _executeScriptInternal(test, [
         "%{cxx} %s %{flags} %{compile_flags} %{link_flags} -o %t.exe",
       ])
       if exitCode != 0:
         return None
def hasLocale(config, locale):
   """
   program = """
     #include <locale.h>
     int main(int, char** argv) {
       if (::setlocale(LC_ALL, argv[1]) != NULL) return 0;
       else                                      return 1;
     }
   """
-  return programOutput(config, program, args=[pipes.quote(locale)]) != None
+  return programOutput(config, program, args=[pipes.quote(locale)],
+                       testPrefix="check_locale_" + locale) is not None
 
 def compilerMacros(config, flags=''):
   """
   Return a dictionary of predefined compiler macros.
 
   The keys are strings representing macros, and the values are strings
   representing what each macro is defined to.
 

libcxx/utils/libcxx/test/features.py

locales = {
   'en_US.UTF-8':     ['en_US.UTF-8', 'en_US.utf8', 'English_United States.1252'],
   'fr_FR.UTF-8':     ['fr_FR.UTF-8', 'fr_FR.utf8', 'French_France.1252'],
   'ru_RU.UTF-8':     ['ru_RU.UTF-8', 'ru_RU.utf8', 'Russian_Russia.1251'],
   'zh_CN.UTF-8':     ['zh_CN.UTF-8', 'zh_CN.utf8', 'Chinese_China.936'],
   'fr_CA.ISO8859-1': ['fr_CA.ISO8859-1', 'French_Canada.1252'],
   'cs_CZ.ISO8859-2': ['cs_CZ.ISO8859-2', 'Czech_Czech Republic.1250']
 }
 for locale, alts in locales.items():
-  DEFAULT_FEATURES += [
-    Feature(name='locale.{}'.format(locale),
-            when=lambda cfg: any(hasLocale(cfg, alt) for alt in alts))
-  ]
+  # Note: Using alts directly in the lambda body here will bind it to the value at the
+  # end of the loop. Assigning it to a default argument works around this issue.
+  DEFAULT_FEATURES.append(Feature(name='locale.{}'.format(locale),
+                                  when=lambda cfg, alts=alts: any(hasLocale(cfg, alt) for alt in alts)))
 
 
 # Add features representing the platform name: darwin, linux, windows, etc...
 DEFAULT_FEATURES += [
   Feature(name='darwin', when=lambda cfg: '__APPLE__' in compilerMacros(cfg)),
   Feature(name='windows', when=lambda cfg: '_WIN32' in compilerMacros(cfg)),
   Feature(name='linux', when=lambda cfg: '__linux__' in compilerMacros(cfg)),
   Feature(name='netbsd', when=lambda cfg: '__NetBSD__' in compilerMacros(cfg))
 ]