[libcxx] Refactoring target_info.py used by lit tests

[bcraig]

Summary

This patch makes it easier to support running the lit tests for new and
unusual platforms. It will break existing users that set LIBCXX_TARGET_INFO
to anything other than the default. I think this is fine, because the old
LIBCXX_TARGET_INFO wasn't terribly useful.

The old way of supporting the different test platforms was to have conditional
code scattered throughout config.py. New platforms would need to add
conditionals there. Alternatively, the new platform could set
no_default_flags to true, and reconstitue almost the entire compile and link
line, including things that don't vary across platforms.

The new way of supporting new platforms is to create a new target info class,
and have make_target_info return an instance of it. For platforms supported
in-tree, that will be done by modifying make_target_info. For out-of-tree
platforms, users can set LIBCXX_TARGET_INFO at cmake configure time.

The target info sub-classes can provide fine-grained information back to
config.py. The hooks that will most commonly be provided will be
add_cxx_compile_flags and add_cxx_link_flags. These hooks can provide the
platform specific flags, while letting config.py handle all the invariant
flags.

Target info hooks were added for each area that the existing config.py had
platform specific behavior. config.py is now mostly free of platform specific
conditionals.

This patch was tested on Linux x86_64. I both targeted Linux x86_64, and an
out-of-tree platform with a custom target_info. In both cases I was able to
run libcxx and libcxxabi tests. I do not have access to FreeBSD, Darwin, or
Windows machines that are set up for lit testing.

[jroelofs]

This is awesome!

LGTM with one nit: add a newline between each function definition, and two between each class. It's a little crowded without that.

[jroelofs]

This patch makes it easier to support running the lit tests for new and unusual platforms. It will break existing users that set LIBCXX_TARGET_INFO to anything other than the default. I think this is fine, because the old LIBCXX_TARGET_INFO wasn't terribly useful.

I think @danalbert and I are the only users of out-of-tree versions of that.

[bcraig]

Committed as r256588.
Thanks for the quick review!

[EricWF]

This is NOT correct and it's breaking bots. I don't want to revert it because I really like the change but it needs to be fixed quickly.

@bcraig You removed the logic that tests if a system provides a given locale. We use this information to enable/disable tests. Please add it back.
I put an inline comment next to the original logic.

Example Breakage: http://lab.llvm.org:8011/builders/libcxx-libcxxabi-arm-linux/builds/780

test/libcxx/test/config.py

302

This test is important!

[EricWF]

I re-added the feature myself in r256621.

[bcraig]

Apologies for the swarm of breakages this caused. Thanks for the late night (for me at least) locale fix EricWF!

test/libcxx/test/config.py

-import importlib
 import locale
 import os
 import platform

 
 from libcxx.test.format import LibcxxTestFormat
 from libcxx.compiler import CXXCompiler
+from libcxx.test.target_info import make_target_info
 from libcxx.test.executor import *
 from libcxx.test.tracing import *
 

         ld_fn(config, site_cfg)
         lit_config.load_config = ld_fn
 
-def getSysrootFlagsOnDarwin(config, lit_config):
-    # On Darwin, support relocatable SDKs by providing Clang with a
-    # default system root path.
-    if 'darwin' in config.target_triple:
-        try:
-            out = lit.util.capture(['xcrun', '--show-sdk-path']).strip()
-            res = 0
-        except OSError:
-            res = -1
-        if res == 0 and out:
-            sdk_path = out
-            lit_config.note('using SDKROOT: %r' % sdk_path)
-            return ["-isysroot", sdk_path]
-    return []
-
-
 class Configuration(object):
     # pylint: disable=redefined-outer-name
     def __init__(self, lit_config, config):

         self.executor = te
 
     def configure_target_info(self):
-        default = "libcxx.test.target_info.LocalTI"
-        info_str = self.get_lit_conf('target_info', default)
-        mod_path, _, info = info_str.rpartition('.')
-        mod = importlib.import_module(mod_path)
-        self.target_info = getattr(mod, info)()
-        if info_str != default:
-            self.lit_config.note("inferred target_info as: %r" % info_str)
+        self.target_info = make_target_info(self)
 
     def configure_cxx(self):
         # Gather various compiler parameters.

     def configure_execute_external(self):
         # Choose between lit's internal shell pipeline runner and a real shell.
         # If LIT_USE_INTERNAL_SHELL is in the environment, we use that as the
