This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Add option to disable libc++ build
ClosedPublic

Authored by smeenai on Jan 9 2019, 12:33 AM.

Details

Summary

Having libc++ checked out doesn't necessarily mean it should be built;
for example, the same source tree might be used for multiple build
configurations, and libc++ might not build in some of those
configurations. Add an option to compiler-rt's build to disable building
libc++. This defaults to ON, so it shouldn't change any existing build
configurations.

Diff Detail

Event Timeline

smeenai created this revision.Jan 9 2019, 12:33 AM
Herald added subscribers: Restricted Project, delcypher, mgorny, dberris. · View Herald Transcript
mgorny added a comment.Jan 9 2019, 3:56 AM

Could you also add a local switch to control using it independently of main build? The sanitized builds are quite time-consuming.

Could you also add a local switch to control using it independently of main build? The sanitized builds are quite time-consuming.

As in, add a compiler-rt specific option to disable using libc++? Maybe something like COMPILER_RT_DISABLE_LIBCXX (or its positive version)? I have no opinions on that (I don't really work on compiler-rt), although I'm wondering if that flag should be in addition to the LLVM_TOOL_LIBCXX_BUILD check or just replace it.

I think it'd be better to have a positive version, e.g. something like COMPILER_RT_USE_LIBCXX_SOURCES. Maybe it could default to ${LLVM_TOOL_LIBCXX_BUILD}.

Sounds good ... I'll update this patch.

smeenai updated this revision to Diff 180896.Jan 9 2019, 12:02 PM
smeenai retitled this revision from [compiler-rt] Respect libc++ build setting to [compiler-rt] Add option to disable libc++ build.
smeenai edited the summary of this revision. (Show Details)

@mgorny suggestion

phosek added a comment.Jan 9 2019, 9:53 PM

This also needs to support the runtimes build which defines HAVE_LIBCXX variable when libc++ is being built.

Where's LLVM_TOOL_LIBCXX_BUILD handled? Searching through the tree, I haven't found any references to LLVM_TOOL_LIBCXX_BUILD, I have found a few references to other LLVM_TOOL_${TOOL} variables in llvm/tools but I haven't found any logic that's consuming them aside from llvm/test/CMakeList.txt and polly/test/CMakeList.txt.

This also needs to support the runtimes build which defines HAVE_LIBCXX variable when libc++ is being built.

Where's LLVM_TOOL_LIBCXX_BUILD handled? Searching through the tree, I haven't found any references to LLVM_TOOL_LIBCXX_BUILD, I have found a few references to other LLVM_TOOL_${TOOL} variables in llvm/tools but I haven't found any logic that's consuming them aside from llvm/test/CMakeList.txt and polly/test/CMakeList.txt.

The LLVM_TOOL_*_BUILD variables are checked by add_llvm_subdirectory (which is called by add_llvm_external_project with project being LLVM and type being TOOL, which is where the LLVM_TOOL_ prefix comes from). They'll default to on for anything that lives inside llvm/tools or llvm/projects, and anything that's enabled via LLVM_ENABLE_PROJECTS or LLVM_EXTERNAL_PROJECTS.

smeenai updated this revision to Diff 181492.Jan 13 2019, 4:51 PM
smeenai edited the summary of this revision. (Show Details)

Take HAVE_LIBCXX into account

smeenai added a comment.EditedJan 13 2019, 4:54 PM

I updated to use HAVE_LIBCXX as well, and confirmed that the following scenarios work:

  • Having both compiler-rt and libc++ checked out under llvm/projects
  • Using -DLLVM_ENABLE_PROJECTS=compiler-rt;libcxx (the order of projects doesn't matter)
  • Using -DLLVM_ENABLE_RUNTIMES=compiler-rt;libcxx (the order of runtimes doesn't matter)

What doesn't work is having libc++ under llvm/projects and compiler-rt under llvm/runtimes (or correspondingly, having libc++ in LLVM_ENABLE_PROJECTS and compiler-rt in LLVM_ENABLE_RUNTIMES). I can fix that by passing through the LLVM_TOOL_*_BUILD variables to the sub-configures, but I'm not sure that's a build setup anyone would use?

The other option would be to just have COMPILER_RT_USE_LIBCXX always default to ON and require users to set it to OFF explicitly if they disable the build. That would require setting an additional cache variable during configuration instead of it being inferred, but it removes the possibility of breaking any existing setups. What do you think?

smeenai updated this revision to Diff 181624.Jan 14 2019, 12:08 PM
smeenai edited the summary of this revision. (Show Details)

Default to ON

Okay, I decided to make it default ON. I think this should be a pretty safe and uncontroversial change at this point?

phosek accepted this revision.Jan 14 2019, 12:13 PM

Okay, I decided to make it default ON. I think this should be a pretty safe and uncontroversial change at this point?

This is fine with me, although I wouldn't be concerned about mixing llvm/projects and compiler-rt under llvm/runtimes either since that setup is untested and most likely broken anyway.

This revision is now accepted and ready to land.Jan 14 2019, 12:13 PM
This revision was automatically updated to reflect the committed changes.