This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Add compiler-rt header files to the list of sources for targets when building with an IDE
ClosedPublic

Authored by delcypher on Jun 21 2018, 5:05 AM.

Details

Summary

[CMake] Add compiler-rt header files to the list of sources for targets
when building with an IDE so that header files show up in the UI.
This massively improves the development workflow in IDEs.

To implement this a new function compiler_rt_process_sources(...) has
been added that adds header files to the list of sources when the
generator is an IDE. For non-IDE generators (e.g. Ninja/Makefile) no
changes are made to the list of source files.

The function can be passed a list of headers via the
ADDITIONAL_HEADERS argument. For each runtime library a list of
explicit header files has been added and passed via
ADDITIONAL_HEADERS. For tsan and sanitizer_common a list of
headers was already present but it was stale and has been updated
to reflect the current state of the source tree.

The original version of this patch used file globbing (*.{h,inc,def})
to find the headers but the approach was changed due to this being a
CMake anti-pattern (if the list of headers changes CMake won't
automatically re-generate if globbing is used).

The LLVM repo contains a similar function named llvm_process_sources()
but we don't use it here for several reasons:

  • It depends on the LLVM_ENABLE_OPTION cache variable which is not set in standalone compiler-rt builds.
  • We would have to include(LLVMProcessSources) which I'd like to avoid because it would include a bunch of stuff we don't need.

Diff Detail

Event Timeline

delcypher created this revision.Jun 21 2018, 5:05 AM
Herald added subscribers: Restricted Project, mgorny, dberris. · View Herald TranscriptJun 21 2018, 5:05 AM

Besides the naming nit, this looks good to me, but I'm not a CMake expert. @george.karpenkov could you review this?

cmake/Modules/AddCompilerRT.cmake
61

Please rename "crt" to "compiler_rt" (throughout the patch). I don't think we're using "crt" to mean "compiler-rt" anywhere in the codebase.

Personally, I'd prefer to spell out all the headers explicitly rather than using globing which is more error-prone to future changes. I also agree with @kubamracek's comment about using crt (which may be also mistaken for crt*.o) to compiler_rt.

Personally, I'd prefer to spell out all the headers explicitly rather than using globing which is more error-prone to future changes. I also agree with @kubamracek's comment about using crt (which may be also mistaken for crt*.o) to compiler_rt.

My patch does allow for this (use ADDITIONAL_HEADERS argument). In fact I used that for clang_rt.tsan because there was already a list of TSan header files (previously unused) already there.

I already mentioned (in the commit message) that globbing for sources is generally a CMake anti-pattern. However this is the approach used the in the LLVM CMake code, which is why I adopted it.

I have no problem listing the header files explicitly, but I don't know how the rest of the compiler-rt developers feel about this. I'll add a few more reviewers to see if we can get a consensus.

cmake/Modules/AddCompilerRT.cmake
61

Sure. I'll fix that.

delcypher updated this revision to Diff 152886.Jun 26 2018, 7:20 AM
delcypher edited the summary of this revision. (Show Details)
  • s/crt_process_sources/compiler_rt_process_sources/
  • s/crt_find_headers_in_dir/compiler_rt_find_headers_in_dir/
  • Improve commit message.
cryptoad resigned from this revision.Jun 26 2018, 8:23 AM
cmake/Modules/CompilerRTUtils.cmake
356

Since we are not testing on MSVC should we really handle it here?

361

I'm not sure what do we need here. Could we just ask Clang on each source file which headers it requires? I believe that's what CMake does behind the scenes to support incremental compilation, it's just harder for compiler-rt since we are not using the default compiler.

george.karpenkov requested changes to this revision.Jun 26 2018, 5:53 PM

Requesting changes/clarification on the globbing approach used.

This revision now requires changes to proceed.Jun 26 2018, 5:53 PM
delcypher added inline comments.Jun 27 2018, 6:21 AM
cmake/Modules/CompilerRTUtils.cmake
356

This conditional is taken from cmake/modules/HandleLLVMOptions.cmake (LLVM repo) and is the expression used to initialize the default value of LLVM_ENABLE_IDE. Given that support for IDE folders was already added to compiler-rt for an in-tree build for Visual Studio (r275111) at least in one point in time Visual Studio was used to build compiler-rt.

I'd rather keep the MSVC_IDE condition and only remove it if the bots break.

361

Could we just ask Clang on each source file which headers it requires? I believe that's what CMake does behind the scenes to support incremental compilation,

No. First we can't assume we are using clang. Compiler-rt is also built by gcc (and possibly by MSVC). Also incremental compilation is actually dealt with (at least in the case of the Makefile and Ninja generators) by the generated build system by having the first invocation of a target generate dependency files that are later consumed by re-compilations of the same target.

What I'm doing here is simply what the LLVM project does (apart from TSan which already had a list lying around that I could just use). I would actually prefer to do what @phosek suggests and list all header files explicitly, however I'm not sure if people will agree with that because that's not what is done in the LLVM project.

george.karpenkov accepted this revision.Jun 27 2018, 1:18 PM

@delcypher OK, I didn't know that!
I'm personally fine with either hardcoding the list of headers or using globbing, as new headers get added rather rarely.

This revision is now accepted and ready to land.Jun 27 2018, 1:18 PM
delcypher updated this revision to Diff 153305.Jun 28 2018, 6:21 AM
delcypher edited the summary of this revision. (Show Details)

Switch to explicit lists of headers instead of globbing.

@eugenis While doing this I think I spotted a typo for a hwasan target. The RTHwasan_dynamic target includes ${TSAN_RTL_CXX_SOURCES} in SOURCES which doesn't make sense. It's probably supposed to be ${HWASAN_RTL_CXX_SOURCES}. I've not fixed it in this patch but I left a FIXME near by. Do you want to fix this?

@george.karpenkov @phosek I switched to using explicit lists of headers and found some problems in the process and fixed them. Could you take another look?

@george.karpenkov @phosek I switched to using explicit lists of headers and found some problems in the process and fixed them. Could you take another look?

Ping.

This revision was automatically updated to reflect the committed changes.