-        # default value. Otherwise we default to internal on Windows and
-        # external elsewhere, as bash on Windows is usually very slow.
+        # default value. Otherwise we ask the target_info.
         use_lit_shell_default = os.environ.get('LIT_USE_INTERNAL_SHELL')
         if use_lit_shell_default is not None:
             use_lit_shell_default = use_lit_shell_default != '0'
         else:
-            use_lit_shell_default = sys.platform == 'win32'
+            use_lit_shell_default = self.target_info.use_lit_shell_default()
         # Check for the command line parameter using the default value if it is
         # not present.
         use_lit_shell = self.get_lit_bool('use_lit_shell',

         if additional_features:
             for f in additional_features.split(','):
                 self.config.available_features.add(f.strip())
+        self.target_info.add_locale_features(self.config.available_features)
 
-        # Figure out which of the required locales we support
-        locales = {
-            'Darwin': {
-                'en_US.UTF-8': 'en_US.UTF-8',
-                'cs_CZ.ISO8859-2': 'cs_CZ.ISO8859-2',
-                'fr_FR.UTF-8': 'fr_FR.UTF-8',
-                'fr_CA.ISO8859-1': 'fr_CA.ISO8859-1',
-                'ru_RU.UTF-8': 'ru_RU.UTF-8',
-                'zh_CN.UTF-8': 'zh_CN.UTF-8',
-            },
-            'FreeBSD': {
-                'en_US.UTF-8': 'en_US.UTF-8',
-                'cs_CZ.ISO8859-2': 'cs_CZ.ISO8859-2',
-                'fr_FR.UTF-8': 'fr_FR.UTF-8',
-                'fr_CA.ISO8859-1': 'fr_CA.ISO8859-1',
-                'ru_RU.UTF-8': 'ru_RU.UTF-8',
-                'zh_CN.UTF-8': 'zh_CN.UTF-8',
-            },
-            'Linux': {
-                'en_US.UTF-8': 'en_US.UTF-8',
-                'cs_CZ.ISO8859-2': 'cs_CZ.ISO-8859-2',
-                'fr_FR.UTF-8': 'fr_FR.UTF-8',
-                'fr_CA.ISO8859-1': 'fr_CA.ISO-8859-1',
-                'ru_RU.UTF-8': 'ru_RU.UTF-8',
-                'zh_CN.UTF-8': 'zh_CN.UTF-8',
-            },
-            'Windows': {
-                'en_US.UTF-8': 'English_United States.1252',
-                'cs_CZ.ISO8859-2': 'Czech_Czech Republic.1250',
-                'fr_FR.UTF-8': 'French_France.1252',
-                'fr_CA.ISO8859-1': 'French_Canada.1252',
-                'ru_RU.UTF-8': 'Russian_Russia.1251',
-                'zh_CN.UTF-8': 'Chinese_China.936',
-            },
-        }
-
-        target_system = self.target_info.system()
         target_platform = self.target_info.platform()
 
-        if target_system in locales:
-            default_locale = locale.setlocale(locale.LC_ALL)
-            for feature, loc in locales[target_system].items():
-                try:
-                    locale.setlocale(locale.LC_ALL, loc)
-                    self.config.available_features.add(
-                        'locale.{0}'.format(feature))
-                except locale.Error:
-                    self.lit_config.warning('The locale {0} is not supported by '
-                                            'your platform. Some tests will be '
-                                            'unsupported.'.format(loc))
-            locale.setlocale(locale.LC_ALL, default_locale)
-        else:
-            # Warn that the user doesn't get any free XFAILs for locale issues
-            self.lit_config.warning("No locales entry for target_system: %s" %
-                                    target_system)
-
         # Write an "available feature" that combines the triple when
         # use_system_cxx_lib is enabled. This is so that we can easily write
         # XFAIL markers for tests that are known to fail with versions of

         # Insert the platform name into the available features as a lower case.
         self.config.available_features.add(target_platform)
 
-        # Some linux distributions have different locale data than others.
-        # Insert the distributions name and name-version into the available
-        # features to allow tests to XFAIL on them.
-        if target_platform == 'linux':
-            name = self.target_info.platform_name()
-            ver = self.target_info.platform_ver()
-            if name:
-                self.config.available_features.add(name)
-            if name and ver:
-                self.config.available_features.add('%s-%s' % (name, ver))
-
         # Simulator testing can take a really long time for some of these tests
         # so add a feature check so we can REQUIRES: long_tests in them
         self.long_tests = self.get_lit_bool('long_tests')

         # Configure extra flags
         compile_flags_str = self.get_lit_conf('compile_flags', '')
         self.cxx.compile_flags += shlex.split(compile_flags_str)
-        sysroot_flags = getSysrootFlagsOnDarwin(self.config, self.lit_config)
-        self.cxx.compile_flags.extend(sysroot_flags)
 
     def configure_default_compile_flags(self):
         # Try and get the std version from the command line. Fall back to

         # Configure include paths
         self.cxx.compile_flags += ['-nostdinc++']
         self.configure_compile_flags_header_includes()
-        if self.target_info.platform() == 'linux':
-            self.cxx.compile_flags += ['-D__STDC_FORMAT_MACROS',
-                                       '-D__STDC_LIMIT_MACROS',
-                                       '-D__STDC_CONSTANT_MACROS']
+        self.target_info.add_cxx_compile_flags(self.cxx.compile_flags)
         # Configure feature flags.
         self.configure_compile_flags_exceptions()
         self.configure_compile_flags_rtti()

         elif cxx_abi == 'libsupc++':
             self.cxx.link_flags += ['-lsupc++']
         elif cxx_abi == 'libcxxabi':
-            # Don't link libc++abi explicitly on OS X because the symbols
-            # should be available in libc++ directly.
-            if self.target_info.platform() != 'darwin':
+            if self.target_info.allow_cxxabi_link():
                 self.cxx.link_flags += ['-lc++abi']
         elif cxx_abi == 'libcxxrt':
             self.cxx.link_flags += ['-lcxxrt']

                 'C++ ABI setting %s unsupported for tests' % cxx_abi)
 
     def configure_extra_library_flags(self):
