[lit] Add a shared llvm configuration library, and use it in llvm

[zturner]

Summary

Building off of D37756, the next step is to take all the common, reusable stuff that is copied across the various LLVM projects, and centralize it.

I've identified two key areas that I'm tackling with this patch:

1. Stuff that can be done automatically for every single LLVM project. For example, almost every test starts with 6-7 lines of copied code to add GnuWin32 to the path on Windows. Those that don't, probably should. Similarly, most of the projects add some common "features" that can be checked by tests. All of these features now get added automatically.

Caveat: In some cases this is introducing *new* features to projects that didn't currently have these features before. This is a change, but I think it's worth doing, as I only chose the most common, generic not-project-specific ones for this purpose.

2. Adding some convenience functions that reduce the amount of code required to do various common things. For example, a lot of projects need to run llvm-config in various ways in order to determine if certain features are enabled. This involves duplicating the same 10 lines of code multiple times. Now, by using this helper function, it's 1 line.

At a high level, the way this works is to add a new llvm-specific submodule to lit. Then, you have your site config put the magic configure-like replacement and configure automatically imports and initializes this module for you. Then, in your main config, an object magically appears that you can just use.

With this patch I'm only introducing this into LLVM's lit.cfg. The idea is to get feedback on the high level ideas. I can go through and cleanup the remainder of the projects later.

[dlj]

Mostly just nit-picky Python comments... the helper class seems to reduce the overhead quite a bit.

llvm/utils/lit/lit/llvm/config.py

6

It might make sense to move this into the LLVMConfig class, so you don't have to keep passing lit_config.

It also looks like this is always called with attributes on config, so you could roll in the getattr()s here as well.

21

Nit: this should probably inherit from object for the benefit of py2.

class LLVMConfig(object):

39

(Minor suggestion) It might make sense to decide the platform default earlier, then pass it as the default to get(). That should let you collapse both of the next blocks:

shell_default = ('0' if sys.platform not in ['win32'] else '')
if os.environ.get('LIT_USE_INTERNAL_SHELL', shell_default) == '0':
  self.use_lit_shell = False
else:
  self.use_lit_shell = True
  config.available_features.add('shell')

