Page MenuHomePhabricator

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

Authored by fjricci on Oct 9 2017, 1:07 PM.



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

Diff Detail


Event Timeline

fjricci created this revision.Oct 9 2017, 1:07 PM
fjricci planned changes to this revision.Oct 9 2017, 1:16 PM

Fixing a couple issues.

fjricci updated this revision to Diff 118264.Oct 9 2017, 1:20 PM

Fix a couple python issues

zturner added inline comments.Oct 9 2017, 2:24 PM
83 ↗(On Diff #118264)

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

zturner added inline comments.Oct 9 2017, 2:36 PM
83 ↗(On Diff #118264)

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.
  1. "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 added inline comments.Oct 9 2017, 2:51 PM
83 ↗(On Diff #118264)

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 added inline comments.Oct 9 2017, 2:59 PM
83 ↗(On Diff #118264)

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

fjricci added inline comments.Oct 9 2017, 3:38 PM
83 ↗(On Diff #118264)

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 added inline comments.Oct 9 2017, 3:40 PM
83 ↗(On Diff #118264)

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 edited edge metadata.Oct 9 2017, 3:41 PM

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

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 updated this revision to Diff 118402.Oct 10 2017, 8:57 AM

Address comments

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

fjricci updated this revision to Diff 118404.Oct 10 2017, 9:16 AM

Add warnings

Clang change here:

zturner accepted this revision.Oct 10 2017, 9:24 AM
zturner added inline comments.
207 ↗(On Diff #118404)

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 ↗(On Diff #118404)

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

This revision is now accepted and ready to land.Oct 10 2017, 9:24 AM
fjricci updated this revision to Diff 118413.Oct 10 2017, 10:10 AM

Improve warning messages

This revision was automatically updated to reflect the committed changes.

@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 edited edge metadata.Oct 11 2017, 11:25 AM

I will.

The problem is fixed.

Thanks again.