-        enable_threads = ('libcpp-has-no-threads' not in
-                          self.config.available_features)
-        llvm_unwinder = self.get_lit_bool('llvm_unwinder', False)
-        target_platform = self.target_info.platform()
-        if target_platform == 'darwin':
-            self.cxx.link_flags += ['-lSystem']
-        elif target_platform == 'linux':
-            self.cxx.link_flags += ['-lm']
-            if not llvm_unwinder:
-                self.cxx.link_flags += ['-lgcc_s', '-lgcc']
-            if enable_threads:
-                self.cxx.link_flags += ['-lpthread']
-            self.cxx.link_flags += ['-lc']
-            if llvm_unwinder:
-                self.cxx.link_flags += ['-lunwind', '-ldl']
-            else:
-                self.cxx.link_flags += ['-lgcc_s', '-lgcc']
-        elif target_platform.startswith('freebsd'):
-            self.cxx.link_flags += ['-lc', '-lm', '-lpthread', '-lgcc_s', '-lcxxrt']
-        else:
-            self.lit_config.fatal("unrecognized system: %r" % target_platform)
+        self.target_info.add_cxx_link_flags(self.cxx.link_flags)
 
     def configure_color_diagnostics(self):
         use_color = self.get_lit_conf('color_diagnostics')

     def configure_sanitizer(self):
         san = self.get_lit_conf('use_sanitizer', '').strip()
         if san:
+            self.target_info.add_sanitizer_features(san, self.config.available_features)
             # Search for llvm-symbolizer along the compiler path first
             # and then along the PATH env variable.
             symbolizer_search_paths = os.environ.get('PATH', '')

                                              symbolizer_search_paths)
             # Setup the sanitizer compile flags
             self.cxx.flags += ['-g', '-fno-omit-frame-pointer']
-            if self.target_info.platform() == 'linux':
-                # The libraries and their order are taken from the
-                # linkSanitizerRuntimeDeps function in
-                # clang/lib/Driver/Tools.cpp
-                self.cxx.link_flags += ['-lpthread', '-lrt', '-lm', '-ldl']
             if san == 'Address':
                 self.cxx.flags += ['-fsanitize=address']
                 if llvm_symbolizer is not None:

                                    '-fno-sanitize-recover']
                 self.cxx.compile_flags += ['-O3']
                 self.config.available_features.add('ubsan')
