This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Make compiler-rt tests work with relocatable SDKs on OS X
ClosedPublic

Authored by kubamracek on Jun 6 2014, 12:38 PM.

Details

Summary

Hi,

I am trying to play a bit with ASan on OS X and I found out that on recent OS X releases, compiler-rt tests fail to build and run with CMake. That's because OS X now provides relocatable SDKs with Xcode and has no system-wide standard headers/libs. I am aware that currently compiler-rt tests are supported only with CMake.

There are actually two issues:

  • Newly built clang binary can't find the platform SDK. Either the SDKROOT env has to be set or -isysroot passed on command line.
  • Newly build clang binary can't find c++ headers. It expects to find them in "../include/c++" relative to its binary. This issue goes away if you put libcxx into "llvm/projects/" and build libcxx simultaneously with llvm.

My proposed patch adds OS X detection into CMake to add the -isysroot parameter and it creates a symlink pointing from the build directory's include/c++ into Xcode's toolchain (but only if you don't simultaneously build libcxx). It also moves a similar SDKROOT detection from clang's lit config into a util function and uses it in compiler-rt lit tests as well.

One other solution to make the tests build and pass would be to enforce OS X users to set their SDKROOT env property (most OS X users don't have it set) and lit would need to be changed in a way to propagate this env property (it doesn't now). Installing a SDK into a system-wide location (e.g. /usr/include) is discouraged and deprecated on OS X, although manually installing Xcode Command Line Tools does that. Note that this will not solve the other issue (missing c++ headers).

The attached patch is three files, which should go into llvm, compiler-rt and clang projects respectively.

Kuba Brecka

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 10188.Jun 6 2014, 12:38 PM
kubamracek retitled this revision from to Make compiler-rt tests work with relocatable SDKs on OS X.
kubamracek updated this object.
kubamracek edited the test plan for this revision. (Show Details)
kubamracek set the repository for this revision to rL LLVM.
kubamracek added subscribers: Unknown Object (MLST), kubamracek.
samsonov added inline comments.
compiler-rt/trunk/CMakeLists.txt
379

I don't like the idea of adding a symlink - it seems too brittle and unexpected that compiler-rt build would for some reason create a symlink in the build tree that would point to c++ headers. If you can't provide path to c++ headers in CMAKE_CXX_FLAGS when you configure a build tree, consider just modifying the compile flags using the output of xcrun.

compiler-rt/trunk/cmake/Modules/AddCompilerRT.cmake
151

Please don't use ASan-specific variables here.

llvm/trunk/utils/lit/lit/util.py
171 ↗(On Diff #10188)

Please send this path to llvm-commits separately, and let lit maintainers decide if they want to add this functionality to lit.

Sorry, haven't had time to look yet.
Will try to do that tomorrow.

kubamracek added inline comments.Jun 9 2014, 6:26 PM
compiler-rt/trunk/CMakeLists.txt
379

Providing the path in a variable (either externally or inside cmake) will make compiler-rt build and pass tests, however the output clang binary would still be unable to compile C++ source out of the box:

$ ../llvm-cmake/bin/clang a.cc
a.cc:4:10: fatal error: 'string' file not found
#include <string>
         ^
1 error generated.

On other platforms this is not an issue, because you usually have /usr/include/c++. If this is expected/intentional, we should be more specific with the build instructions for OS X at http://clang.llvm.org/get_started.html.

compiler-rt/trunk/cmake/Modules/AddCompilerRT.cmake
151

is there something like COMPILER_RT_COMMON_LINKFLAGS? if not, can I introduce such a variable?

kubamracek updated this revision to Diff 10257.Jun 9 2014, 6:28 PM

Moved the lit-related stuff to http://reviews.llvm.org/D4072.

Looks mostly good (see the comments). I'm not sure this doesn't break on 10.8, but let's see if any problems appear.

compiler-rt/trunk/cmake/Modules/AddCompilerRT.cmake
143

This comment is misleading. This build config is for compiler-rt, not unit tests.

compiler-rt/trunk/lib/asan/tests/CMakeLists.txt
68

Why remove this line? The tests actually do depend on Foundation.

I've recently deinstalled LLVM from our Mavericks machine and am seeing the same problem now.
But I'm not convinced it's a good idea to make all these symlink changes in compiler-rt CMake files. Instead this should probably be done in Clang.

In D4047#11, @glider wrote:

I've recently deinstalled LLVM from our Mavericks machine and am seeing the same problem now.
But I'm not convinced it's a good idea to make all these symlink changes in compiler-rt CMake files. Instead this should probably be done in Clang.

Yeah, the automatic symlink creation is weird. How about just having a detection that there are c++ headers available, if not then we'll just stop building/testing informing you to either copy or symlink libcxx into your build directory.

For the SDKROOT stuff I still think we should detect and set this automatically, mostly because other parts of LLVM/Clang are doing it already.

Yeah, the automatic symlink creation is weird. How about just having a detection that there are c++ headers available, if not then we'll just stop building/testing informing you to either copy or symlink libcxx into your build directory.

Can't this be done by installing the Xcode command line tools? (I've failed to, but perhaps I was doing something wrong)

For the SDKROOT stuff I still think we should detect and set this automatically, mostly because other parts of LLVM/Clang are doing it already.

Any pointers? A quick grep doesn't show any additional -isysroot flags.

compiler-rt/trunk/cmake/Modules/AddCompilerRT.cmake
151

I think ASAN_UNITTEST_COMMON_LINKFLAGS should be set in the corresponding build config.

For the SDKROOT stuff I still think we should detect and set this automatically, mostly because other parts of LLVM/Clang are doing it already.

Any pointers? A quick grep doesn't show any additional -isysroot flags.

lit tests (utils/lit/lit/util.py, tools/clang/test/lit.cfg, projects/compiler-rt/test/lit.common.cfg). It's only needed when you want to *run* the just built clang without doing stuff like "make install". And compiler-rt uses the newly build clang during its build phase of check-asan, so that's why we're seeing this issue.

Can't this be done by installing the Xcode command line tools? (I've failed to, but perhaps I was doing something wrong)

Probably. But the whole point of having relocatable SDKs and toolchains on OS X is so you don't have to install CL Tools, and so you can switch between different compilers/Xcodes/SDKs.

kubamracek updated this revision to Diff 13436.Sep 8 2014, 7:09 PM
kubamracek retitled this revision from Make compiler-rt tests work with relocatable SDKs on OS X to [compiler-rt] Make compiler-rt tests work with relocatable SDKs on OS X.

Another try to solve the issue with compiler-rt unit tests and relocatable SDKs on OS X. This patch adds a check into the build process that:

  • before using the just-built clang, tries to compile a single-line #include <iostream>
  • if that fails, prints out a user-friendly message that they should copy or symlink existing C++ headers into the LLVM output directory

I like this more. There's yet another option which I ended up with when I've encountered this problem on our OSX bot that bootstraps Clang.
It is to run:

cd ${LLVM_BINARY_DIR} && make -C ${LLVM_CHECKOUT}/projects/libcxx installheaders HEADER_DIR=${PWD}/include

, which installs the libcxx headers from the Clang checkout for of the newly built Clang.

kubamracek updated this revision to Diff 13490.Sep 9 2014, 12:59 PM

Changing the error message to also suggest checking out libcxx from llvm.org and doing "installheaders" into the build directory.

This revision is now accepted and ready to land.Sep 10 2014, 8:07 AM
glider accepted this revision.Sep 10 2014, 8:11 AM
glider added a reviewer: glider.

Sorry, the wrong Glider

kubamracek closed this revision.Sep 10 2014, 10:35 AM

Landed in r217523.