This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by arichardson on Jul 17 2020, 9:34 AM.

Details

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.

Diff Detail

Event Timeline

arichardson created this revision.Jul 17 2020, 9:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2020, 9:34 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Jul 17 2020, 9:44 AM

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.

This revision now requires changes to proceed.Jul 17 2020, 9:44 AM
arichardson marked an inline comment as done.Jul 17 2020, 9:49 AM
arichardson added inline comments.
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 marked 3 inline comments as done.

Address review comments

ldionne requested changes to this revision.Jul 17 2020, 10:36 AM
ldionne added inline comments.
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.

This revision now requires changes to proceed.Jul 17 2020, 10:36 AM

Apply suggested simplification

ldionne accepted this revision.Jul 22 2020, 9:02 PM
This revision is now accepted and ready to land.Jul 22 2020, 9:02 PM
This revision was automatically updated to reflect the committed changes.
bjope added a subscriber: bjope.Jul 25 2020, 4:07 PM
bjope added inline comments.
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 added inline comments.Jul 25 2020, 4:38 PM
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 marked an inline comment as done.Jul 26 2020, 3:28 AM
arichardson added inline comments.
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