[lit] Only enable LSan on darwin when clang supports it

[fjricci]

Summary

LSan on darwin doesn't exist on older versions of clang,
causing non-boostrapped sanitized buildbots to fail

[fjricci]

Fixing a couple issues.

[fjricci]

Fix a couple python issues

[zturner]

utils/lit/lit/llvm/config.py

83–84

How is config.host_cxx set? I don't think this is always set is it?

[zturner]

utils/lit/lit/llvm/config.py

83–84

Actually I think this is wrong, for several reasons:

1. This is only set in LLVM's lit configuration, and not in, for example, clang's lit configuration. But this code will run no matter what. So if you were to run ninja check-clang then you'd get an error here.

2. "Which clang are we using?" is not easy to answer generally. If you're testing LLVM then there is not necessarily even a clang to begin with, you coudl be using gcc or something. Regardless, you want to refer to the host compiler (i.e. the compiler used to build LLVM). On the other hand, if you're testing clang then you probably want to test the clang you just built, and not the host compiler (even if the host compiler was clang). So in this case, the logic to determine which clang to use is not so simple, and clang's configuration files hav a function called 'inferClang' for this purpose.

[fjricci]

utils/lit/lit/llvm/config.py

83–84

config.host_cxx is set in the same file that sets config.llvm_use_sanitizer (which probably means I should use getattr() instead, now that I look at it), but which also means that it will always be defined in this if block. config.host_cxx is set to HOST_CXX from cmake.

I do think that this is correct generally, because we don't actually want the clang we're testing, we want the clang that built the llvm source (since that's what will be compiled with -fsanitize=address, and which will provide the ASan DSO which needs to have LSan in it). The unit tests themselves won't be compiled with ASan afaik, so the version of clang used to build the unit tests isn't what we want here.

[fjricci]

utils/lit/lit/llvm/config.py

83–84

I'll verify with check-clang though, to be sure.

[fjricci]

utils/lit/lit/llvm/config.py

83–84

Yup, you're right, host_cxx isn't defined. We still don't want the just-built clang though, I'll try to figure out a solution.

[fjricci]

utils/lit/lit/llvm/config.py

83–84

Is there a reason we can't just add config.host_cxx = CMAKE_CXX_COMPILER to the clang lit config? Or if not that, config.host_cxx_version = CMAKE_CXX_COMPILER_VERSION

[zturner]

You can probably just define host_cxx, and then use getattr in the function to check for it.

[zturner]

Heh, we crossed paths. Yea that seems reasonable. Can you emit a warning if any of the following are true though?

1. llvm_use_sanitizer is true, but host_cxx doesn't look like a clang compiler.
2. 'self.get_clang_has_lsan` is called, but host_cxx is not present.

[fjricci]

Address comments

[fjricci]

I'll upload the clang host_cxx patch shortly, but this shouldn't be blocked on that (since it handles the undefined case).

[fjricci]

Add warnings

[fjricci]

Clang change here: reviews.llvm.org/D38741

[zturner]

utils/lit/lit/llvm/config.py

207

Can you improve this warning to say why it's needed? Something like "config.host_cxx is unset but test suite is configured to use sanitizers.

213–214

Same here. compiler '%s' does not appear to be clang, but test suite is configured to use sanitizers.

[fjricci]

Improve warning messages

[fjricci]

@qcolombet - Can you keep an eye on the internal apple builders and let me know if this doesn't fix them? I'll be out of the town the rest of the week though, so I probably won't be able to do much until Monday.

[qcolombet]

I will.
Thanks!

[qcolombet]

The problem is fixed.

Thanks again.

utils/lit/lit/llvm/config.py

def __init__(self, lit_config, config):
 
         # Check if we should run long running tests.
         long_tests = lit_config.params.get('run_long_tests', None)
         if lit.util.pythonize_bool(long_tests):
             features.add('long_tests')
 
         if target_triple:
             if re.match(r'^x86_64.*-apple', target_triple):
-                if 'address' in sanitizers:
+                host_cxx = getattr(config, 'host_cxx', None)
+                if 'address' in sanitizers and self.get_clang_has_lsan(host_cxx, target_triple):
                     self.with_environment(
                         'ASAN_OPTIONS', 'detect_leaks=1', append_path=True)
             if re.match(r'^x86_64.*-linux', target_triple):
                 features.add('x86_64-linux')
             if re.match(r'.*-win32$', target_triple):
                 features.add('target-windows')
 
         use_gmalloc = lit_config.params.get('use_gmalloc', None)
def get_clang_builtin_include_dir(self, clang):
 
         clang_dir = clang_dir.strip()
         if sys.platform in ['win32'] and not self.use_lit_shell:
             # Don't pass dosish path separator to msys bash.exe.
             clang_dir = clang_dir.replace('\\', '/')
         # Ensure the result is an ascii string, across Python2.5+ - Python3.
         return clang_dir
 
+    # On macOS, LSan is only supported on clang versions 5 and higher
+    def get_clang_has_lsan(self, clang, triple):
+        if not clang:
+            self.lit_config.warning("config.host_cxx is unset")
+            return False
+
+        clang_binary = clang.split()[0]
+        version_string, _ = self.get_process_output([clang_binary, '--version'])
+        if not 'clang' in version_string:
+            self.lit_config.warning(
+                "compiler '%s' does not appear to be clang" % clang_binary)
+            return False
+
+        if re.match(r'.*-linux', triple):
+            return True
+
+        if re.match(r'^x86_64.*-apple', triple):
+            version_number = int(re.search(r'version ([0-9]+)\.', version_string).group(1))
+            if 'Apple LLVM' in version_string:
+                return version_number >= 9
+            else:
+                return version_number >= 5
+
+        return False
+
     def make_itanium_abi_triple(self, triple):
         m = re.match(r'(\w+)-(\w+)-(\w+)', triple)
         if not m:
             self.lit_config.fatal(
                 "Could not turn '%s' into Itanium ABI triple" % triple)
         if m.group(3).lower() != 'win32':
             # All non-win32 triples use the Itanium ABI.
             return triple