[CMake] respect LLVMConfig.cmake's LLVM_DEFINITIONS in standalone builds #138587
Conversation
|
@llvm/pr-subscribers-lldb @llvm/pr-subscribers-clang Author: None (jeremyd2019) ChangesIn #138329, _GNU_SOURCE was added for Cygwin, but when building Clang standalone against an installed LLVM this definition was not picked up, resulting in undefined strnlen. Follow the documentation in https://llvm.org/docs/CMake.html#developing-llvm-passes-out-of-source and add the LLVM_DEFINITIONS in Clang's CMakeLists.txt. Get rid of the list(REMOVE CMAKE_REQUIRED_DEFINITIONS -D_GNU_SOURCE) later, as list(REMOVE) is documented to remove all occurences of the item, not just the one that was just added. Instead, make use of cmake_push_check_state() and cmake_pop_check_state(), which is already used in other cmake files. Full diff: https://github.com/llvm/llvm-project/pull/138587.diff 1 Files Affected:
diff --git a/clang/CMakeLists.txt b/clang/CMakeLists.txt
index c3f30e2a8e9c0..ab2ac9bc6b9ad 100644
--- a/clang/CMakeLists.txt
+++ b/clang/CMakeLists.txt
@@ -68,6 +68,10 @@ if(CLANG_BUILT_STANDALONE)
option(CLANG_ENABLE_BOOTSTRAP "Generate the clang bootstrap target" OFF)
option(LLVM_ENABLE_LIBXML2 "Use libxml2 if available." ON)
+ separate_arguments(LLVM_DEFINITIONS_LIST NATIVE_COMMAND ${LLVM_DEFINITIONS})
+ add_definitions(${LLVM_DEFINITIONS_LIST})
+ list(APPEND CMAKE_REQUIRED_DEFINITIONS ${LLVM_DEFINITIONS_LIST})
+
include(AddLLVM)
include(TableGen)
include(HandleLLVMOptions)
@@ -183,18 +187,17 @@ check_include_file(sys/resource.h CLANG_HAVE_RLIMITS)
# This check requires _GNU_SOURCE on linux
check_include_file(dlfcn.h CLANG_HAVE_DLFCN_H)
if( CLANG_HAVE_DLFCN_H )
+ include(CMakePushCheckState)
include(CheckLibraryExists)
include(CheckSymbolExists)
check_library_exists(dl dlopen "" HAVE_LIBDL)
+ cmake_push_check_state()
if( HAVE_LIBDL )
list(APPEND CMAKE_REQUIRED_LIBRARIES dl)
endif()
list(APPEND CMAKE_REQUIRED_DEFINITIONS -D_GNU_SOURCE)
check_symbol_exists(dladdr dlfcn.h CLANG_HAVE_DLADDR)
- list(REMOVE_ITEM CMAKE_REQUIRED_DEFINITIONS -D_GNU_SOURCE)
- if( HAVE_LIBDL )
- list(REMOVE_ITEM CMAKE_REQUIRED_LIBRARIES dl)
- endif()
+ cmake_pop_check_state()
endif()
set(CLANG_RESOURCE_DIR "" CACHE STRING
|
|
If this is right, it should probably be done to other standalone-capable projects' CMakeLists.txt also (LLD in particular, for my interests). Actually, it seems there's nothing in LLD that requires _GNU_SOURCE on Cygwin... |
Yep, indeed. I guess the main question is who others regularly build in standalone mode and is familiar with how this is supposed to work, cmake-wise. CC @mgorny @tstellar who seem to have touched things for standalone build mode, and CC @petrhosek for overall cmake review. |
|
I'd slightly prefer doing this as two separate changes, that is the first change to replace the use of I agree that the second change, that is using |
Sounds reasonable!
Ok, so this is an oversight, and it just happens that none of the projects that support standalone builds have been built in configurations where the defines in |
Is there a list somewhere or do I just rely on my grep-foo?
grep also turned up these, but they don't seem like things that link to LLVM, so probably shouldn't have these defines.
|
|
Also, should all of those projects be updated in one pull request, or do I need to open a half-dozen more? |
Yes, this seems correct.
I think it's fine to do that kind of project-wide consistency fixes all in one pull request, especially if the change in each of the projects isn't very large and involved. |
|
Yeah, I think it's a good idea. Thanks for doing that! |
jeremyd2019
force-pushed
the
clang-respect-llvm-definitons
branch
from
a61d253 to
b145528
Compare
jeremyd2019
changed the title
|
I rebased this on top of #138783 and adjusted the title and description. Now it should be in a good state to push cmake changes for other projects. |
jeremyd2019
requested review from
JDevlieghere,
aaupov,
ayermolo,
maksfb,
rafaelauler and
yota9
as code owners
jeremyd2019
changed the title
The changes look good, but it looks like the changes from #138783 still show up when viewing the changes; can you check that you've rebased past the merged #138783? (Also, I take it that no other subprojects than clang need the |
In llvm#138329, _GNU_SOURCE was added for Cygwin, but when building Clang standalone against an installed LLVM this definition was not picked up, resulting in undefined strnlen. Follow the documentation in https://llvm.org/docs/CMake.html#developing-llvm-passes-out-of-source and add the LLVM_DEFINITIONS in standalone projects' cmakes.
jeremyd2019
force-pushed
the
clang-respect-llvm-definitons
branch
from
3078853 to
be29dee
Compare
I had not - I have now though.
No, the other projects were not messing with |
There was a problem hiding this comment.
LGTM
IIRC @petrhosek had commented on this before, and was generally in favour of it, but I'd still leave it open for a couple days if he wants to comment further on it.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/202/builds/1378 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/3735 Here is the relevant piece of the build log for the reference |
6 participants
In #138329, _GNU_SOURCE was added for Cygwin, but when building Clang standalone against an installed LLVM this definition was not picked up, resulting in undefined strnlen. Follow the documentation in https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project and add the LLVM_DEFINITIONS in standalone projects' cmakes.