Skip to content

Commit

Permalink
[libcxx][lit] Fix incorrect lambda capture in hasLocale checks
Browse files Browse the repository at this point in the history
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 a default argument capture.

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.

Reviewed By: #libc, ldionne

Differential Revision: https://reviews.llvm.org/D84040
  • Loading branch information
arichardson committed Jul 23, 2020
1 parent 1162ffe commit 9020d28
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 9 deletions.
12 changes: 7 additions & 5 deletions libcxx/utils/libcxx/test/dsl.py
Expand Up @@ -52,13 +52,14 @@ def _executeScriptInternal(test, commands):
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):
Expand All @@ -82,7 +83,7 @@ def sourceBuilds(config, source):
_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.
Expand All @@ -91,7 +92,7 @@ def programOutput(config, program, args=[]):
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:
Expand Down Expand Up @@ -142,7 +143,8 @@ def hasLocale(config, locale):
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=''):
"""
Expand Down
8 changes: 4 additions & 4 deletions libcxx/utils/libcxx/test/features.py
Expand Up @@ -104,10 +104,10 @@
'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...
Expand Down

0 comments on commit 9020d28

Please sign in to comment.