Page MenuHomePhabricator

[cmake] Always build the libLLVM shared library
Needs ReviewPublic

Authored by tstellar on Nov 8 2019, 1:26 PM.

Details

Summary

We discourage non-developers from using the BUILD_SHARED_LIBS option
and recommend using LLVM_BUILD_LLVM_DYLIB=ON for building LLVM as
a shared library. We should make it as easy as possible for
users to build with our recommended configuration, so to help with that
this patch makes building libLLVM.so the default and removes the
LLVM_BUILD_LLVM_DYLIB option altogether.

Removing this option will help simplify the build system and also
help downstream users of LLVM, because they won't need to add
special logic to their own build systems to detect whether or not libLLVM
was built and installed.

Diff Detail

Event Timeline

tstellar created this revision.Nov 8 2019, 1:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2019, 1:26 PM
lebedev.ri added inline comments.
llvm/docs/CMake.rst
568

What happens to this limitation?
Does -DBUILD_SHARED_LIBS=ON begins to just work?

I'm kinda torn on this. Although this simplifies the build, linking the DSO on Linux is painful unless you externalise the debug info (i.e. build with fission). On the other hand, people should be encouraged to setup the distributions and use that to build the full set that they need. I think that marking libLLVM as EXCLUDE_FROM_ALL is a decent compromise.

I'm kinda torn on this. Although this simplifies the build, linking the DSO on Linux is painful unless you externalise the debug info (i.e. build with fission). On the other hand, people should be encouraged to setup the distributions and use that to build the full set that they need. I think that marking libLLVM as EXCLUDE_FROM_ALL is a decent compromise.

Is it more painful than linking any other tool in the tools/ dir?

I very much dislike using EXCLUDE_FROM_ALL. I really believe all should be all, not most things I think are important. In general iteration cycles the check-* targets won't build or link libLLVM unless you specify LLVM_LINK_LLVM_DYLIB, and if you actually want to build everything as a means of testing your changes the all target should do that.

On the specifics of this change I think there are some problems because of CMake's lack of cache invalidation. Changing this variable won't actually change anything until people delete their build directories.

Instead if you remove all uses of the variable, and you could add:

if (MSVC or BUILD_SHARED_LIBS)
  if (LLVM_LINK_LLVM_DYLIB)
    message(FATAL_ERROR "LLVM_LINK_LLVM_DYLIB is not supported with BUILD_SHARED_LIBS or using MSVC")
  endif()
  return()
endif()

to llvm-shlib/CMakelists.txt to handle Win32, and it should have roughly the same impact without cache invalidation problems.

tstellar marked an inline comment as done.Nov 11 2019, 5:56 PM
llvm/CMakeLists.txt
573–577

@beanz Does setting the variables here override the cache variables?

smeenai added inline comments.Nov 11 2019, 8:00 PM
llvm/CMakeLists.txt
573–577

Yup, setting the variable here will override the cached value. Any reason to prefer this to just removing all uses of the variable though?

tstellar marked an inline comment as done.Nov 11 2019, 8:15 PM
tstellar added inline comments.
llvm/CMakeLists.txt
573–577

I thought it would be cleaner to keep the variable than to replace it with if (MSVC) everywhere.

tstellar marked an inline comment as done.
tstellar added inline comments.
llvm/docs/CMake.rst
568

D70179 will fix this.

beanz added inline comments.Nov 13 2019, 11:19 AM
llvm/CMakeLists.txt
573–577

There is really only one place the if (MSVC) check would be needed, in the llvm-shlib/CMakeLists.txt file. All the other uses of the variable could disappear.

tstellar updated this revision to Diff 229396.Nov 14 2019, 1:31 PM

Remove all uses of the LLVM_BUILD_LLVM_DYLIB variable.

tstellar marked an inline comment as done.Nov 14 2019, 1:34 PM
tstellar added inline comments.
llvm/CMakeLists.txt
651–652

This change I'm not so sure about. Can we drop the WIN32 part of the condition? Does CYGWIN support building shared libs?

tstellar accepted this revision.Nov 22 2019, 9:02 AM

Ping. All the prerequisite patches have been committed now.

This revision is now accepted and ready to land.Nov 22 2019, 9:02 AM
tstellar marked an inline comment as done.Nov 22 2019, 9:03 AM
tstellar requested review of this revision.Nov 22 2019, 10:22 PM
compnerd requested changes to this revision.Nov 29 2019, 10:54 AM
compnerd added inline comments.
llvm/CMakeLists.txt
651–652

Why the desire to drop WIN32? Both Windows and cygwin support shared libraries.

llvm/tools/llvm-config/CMakeLists.txt
60

I think that this should really be NOT CMAKE_SYSTEM_NAME STREQUAL Windows. I can build for windows using clang (not clang-cl) in which case, we get the value wrong.

llvm/tools/llvm-shlib/CMakeLists.txt
12

Similar

This revision now requires changes to proceed.Nov 29 2019, 10:54 AM
tstellar updated this revision to Diff 232445.Dec 5 2019, 1:32 PM

Use correct checks for Windows.

Build result: FAILURE - Could not check out parent git hash "6f585e1ccfdce0c807fc2844e5c9cc3e6694101d". It was not found in the repository. Did you configure the "Parent Revision" in Phabricator properly? Trying to apply the patch to the master branch instead...

ERROR: arc patch failed with error code 1. Check build log for details.
Log files: console-log.txt, CMakeCache.txt

tstellar marked 2 inline comments as done.Dec 5 2019, 1:35 PM
tstellar added inline comments.
llvm/CMakeLists.txt
651–652

I just wasn't sure what WIN32 meant. Does it mean all of windows or just 32-bit windows? I'm fine with keeping it.

compnerd added inline comments.Dec 6 2019, 3:15 PM
llvm/CMakeLists.txt
651–652

Yes, it means all windows builds.

smeenai resigned from this revision.May 13 2020, 2:57 PM

Anything holding this up?

Resigning as reviewer to remove this from my queue.