[lldb] Fix detection of existing libcxx

[fdeazeve]

Summary

The CMake variable LLDB_HAS_LIBCXX is passed to
llvm_canonicalize_cmake_booleans, which transforms TRUE/FALSE into
'1'/'0'. It also transforms undefined variables to '0'.

In particular, this means that the configuration script for LLDB API's
test always has _some_ value for the has_libcxx configuration:

config.has_libcxx = '@LLDB_HAS_LIBCXX@'

When deciding whether a libcxx exist, the testing scripts would only
check for the existence of has_libcxx, but not for its value. In other
words, because if ('0') is true in python we always think there is a
libcxx.

This was caught once D132940 was merged and most tests started to use
libcxx by default if has_libcxx is true. Prior to that, no failures
were seen because only tests are marked with
@add_test_categories(["libc++"]) would require a libcxx, and these
would be filtered out on builds without the libcxx target. Furthermore,
the MacOS bots always build libcxx.

We fix this by making has_libcxx a boolean (instead of a string) and
by checking its value in the test configuration.

[fdeazeve]

Reworded commit message

[JDevlieghere]

lldb/test/API/lit.cfg.py

94

is_enabled maybe?

173–178

Are there other places where this helper would be useful? If not I think we can check the value directly. With the helper we lose a little bit of signal (if it returns false, was it not configured or set to false?). I don't think it's a big deal, but those two things combined make me favor inlining the check.

[fdeazeve]

lldb/test/API/lit.cfg.py

173–178

I agree that inlining is better here, but note that config.has_libcxx doesn't work, this is still going to return True: config.has_libcxx is a string, not a boolean, so anything != None -- in particular '0' -- will evaluate to true.

There are two ways to inline this:

1.is_configured('has_libcxx') and config.has_libcxx == '1'
2.is_configured('has_libcxx') == '1'

Actually, since has_libcxx comes from a canonicalized variable (in the sense described in the commit message), we could also just do:

3.config.has_libcxx == '1'

What do you think?

Related: in a separate patch I think we should change the implementation of is_configured to compare against None, so that it returns a boolean.

[fdeazeve]

Addressed review comment

[JDevlieghere]

lldb/test/API/lit.cfg.py

173–178

Aha, if has_libcxx is a string then that's a bug. It should be 0 or 1 like config.shared_libs and config.lldb_enable_python. Seems like someone (me) unintentionally added quotes.

This should fix that:

-config.has_libcxx = '@LLDB_HAS_LIBCXX@'
+config.has_libcxx = @LLDB_HAS_LIBCXX@

[fdeazeve]

Addressed review comment

[JDevlieghere]

LGTM!

lldb/test/API/lit.cfg.py

 
 
 def is_configured(attr):
   """Return the configuration attribute if it exists and None otherwise.
 
   This allows us to check if the attribute exists before trying to access it."""
   return getattr(config, attr, None)
 
 
 def delete_module_cache(path):
   """Clean the module caches in the test build directory.
 
   This is necessary in an incremental build whenever clang changes underneath,
   so doing it once per lit.py invocation is close enough. """
   if os.path.isdir(path):
     lit_config.note("Deleting module cache at %s." % path)
     shutil.rmtree(path)

 # Include path may be needed to locate just-built libcxx.
 if is_configured('llvm_include_dir'):
   dotest_cmd += ['--env', 'LLVM_INCLUDE_DIR=' + config.llvm_include_dir]
 
 # This path may be needed to locate required llvm tools
 if is_configured('llvm_tools_dir'):
   dotest_cmd += ['--env', 'LLVM_TOOLS_DIR=' + config.llvm_tools_dir]
 
 # If we have a just-built libcxx, prefer it over the system one.
-if is_configured('has_libcxx') and platform.system() != 'Windows':
+if is_configured('has_libcxx') and config.has_libcxx:
+  if platform.system() != 'Windows':
     if is_configured('llvm_include_dir') and is_configured('llvm_libs_dir'):
       dotest_cmd += ['--libcxx-include-dir', os.path.join(config.llvm_include_dir, 'c++', 'v1')]
       dotest_cmd += ['--libcxx-library-dir', config.llvm_libs_dir]
 
 # Forward ASan-specific environment variables to tests, as a test may load an
 # ASan-ified dylib.
 for env_var in ('ASAN_OPTIONS', 'DYLD_INSERT_LIBRARIES'):
   if env_var in config.environment:
     dotest_cmd += ['--inferior-env', env_var + '=' + config.environment[env_var]]
 
 if is_configured('test_arch'):

lldb/test/API/lit.site.cfg.py.in

 config.dotest_user_args_str = lit_config.substitute("@LLDB_TEST_USER_ARGS@")
 config.lldb_enable_python = @LLDB_ENABLE_PYTHON@
 config.dotest_lit_args_str = None
 config.enabled_plugins = []
 config.lldb_executable = lit_config.substitute('@LLDB_TEST_EXECUTABLE@')
 config.test_arch = '@LLDB_TEST_ARCH@'
 config.test_compiler = lit_config.substitute('@LLDB_TEST_COMPILER@')
 config.dsymutil = lit_config.substitute('@LLDB_TEST_DSYMUTIL@')
-config.has_libcxx = '@LLDB_HAS_LIBCXX@'
+config.has_libcxx = @LLDB_HAS_LIBCXX@
 # The API tests use their own module caches.
 config.lldb_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_LLDB@", "lldb-api")
 config.clang_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_CLANG@", "lldb-api")
 
 # Plugins
 lldb_build_intel_pt = '@LLDB_BUILD_INTEL_PT@'
 if lldb_build_intel_pt == '1':
     config.enabled_plugins.append('intel-pt')