-                if self.target_info.platform() == 'darwin':
-                    self.config.available_features.add('sanitizer-new-delete')
             elif san == 'Thread':
                 self.cxx.flags += ['-fsanitize=thread']
                 self.config.available_features.add('tsan')

                 "inferred target_triple as: %r" % self.config.target_triple)
 
     def configure_env(self):
-        if self.target_info.platform() == 'darwin':
-            library_paths = []
-            # Configure the library path for libc++
-            libcxx_library = self.get_lit_conf('libcxx_library')
-            if self.use_system_cxx_lib:
-                pass
-            elif libcxx_library:
-                library_paths += [os.path.dirname(libcxx_library)]
-            elif self.cxx_library_root:
-                library_paths += [self.cxx_library_root]
-            # Configure the abi library path
-            if self.abi_library_root:
-                library_paths += [self.abi_library_root]
-            if library_paths:
-                self.env['DYLD_LIBRARY_PATH'] = ':'.join(library_paths)
+        self.target_info.configure_env(self.env)

test/libcxx/test/target_info.py

+import importlib
 import locale
+import os
 import platform
 import sys
 
-class TargetInfo(object):
-    def platform(self):
-        raise NotImplementedError
-
-    def system(self):
-        raise NotImplementedError
-
-    def platform_ver(self):
-        raise NotImplementedError
-
-    def platform_name(self):
-        raise NotImplementedError
-
-    def supports_locale(self, loc):
-        raise NotImplementedError
-
-
-class LocalTI(TargetInfo):
+class DefaultTargetInfo(object):
+    def __init__(self, full_config):
+        self.full_config = full_config
+    def platform(self):        return sys.platform.lower().strip()
+
+    def add_locale_features(self, features):
+        self.full_config.lit_config.warning(
+            "No locales entry for target_system: %s" % self.platform())
+
+    def add_cxx_compile_flags(self, flags): pass
+    def add_cxx_link_flags(self, flags): pass
+    def configure_env(self, env): pass
+    def allow_cxxabi_link(self): return True
+    def add_sanitizer_features(self, sanitizer_type, features): pass
+    def use_lit_shell_default(self): return False
+
+def add_common_locales(features):
+    locales = [
+        'en_US.UTF-8',
+        'cs_CZ.ISO8859-2',
+        'fr_FR.UTF-8',
+        'fr_CA.ISO8859-1',
+        'ru_RU.UTF-8',
+        'zh_CN.UTF-8',
+    ]
+    for loc in locales:
+        features.add('locale.{0}'.format(loc))
+
+class DarwinLocalTI(DefaultTargetInfo):
+    def __init__(self, full_config):
+        super(DarwinLocalTI, self).__init__(full_config)
+    def add_locale_features(self, features):
+        add_common_locales(features)
+    def add_cxx_compile_flags(self, flags):
+        try:
+            out = lit.util.capture(['xcrun', '--show-sdk-path']).strip()
+            res = 0
+        except OSError:
+            res = -1
+        if res == 0 and out:
+            sdk_path = out
+            self.full_config.lit_config.note('using SDKROOT: %r' % sdk_path)
+            flags += ["-isysroot", sdk_path]
+    def add_cxx_link_flags(self, flags):
+        flags += ['-lSystem']
+    def configure_env(self, env):
+        library_paths = []
+        # Configure the library path for libc++
+        libcxx_library = self.full_config.get_lit_conf('libcxx_library')
+        if self.full_config.use_system_cxx_lib:
+            pass
+        elif libcxx_library:
+            library_paths += [os.path.dirname(libcxx_library)]
+        elif self.full_config.cxx_library_root:
+            library_paths += [self.full_config.cxx_library_root]
+        # Configure the abi library path
+        if self.full_config.abi_library_root:
+            library_paths += [self.full_config.abi_library_root]
+        if library_paths:
+            env['DYLD_LIBRARY_PATH'] = ':'.join(library_paths)
+    def allow_cxxabi_link(self):
+        # Don't link libc++abi explicitly on OS X because the symbols
+        # should be available in libc++ directly.
+        return False
+    def add_sanitizer_features(self, sanitizer_type, features):
+        if san == 'Undefined':
+            features.add('sanitizer-new-delete')
+
+class FreeBSDLocalTI(DefaultTargetInfo):
+    def __init__(self, full_config):
+        super(FreeBSDLocalTI, self).__init__(full_config)
+    def add_locale_features(self, features):
+        add_common_locales(features)
+    def add_cxx_link_flags(self, flags):
+        flags += ['-lc', '-lm', '-lpthread', '-lgcc_s', '-lcxxrt']
+
+class LinuxLocalTI(DefaultTargetInfo):
+    def __init__(self, full_config):
+        super(LinuxLocalTI, self).__init__(full_config)
     def platform(self):
