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

[Francis Ricci via Phabricator via llvm-commits]

fjricci added inline comments.


================
Comment at: utils/lit/lit/llvm/config.py:83
             if re.match(r'^x86_64.*-apple', target_triple):
-                if 'address' in sanitizers:
+                if 'address' in sanitizers and self.get_clang_has_lsan(config.host_cxx, target_triple):
                     self.with_environment(
----------------
fjricci wrote:
> fjricci wrote:
> > fjricci wrote:
> > > zturner wrote:
> > > > zturner wrote:
> > > > > How is `config.host_cxx` set?  I don't think this is always set is it?
> > > > 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.
> > > > 
> > > `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.
> > I'll verify with check-clang though, to be sure.
> 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.
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


https://reviews.llvm.org/D38703