(Not sure if that's much clearer or not, especially with the comments.)

59

If this value is set, will it be in CMake's internal semicolon-separated format? It might be better to split the value into a list if you can, instead of relying on substring searching.

60

If you pass an iterable default to getattr() instead of None, you should be able to just omit this check.

61

You could cut down on the duplication here with a lambda:

_check = lambda name, feature: ('' if name in use_sanitizer else 'not_') + feature
config.available_features.add(_check('Address', 'asan'))

75

It looks like this should be have_zlib... but in any case, you could reduce duplication (and the potential for typops) by building the string directly with a ternary:

config.available_features.add(('' if getattr(config, 'have_zlib', None) else 'no') + 'zlib')

91

(Side note: if you move _is_true_like into the class, you can probably just move the logic of this function into the if statement.)

109

It would probably make more sense for this to be a default-valued argument:

def with_environment(..., append_path=False)

I don't see any reason that you would want to reprocess or pass along other kwargs, so the default valued option is probably clearer.

114

Minor nit: maybe call the variable "prepend_path"?

118

Are you sure it's safe normalize the existing values?

It seems like it should be OK, since you won't be re-normalizing all the time. Doing normalized comparisons would be a bit more complex, too.

118

I think you want this to be:

os.path.pathsep.split(current_value)

119

You can narrow the scope here:

try:
  items.remove(value)
except ValueError:
  pass
items = [value] + items

128

Minor nit: 'vars' is a Python builtin name, maybe call it 'variables' instead?

129

I think you want:

if isinstance(vars, str):

144

Should this use self.config.fatal?

146

I think you might want to use this pattern instead:

output, _ = llvm_config_cmd.communicate()
if re.search('ON', output.decode('ascii')):
  # ...

That will handle the blocking semantics correctly on the subprocess's file handles. It also eliminates the need for the wait() below.

[zturner]

llvm/utils/lit/lit/llvm/config.py

6

Hmm, even better might be perhaps to move this into lit.util. There's nothing llvm specific about this

118

Yea, the problem is that I really need to compare whether or not the paths are "equivalent". This means handling case-sensitivity and normalizing slashes, otherwise the string compare won't work. If someone puts D:/src/llvm into a variable, and then I add D:\src\llvm, they should match. Luckily this code isn't really in a hot path, so performance doesn't matter, and I can't think of a reason why it would be unsafe. On non-Windows platforms, norm is a no-op

I think you want this to be: os.path.pathsep.split(current_value)

Is there a difference?

146

Good suggestion. It's almost as if centralizing all this copied code into a single place has tangible value :D

[zturner]

Updated from suggestions

[zturner]

A duplicated copy of the new files got added somehow in the last patch. Removed them.

llvm/cmake/modules/AddLLVM.cmake

string(CONCAT LIT_SITE_CFG_IN_HEADER "${LIT_SITE_CFG_IN_HEADER}\n\n"
       "target_env = \"${LLVM_TARGET_TRIPLE_ENV}\"\n"
       "config.target_triple = config.environment[target_env] = os.environ.get(target_env, \"${TARGET_TRIPLE}\")\n"
       )
 
     # This is expanded to; config.target_triple = ""+config.target_triple+""
     set(TARGET_TRIPLE "\"+config.target_triple+\"")
   endif()
 
+  string(CONCAT LIT_SITE_CFG_IN_FOOTER
+     "import lit.llvm\n"
+     "lit.llvm.initialize(lit_config, config)\n")
+
   configure_file(${input} ${output} @ONLY)
   get_filename_component(INPUT_DIR ${input} DIRECTORY)
   if (EXISTS "${INPUT_DIR}/lit.cfg")
     set(PYTHON_STATEMENT "map_config('${INPUT_DIR}/lit.cfg', '${output}')")
     get_property(LLVM_LIT_CONFIG_MAP GLOBAL PROPERTY LLVM_LIT_CONFIG_MAP)
     set(LLVM_LIT_CONFIG_MAP "${LLVM_LIT_CONFIG_MAP}\n${PYTHON_STATEMENT}")
     set_property(GLOBAL PROPERTY LLVM_LIT_CONFIG_MAP ${LLVM_LIT_CONFIG_MAP})
   endif()

llvm/test/lit.cfg

 # -*- Python -*-
 
 # Configuration file for the 'lit' test runner.
 
 import os
 import sys
 import re
 import platform
 import subprocess
 
 import lit.util
 import lit.formats
+from lit.llvm import llvm_config
 
 # name: The name of this test suite.
 config.name = 'LLVM'
 
-# Tweak PATH for Win32 to decide to use bash.exe or not.
-if sys.platform in ['win32']:
-    # Seek sane tools in directories and set to $PATH.
-    path = getattr(config, 'lit_tools_dir', None)
-    path = lit_config.getToolsPath(path,
-                                   config.environment['PATH'],
-                                   ['cmp.exe', 'grep.exe', 'sed.exe'])
-    if path is not None:
-        path = os.path.pathsep.join((path,
-                                     config.environment['PATH']))
-        config.environment['PATH'] = path
-
-# 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 an override.
-use_lit_shell = os.environ.get("LIT_USE_INTERNAL_SHELL")
-if use_lit_shell:
-    # 0 is external, "" is default, and everything else is internal.
-    execute_external = (use_lit_shell == "0")
-else:
-    # Otherwise we default to internal on Windows and external elsewhere, as
-    # bash on Windows is usually very slow.
-    execute_external = (not sys.platform in ['win32'])
-
 # testFormat: The test format to use to interpret tests.
-config.test_format = lit.formats.ShTest(execute_external)
+config.test_format = lit.formats.ShTest(not llvm_config.use_lit_shell)
 
 # suffixes: A list of file extensions to treat as test files. This is overriden
 # by individual lit.local.cfg files in the test subdirectories.
 config.suffixes = ['.ll', '.c', '.cxx', '.test', '.txt', '.s', '.mir']
 
 # excludes: A list of directories to exclude from the testsuite. The 'Inputs'
 # subdirectories contain auxiliary inputs for various tests in their parent
 # directories.
 config.excludes = ['Inputs', 'CMakeLists.txt', 'README.txt', 'LICENSE.txt']
 
 # test_source_root: The root path where tests are located.
 config.test_source_root = os.path.dirname(__file__)
 
 # test_exec_root: The root path where tests should be run.
 config.test_exec_root = os.path.join(config.llvm_obj_root, 'test')
 
 # Tweak the PATH to include the tools dir.
-path = os.path.pathsep.join((config.llvm_tools_dir, config.environment['PATH']))
-config.environment['PATH'] = path
+llvm_config.with_environment('PATH', config.llvm_tools_dir, append_path=True)
 
-# Propagate 'HOME' through the environment.
-if 'HOME' in os.environ:
-    config.environment['HOME'] = os.environ['HOME']
-
-# Propagate 'INCLUDE' through the environment.
-if 'INCLUDE' in os.environ:
-    config.environment['INCLUDE'] = os.environ['INCLUDE']
-
-# Propagate 'LIB' through the environment.
-if 'LIB' in os.environ:
-    config.environment['LIB'] = os.environ['LIB']
-
-# Propagate the temp directory. Windows requires this because it uses \Windows\
-# if none of these are present.
-if 'TMP' in os.environ:
-    config.environment['TMP'] = os.environ['TMP']
-if 'TEMP' in os.environ:
-    config.environment['TEMP'] = os.environ['TEMP']
+# Propagate some variables from the host environment.
+llvm_config.with_system_environment(['HOME', 'INCLUDE', 'LIB', 'TMP', 'TEMP', 'ASAN_SYMBOLIZER_PATH', 'MSAN_SYMBOLIZER_PATH'])
 
 # Propagate LLVM_SRC_ROOT into the environment.
 config.environment['LLVM_SRC_ROOT'] = config.llvm_src_root
 
-# Propagate PYTHON_EXECUTABLE into the environment
-config.environment['PYTHON_EXECUTABLE'] = getattr(config, 'python_executable',
-                                                  '')
-
-# Propagate path to symbolizer for ASan/MSan.
-for symbolizer in ['ASAN_SYMBOLIZER_PATH', 'MSAN_SYMBOLIZER_PATH']:
-    if symbolizer in os.environ:
-        config.environment[symbolizer] = os.environ[symbolizer]
-
 # Set up OCAMLPATH to include newly built OCaml libraries.
 top_ocaml_lib = os.path.join(config.llvm_lib_dir, 'ocaml')
 llvm_ocaml_lib = os.path.join(top_ocaml_lib, 'llvm')
-ocamlpath = os.path.pathsep.join((llvm_ocaml_lib, top_ocaml_lib))
-if 'OCAMLPATH' in os.environ:
-    ocamlpath = os.path.pathsep.join((ocamlpath, os.environ['OCAMLPATH']))
-config.environment['OCAMLPATH'] = ocamlpath
-
-if 'CAML_LD_LIBRARY_PATH' in os.environ:
-    caml_ld_library_path = os.path.pathsep.join((llvm_ocaml_lib,
-                                os.environ['CAML_LD_LIBRARY_PATH']))
-    config.environment['CAML_LD_LIBRARY_PATH'] = caml_ld_library_path
-else:
-    config.environment['CAML_LD_LIBRARY_PATH'] = llvm_ocaml_lib
+
+llvm_config.with_system_environment('OCAMLPATH')
+llvm_config.with_environment('OCAMLPATH', top_ocaml_lib, append_path=True)
+llvm_config.with_environment('OCAMLPATH', llvm_ocaml_lib, append_path=True)
+
+llvm_config.with_system_environment('CAML_LD_LIBRARY_PATH')
+llvm_config.with_environment('CAML_LD_LIBRARY_PATH', llvm_ocaml_lib, append_path=True)
 
 # Set up OCAMLRUNPARAM to enable backtraces in OCaml tests.
-config.environment['OCAMLRUNPARAM'] = 'b'
+llvm_config.with_environment('OCAMLRUNPARAM', 'b')
 
 # Provide the path to asan runtime lib 'libclang_rt.asan_osx_dynamic.dylib' if
 # available. This is darwin specific since it's currently only needed on darwin.
 def get_asan_rtlib():
     if not "Address" in config.llvm_use_sanitizer or \
        not "Darwin" in config.host_os or \
        not "x86" in config.host_triple:
         return ""

 
 config.targets = frozenset(config.targets_to_build.split())
 
 for arch in config.targets_to_build.split():
     config.available_features.add(arch.lower() + '-registered-target')
 
 ### Features
 
-# Shell execution
-if execute_external:
-    config.available_features.add('shell')
-
 # Others/can-execute.txt
 if sys.platform not in ['win32']:
     config.available_features.add('can-execute')
     config.available_features.add('not_COFF')
 
 # Loadable module
 # FIXME: This should be supplied by Makefile or autoconf.
 if sys.platform in ['win32', 'cygwin']:
     loadable_module = (config.enable_shared == 1)
 else:
     loadable_module = True
 
 if loadable_module:
     config.available_features.add('loadable_module')
 
 # Static libraries are not built if BUILD_SHARED_LIBS is ON.
 if not config.build_shared_libs:
     config.available_features.add("static-libs")
 
-# Sanitizers.
-if 'Address' in config.llvm_use_sanitizer:
-    config.available_features.add("asan")
-else:
-    config.available_features.add("not_asan")
-if 'Memory' in config.llvm_use_sanitizer:
-    config.available_features.add("msan")
-else:
-    config.available_features.add("not_msan")
-if 'Undefined' in config.llvm_use_sanitizer:
-    config.available_features.add("ubsan")
-else:
-    config.available_features.add("not_ubsan")
-
-# Check if we should run long running tests.
-if lit_config.params.get("run_long_tests", None) == "true":
-    config.available_features.add("long_tests")
-
 # Direct object generation
 if not 'hexagon' in config.target_triple:
     config.available_features.add("object-emission")
 
-if config.have_zlib:
-    config.available_features.add("zlib")
-else:
-    config.available_features.add("nozlib")
-
 # LLVM can be configured with an empty default triple
 # Some tests are "generic" and require a valid default triple
 if config.target_triple:
     config.available_features.add("default_triple")
-    if re.match(r'^x86_64.*-linux', config.target_triple):
-      config.available_features.add("x86_64-linux")
-
-# Native compilation: host arch == default triple arch
-# FIXME: Consider cases that target can be executed
-# even if host_triple were different from target_triple.
-if config.host_triple == config.target_triple:
-    config.available_features.add("native")
 
 import subprocess
 
 def have_ld_plugin_support():
     if not os.path.exists(os.path.join(config.llvm_shlib_dir, 'LLVMgold.so')):
         return False
 
     ld_cmd = subprocess.Popen([config.gold_executable, '--help'], stdout = subprocess.PIPE, env={'LANG': 'C'})
def have_ld64_plugin_support():
     if 'ld64' not in ld_out or 'LTO' not in ld_out:
         return False
 
     return True
 
 if have_ld64_plugin_support():
     config.available_features.add('ld64_plugin')
 
-# Ask llvm-config about assertion mode.
-try:
-    llvm_config_cmd = subprocess.Popen(
-        [os.path.join(config.llvm_tools_dir, 'llvm-config'), '--assertion-mode'],
-        stdout = subprocess.PIPE,
-        env=config.environment)
-except OSError:
-    print("Could not find llvm-config in " + config.llvm_tools_dir)
-    exit(42)
-
-if re.search(r'ON', llvm_config_cmd.stdout.read().decode('ascii')):
-    config.available_features.add('asserts')
-llvm_config_cmd.wait()
+# Ask llvm-config about asserts and global-isel.
+llvm_config.feature_config('--assertion-mode', 'asserts')
+llvm_config.feature_config('--has-global-isel', 'global-isel')
 
 if 'darwin' == sys.platform:
     try:
         sysctl_cmd = subprocess.Popen(['sysctl', 'hw.optional.fma'],
                                     stdout = subprocess.PIPE)
     except OSError:
         print("Could not exec sysctl")
     result = sysctl_cmd.stdout.read().decode('ascii')
     if -1 != result.find("hw.optional.fma: 1"):
         config.available_features.add('fma3')
     sysctl_cmd.wait()
 
-if platform.system() in ['Windows']:
-    if re.match(r'.*-win32$', config.target_triple):
-        config.available_features.add('target-windows')
-    # For tests that require Windows to run.
-    config.available_features.add('system-windows')
-
 # .debug_frame is not emitted for targeting Windows x64.
 if not re.match(r'^x86_64.*-(mingw32|windows-gnu|win32)', config.target_triple):
     config.available_features.add('debug_frame')
 
-# Check if we should use gmalloc.
-use_gmalloc_str = lit_config.params.get('use_gmalloc', None)
-if use_gmalloc_str is not None:
-    if use_gmalloc_str.lower() in ('1', 'true'):
-        use_gmalloc = True
-    elif use_gmalloc_str.lower() in ('', '0', 'false'):
-        use_gmalloc = False
-    else:
-        lit_config.fatal('user parameter use_gmalloc should be 0 or 1')
-else:
-    # Default to not using gmalloc
-    use_gmalloc = False
-
-# Allow use of an explicit path for gmalloc library.
-# Will default to '/usr/lib/libgmalloc.dylib' if not set.
-gmalloc_path_str = lit_config.params.get('gmalloc_path',
-                                         '/usr/lib/libgmalloc.dylib')
-
-if use_gmalloc:
-     config.environment.update({'DYLD_INSERT_LIBRARIES' : gmalloc_path_str})
-
-# Ask llvm-config about global-isel.
-try:
-    llvm_config_cmd = subprocess.Popen(
-        [os.path.join(config.llvm_tools_dir, 'llvm-config'), '--has-global-isel'],
-        stdout = subprocess.PIPE,
-        env=config.environment)
-except OSError:
-    print("Could not find llvm-config in " + config.llvm_tools_dir)
-    exit(42)
-
-if re.search(r'ON', llvm_config_cmd.stdout.read().decode('ascii')):
-    config.available_features.add('global-isel')
-llvm_config_cmd.wait()
-
 if config.have_libxar:
     config.available_features.add('xar')
 
-if config.enable_abi_breaking_checks == "1":
-    config.available_features.add('abi-breaking-checks')
-
 if config.llvm_libxml2_enabled == "1":
     config.available_features.add('libxml2')

llvm/test/lit.site.cfg.in

 try:
     config.llvm_tools_dir = config.llvm_tools_dir % lit_config.params
     config.llvm_shlib_dir = config.llvm_shlib_dir % lit_config.params
 except KeyError:
     e = sys.exc_info()[1]
     key, = e.args
     lit_config.fatal("unable to find %r parameter, use '--param=%s=VALUE'" % (key,key))
 
+@LIT_SITE_CFG_IN_FOOTER@
+
 # Let the main config do the real work.
 lit_config.load_config(config, "@LLVM_SOURCE_DIR@/test/lit.cfg")

llvm/utils/lit/lit/llvm/__init__.py

+
+from lit.llvm import config
+
+llvm_config = None
+
+def initialize(lit_config, test_config):
+    global llvm_config
+    llvm_config = config.LLVMConfig(lit_config, test_config)
+

llvm/utils/lit/lit/llvm/config.py

+import os
+import re
+import subprocess
+import sys
+
+def _is_true_like(lit_config, value):
+    if value is None:
+        return False
+    if type(value) is bool:
+        return value
+    if isinstance(value, (int, long)):
+        return value != 0
+    if isinstance(value, basestring):
+        if value.lower() in ('1', 'true'):
+            return True
+        elif value.lower() in ('', '0', 'false'):
+            return False
+    lit_config.fatal('user boolean parameter {} is not boolean-like'.format(value))
+    return False
+
+class LLVMConfig:
+    def __init__(self, lit_config, config):
+        self.lit_config = lit_config
+        self.config = config
+
+        # Tweak PATH for Win32 to decide to use bash.exe or not.
+        if sys.platform == 'win32':
+            # For tests that require Windows to run.
+            config.available_features.add('system-windows')
+
+            # Seek sane tools in directories and set to $PATH.
+            path = self.lit_config.getToolsPath(config.lit_tools_dir,
+                                           config.environment['PATH'],
+                                           ['cmp.exe', 'grep.exe', 'sed.exe'])
+            self.with_environment('PATH', path, append_path=True)
+
+        # 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 an override.
+        use_lit_shell = os.environ.get("LIT_USE_INTERNAL_SHELL")
+        if use_lit_shell:
+            # 0 is external, "" is default, and everything else is internal.
+            self.use_lit_shell = (use_lit_shell != "0")
+        else:
+            # Otherwise we default to internal on Windows and external elsewhere, as
+            # bash on Windows is usually very slow.
+            self.use_lit_shell = (sys.platform in ['win32'])
+
+        # Shell execution
+        if not self.use_lit_shell:
+            config.available_features.add('shell')
+
+        # Native compilation: host arch == default triple arch
+        # FIXME: Consider cases that target can be executed
+        # even if host_triple were different from target_triple.
+        if config.host_triple == config.target_triple:
+            config.available_features.add("native")
+
+        # Sanitizers.
+        use_sanitizer = getattr(config, 'llvm_use_sanitizer', None)
+        if use_sanitizer:
+            if 'Address' in use_sanitizer:
+                config.available_features.add("asan")
+            else:
+                config.available_features.add("not_asan")
+            if 'Memory' in use_sanitizer:
+                config.available_features.add("msan")
+            else:
+                config.available_features.add("not_msan")
+            if 'Undefined' in use_sanitizer:
+                config.available_features.add("ubsan")
+            else:
+                config.available_features.add("not_ubsan")
+
+        have_zlib = getattr(config, 'have_zlib', None)
+        if config.have_zlib:
+            config.available_features.add("zlib")
+        else:
+            config.available_features.add("nozlib")
+
+        # Check if we should run long running tests.
+        if lit_config.params.get("run_long_tests", None) == "true":
+            self.config.available_features.add("long_tests")
+
+        target_triple = getattr(config, 'target_triple', None)
+        if target_triple:
+            if re.match(r'^x86_64.*-linux', target_triple):
+                config.available_features.add("x86_64-linux")
+            if re.match(r'.*-win32$', target_triple):
+                config.available_features.add('target-windows')
+
+        def should_use_gmalloc(lit_config):
+            # Check if we should use gmalloc.
+            use_gmalloc_str = lit_config.params.get('use_gmalloc', None)
+            return _is_true_like(lit_config, use_gmalloc_str)
+
+        if should_use_gmalloc(lit_config):
+            # Allow use of an explicit path for gmalloc library.
+            # Will default to '/usr/lib/libgmalloc.dylib' if not set.
+            gmalloc_path_str = lit_config.params.get('gmalloc_path',
+                                                     '/usr/lib/libgmalloc.dylib')
+            if gmalloc_path_str is not None:
+                self.with_environment('DYLD_INSERT_LIBRARIES', gmalloc_path_str)
+
+        breaking_checks = getattr(config, 'enable_abi_breaking_checks')
+        if _is_true_like(lit_config, breaking_checks):
+            config.available_features.add('abi-breaking-checks')
+
+    def with_environment(self, variable, value, **kwargs):
+        append_path = kwargs.get('append_path', None)
+        if append_path == True and variable in self.config.environment:
+            def norm(x):
+                return os.path.normcase(os.path.normpath(x))
+
+            # Move it to the front if it already exists, otherwise insert it at the
+            # beginning.
+            value = norm(value)
+            current_value = self.config.environment[variable]
+            items = [norm(x) for x in current_value.split(os.path.pathsep)]
+            try:
+                index = items.index(value)
+                items.insert(0, items.pop(index))
+            except:
+                items.insert(0, value)
+            value = os.path.pathsep.join(items)
+        self.config.environment[variable] = value
+
+
+    def with_system_environment(self, vars, **kwargs):
+        if vars is str:
+            vars = [vars]
+        for v in vars:
+            if v in os.environ:
+                self.with_environment(v, os.environ[v], **kwargs)
+
+    def feature_config(self, flag, feature):
+        # Ask llvm-config about assertion mode.
+        try:
+            llvm_config_cmd = subprocess.Popen(
+                [os.path.join(self.config.llvm_tools_dir, 'llvm-config'), flag],
+                stdout = subprocess.PIPE,
+                env=self.config.environment)
+        except OSError:
+            print("Could not find llvm-config in " + self.config.llvm_tools_dir)
+            exit(42)
+
+        if re.search(r'ON', llvm_config_cmd.stdout.read().decode('ascii')):
+            self.config.available_features.add(feature)
+        llvm_config_cmd.wait()