-        platform_name = sys.platform.lower().strip()
-        # Strip the '2' from linux2.
-        if platform_name.startswith('linux'):
-            platform_name = 'linux'
-        return platform_name
-
-    def system(self):
-        return platform.system()
+        return 'linux'
 
     def platform_name(self):
-        if self.platform() == 'linux':
-            name, _, _ = platform.linux_distribution()
-            name = name.lower().strip()
-            if name:
-                return name
-        return None
+        return name # Permitted to be None
 
     def platform_ver(self):
-        if self.platform() == 'linux':
-            _, ver, _ = platform.linux_distribution()
-            ver = ver.lower().strip()
-            if ver:
-                return ver
-        return None
+        return ver # Permitted to be None.
 
-    def supports_locale(self, loc):
-        try:
-            locale.setlocale(locale.LC_ALL, loc)
-            return True
-        except locale.Error:
-            return False
+    def add_locale_features(self, features):
+        add_common_locales(features)
+        # Some linux distributions have different locale data than others.
+        # Insert the distributions name and name-version into the available
+        # features to allow tests to XFAIL on them.
+        name = self.platform_name()
+        ver = self.platform_ver()
+        if name:
+            features.add(name)
+        if name and ver:
+            features.add('%s-%s' % (name, ver))
+
+    def add_cxx_compile_flags(self, flags):
+        flags += ['-D__STDC_FORMAT_MACROS',
+                  '-D__STDC_LIMIT_MACROS',
+                  '-D__STDC_CONSTANT_MACROS']
+    def add_cxx_link_flags(self, flags):
+        enable_threads = ('libcpp-has-no-threads' not in
+                          self.full_config.config.available_features)
+        llvm_unwinder = self.full_config.get_lit_bool('llvm_unwinder', False)
+        flags += ['-lm']
+        if not llvm_unwinder:
+            flags += ['-lgcc_s', '-lgcc']
+        if enable_threads:
+            flags += ['-lpthread']
+        flags += ['-lc']
+        if llvm_unwinder:
+            flags += ['-lunwind', '-ldl']
+        else:
+            flags += ['-lgcc_s', '-lgcc']
+        if self.full_config.get_lit_bool('use_sanitizer', False):
+            # The libraries and their order are taken from the
+            # linkSanitizerRuntimeDeps function in
+            # clang/lib/Driver/Tools.cpp
+            flags += ['-lpthread', '-lrt', '-lm', '-ldl']
+
+class WindowsLocalTI(DefaultTargetInfo):
+    def __init__(self, full_config):
+        super(WindowsLocalTI, self).__init__(full_config)
+    def add_locale_features(self, features):
+        add_common_locales(features)
+    def use_lit_shell_default(self):
+        # Default to the internal shell on Windows, as bash on Windows is
+        # usually very slow.
+
+def make_target_info(full_config):
+    default = "libcxx.test.target_info.LocalTI"
+    info_str = full_config.get_lit_conf('target_info', default)
+    if info_str != default:
+        mod_path, _, info = info_str.rpartition('.')
+        mod = importlib.import_module(mod_path)
+        target_info = getattr(mod, info)(full_config)
+        full_config.lit_config.note("inferred target_info as: %r" % info_str)
+        return target_info
+    target_system = platform.system()
+    if target_system == 'Darwin':  return DarwinLocalTI(full_config)
+    if target_system == 'FreeBSD': return FreeBSDLocalTI(full_config)
+    if target_system == 'Linux':   return LinuxLocalTI(full_config)
+    if target_system == 'Windows': return WindowsLocalTI(full_config)
+    return DefaultTargetInfo(full_config)