Closed llvmbot closed 8 years ago
llvmbot
commented
8 years ago Fixed with the commit of http://llvm.org/viewvc/llvm-project?view=revision&revision=260641 and back port of http://llvm.org/viewvc/llvm-project?view=revision&revision=260693 to 3.8 branch
llvmbot
commented
8 years ago Moving the latest revision of the proposed patch to http://reviews.llvm.org/D16945.
llvmbot
commented
8 years ago Note that the new patch, which implements and uses --enable-llvm-link-llvm-dylib flag for utils/llvm-build/llvmbuild/main.py changes the tools/llvm-config/LibraryDependencies.inc file generated by llvm-build from containing...
{ "gtest", "libgtest.a", false, { "support" } }, { "gtest_main", "libgtest_main.a", false, { "gtest" } },
to
{ "gtest", "libgtest.a", false, { } }, { "gtest_main", "libgtest_main.a", false, { "gtest" } },
The current patch now looks like...
--- CMakeLists.txt (revision 259975) +++ CMakeLists.txt (working copy) @@ -460,9 +460,16 @@ set(LLVMOPTIONALCOMPONENTS ${LLVMOPTIONALCOMPONENTS} OProfileJIT) endif (LLVM_USE_OPROFILE)
+if (LLVM_LINK_LLVM_DYLIB)
set(ENABLE_LLVM_LINK_LLVM_DYLIB "--enable-llvm-link-llvm-dylib") +else()
set(ENABLE_LLVM_LINK_LLVM_DYLIB "") +endif()
message(STATUS "Constructing LLVMBuild project information") execute_process( COMMAND ${PYTHON_EXECUTABLE} ${LLVMBUILDTOOL}
--- cmake/modules/AddLLVM.cmake (revision 259975) +++ cmake/modules/AddLLVM.cmake (working copy) @@ -475,13 +475,15 @@
get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIBDEPS${name})
if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_STATIC AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
set(llvm_libs LLVM)
else()
llvm_map_components_to_libnames(llvm_libs
${ARG_LINK_COMPONENTS}
${LLVM_LINK_COMPONENTS}
)
if (DEFINED LLVM_LINK_COMPONENTS OR DEFINED ARG_LINK_COMPONENTS)
if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
set(llvm_libs LLVM)
else()
llvm_map_components_to_libnames(llvm_libs
${ARG_LINK_COMPONENTS}
${LLVM_LINK_COMPONENTS}
)
endif() endif()
if(CMAKE_VERSION VERSION_LESS 2.8.12) @@ -888,7 +890,6 @@ target_link_libraries(${test_name} gtest gtest_main
LLVMSupport # gtest needs it for raw_ostream. )
--- cmake/modules/LLVM-Config.cmake (revision 259975) +++ cmake/modules/LLVM-Config.cmake (working copy) @@ -40,10 +40,17 @@
#
if (DEFINED link_components AND DEFINED LLVM_DYLIB_COMPONENTS)
if("${LLVM_DYLIB_COMPONENTS}" STREQUAL "all")
set(link_components "")
else()
list(REMOVE_ITEM link_components ${LLVM_DYLIB_COMPONENTS})
endif()
endif()
target_link_libraries(${executable} LLVM)
endif()
--- utils/llvm-build/llvmbuild/main.py (revision 259975) +++ utils/llvm-build/llvmbuild/main.py (working copy) @@ -350,6 +350,10 @@ library_name = None is_installed = True
if (library_name == 'gtest') and use_llvm_link_llvm_dylib:
c.required_libraries.remove('Support')
# Get the component names of all the required libraries.
required_llvmconfig_component_names = [
self.component_info_map[dep].get_llvmconfig_component_name()@@ -907,6 +911,13 @@ help=("Enable the given space or semi-colon separated " "list of optional components"), action="store", default="")
group.add_option("", "--enable-llvm-link-llvm-dylib",
dest="llvm_link_llvm_dylib",
help=("Use libLLVM instead of LLVM component shared "
"libraries for linkage"),
action="store_true", default=False)
parser.add_option_group(group)
(opts, args) = parser.parse_args() @@ -937,6 +948,13 @@
project_info.validate_components()
global use_llvm_link_llvm_dylib
if opts.llvm_link_llvm_dylib:
use_llvm_link_llvm_dylib = True
else:
use_llvm_link_llvm_dylib = False
--- utils/unittest/CMakeLists.txt (revision 259975) +++ utils/unittest/CMakeLists.txt (working copy) @@ -32,10 +32,6 @@ add_definitions( -DGTEST_HAS_PTHREAD=0 ) endif()
-set(LIBS
--- utils/unittest/UnitTestMain/CMakeLists.txt (revision 259975) +++ utils/unittest/UnitTestMain/CMakeLists.txt (working copy) @@ -3,5 +3,4 @@
LINK_LIBS gtest
llvmbot
commented
8 years ago llvmbot
commented
8 years ago --- CMakeLists.txt (revision 259975) +++ CMakeLists.txt (working copy) @@ -460,9 +460,16 @@ set(LLVMOPTIONALCOMPONENTS ${LLVMOPTIONALCOMPONENTS} OProfileJIT) endif (LLVM_USE_OPROFILE)
+if (LLVM_LINK_LLVM_DYLIB)
message(STATUS "Constructing LLVMBuild project information") execute_process( COMMAND ${PYTHON_EXECUTABLE} ${LLVMBUILDTOOL}
--- utils/llvm-build/llvmbuild/main.py (revision 259975) +++ utils/llvm-build/llvmbuild/main.py (working copy) @@ -907,6 +907,13 @@ help=("Enable the given space or semi-colon separated " "list of optional components"), action="store", default="")
group.add_option("", "--enable-llvm-link-llvm-dylib",
dest="llvm_link_llvm_dylib",
help=("Use libLLVM instead of LLVM component shared "
"libraries for linkage"),
action="store_true", default=False)
parser.add_option_group(group)
(opts, args) = parser.parse_args() @@ -937,6 +944,14 @@
project_info.validate_components()
if opts.llvm_link_llvm_dylib:
use_llvm_link_llvm_dylib = True
print "--enable-llvm-link-llvm-dylib passed"
else:
use_llvm_link_llvm_dylib = False
print "--enable-llvm-link-llvm-dylib not passed"
if opts.print_tree: project_info.print_tree()
works here in passing the flag over. Now to puzzle out how to utilize it with these python scripts to suppress the LLVMSupport dependency for gtest.
llvmbot
commented
8 years ago We should be able to use this approach to get the setting of LLVM_LINK_LLVM_DYLIB into the llvm-build python scripts...
--- utils/llvm-build/llvmbuild/main.py (revision 259975) +++ utils/llvm-build/llvmbuild/main.py (working copy) @@ -907,6 +907,13 @@ help=("Enable the given space or semi-colon separated " "list of optional components"), action="store", default="")
parser.add_option_group(group)
--- CMakeLists.txt (revision 259975) +++ CMakeLists.txt (working copy) @@ -460,9 +460,16 @@ set(LLVMOPTIONALCOMPONENTS ${LLVMOPTIONALCOMPONENTS} OProfileJIT) endif (LLVM_USE_OPROFILE)
+if (LLVM_LINK_LLVM_DYLIB)
message(STATUS "Constructing LLVMBuild project information") execute_process( COMMAND ${PYTHON_EXECUTABLE} ${LLVMBUILDTOOL}
and then use it to override the 'required_libraries = Support' setting in LLVMBuild.txt for the gtest library.
llvmbot
commented
8 years ago It is unclear from the python trace if that code has access to the cmake option defines like DLLVM_LINK_LLVM_DYLIB. If it does, perhaps a check could be inserted to prune the Support dependency for that particular case.
llvmbot
commented
8 years ago --- CMakeLists.txt (revision 259875) +++ CMakeLists.txt (working copy) @@ -462,7 +462,7 @@
message(STATUS "Constructing LLVMBuild project information") execute_process(
will allow you to see the python code execution during the cmake run...
--- modulename: componentinfo, funcname: get_library_name componentinfo.py(176): return self.library_name or self.name componentinfo.py(190): if basename in ('gtest', 'gtest_main'): componentinfo.py(193): return 'LLVM%s' % basename main.py(348): is_installed = c.installed main.py(355): self.component_info_map[dep].get_llvmconfig_component_name() main.py(356): for dep in c.required_libraries] main.py(359): for dep in c.add_to_library_groups: main.py(363): entries[c.name] = (llvmconfig_component_name, library_name, main.py(364): required_llvmconfig_component_names, main.py(365): is_installed) main.py(325): for c in self.ordered_component_infos: main.py(327): if c.type_name == 'OptionalLibrary' \ main.py(332): tg = c.get_parent_target_group() --- modulename: componentinfo, funcname: get_parent_target_group componentinfo.py(82): if self.type_name == 'TargetGroup': componentinfo.py(86): if self.parent_instance: componentinfo.py(87): return self.parent_instance.get_parent_target_group() --- modulename: componentinfo, funcname: get_parent_target_group componentinfo.py(82): if self.type_name == 'TargetGroup': componentinfo.py(83): return self main.py(333): if tg and not tg.enabled: main.py(334): continue main.py(325): for c in self.ordered_component_infos: main.py(327): if c.type_name == 'OptionalLibrary' \ main.py(332): tg = c.get_parent_target_group() --- modulename: componentinfo, funcname: get_parent_target_group componentinfo.py(82): if self.type_name == 'TargetGroup': componentinfo.py(86): if self.parent_instance: componentinfo.py(87): return self.parent_instance.get_parent_target_group() --- modulename: componentinfo, funcname: get_parent_target_group componentinfo.py(82): if self.type_name == 'TargetGroup': componentinfo.py(86): if self.parent_instance: componentinfo.py(87): return self.parent_instance.get_parent_target_group() --- modulename: componentinfo, funcname: get_parent_target_group componentinfo.py(82): if self.type_name == 'TargetGroup': componentinfo.py(86): if self.parent_instance: main.py(333): if tg and not tg.enabled: main.py(337): if c.type_name not in ('Library', 'OptionalLibrary', \ main.py(338): 'LibraryGroup', 'TargetGroup'): main.py(343): llvmconfig_component_name = c.get_llvmconfig_component_name() --- modulename: componentinfo, funcname: get_llvmconfig_component_name componentinfo.py(196): return self.get_library_name().lower() --- modulename: componentinfo, funcname: get_library_name componentinfo.py(176): return self.library_name or self.name main.py(346): if c.type_name == 'Library' or c.type_name == 'OptionalLibrary': main.py(347): library_name = c.get_prefixed_library_name() --- modulename: componentinfo, funcname: get_prefixed_library_name componentinfo.py(186): basename = self.get_library_name() --- modulename: componentinfo, funcname: get_library_name componentinfo.py(176): return self.library_name or self.name componentinfo.py(190): if basename in ('gtest', 'gtest_main'): componentinfo.py(193): return 'LLVM%s' % basename main.py(348): is_installed = c.installed main.py(355): self.component_info_map[dep].get_llvmconfig_component_name() main.py(356): for dep in c.required_libraries] --- modulename: componentinfo, funcname: get_llvmconfig_component_name componentinfo.py(196): return self.get_library_name().lower() --- modulename: componentinfo, funcname: get_library_name componentinfo.py(176): return self.library_name or self.name
llvmbot
commented
8 years ago FYI, the original commit to hard-code in the linkages of LLVMSupport for gtest was...
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20101122/112439.html
The other one was committed in...
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20140210/204202.html
Lastly the utils/unittest/LLVMBuild.txt was added until...
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20111107/131728.html
My reading of these commits is that the first two, r120101 and r201079, automated the build system to always link LLVMSupport in with gtest. The third commit, r144445, simply added the LLVMBuild.txt with the normal format. However, the pre-existing automation of the linkage for LLVMSupport allows us to safely comment out the 'required_libraries = Support' line in LLVMBuild.txt for this particular case without concern.
llvmbot
commented
8 years ago FYI, the original commit to hard-code in the linkages of LLVMSupport for gtest was...
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20101122/112439.html
The other one was committed in...
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20140210/204202.html
Lastly the utils/unittest/LLVMBuild.txt was added until...
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20111107/131728.html
llvmbot
commented
8 years ago FYI, the original commit to hard-code in the linkages of LLVMSupport for gtest was...
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20101122/112439.html
The other one was committed in...
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20140210/204202.html
llvmbot
commented
8 years ago FYI, the original commit to hard-code in the linkages of LLVMSupport for gtest was...
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20101122/112439.html
llvmbot
commented
8 years ago Also tested a back port of the final complete patch, llvm/llvm-project#26767 _v5.patch, applied to current 3.8 branch and 3-stage bootstrapped (with stage2/stage3 file comparisons), of llvm/clang/clang-tools-extras/compiler-rt/polly/openmp/libc++ for the stock static, -DBUILD_SHARED_LIBS:BOOL=ON and -DLLVM_LINK_LLVM_DYLIB:BOOL=ON builds. All three builds bootstrap successfully and produce identical clean test suite results on x86_64 darwin of...
Expected Passes : 11648 Expected Failures : 51 Unsupported Tests : 3808 [100%] Built target check-llvm
Expected Passes : 8761 Expected Failures : 16 Unsupported Tests : 101 [100%] Built target check-clang
Expected Passes : 231 [100%] Built target check-clang-tools
Expected Passes : 571 Expected Failures : 17 Unsupported Tests : 7 [100%] Built target check-polly
Expected Passes : 5004 Expected Failures : 12 Unsupported Tests : 10 [100%] Built target check-libcxx
Expected Passes : 67 [100%] Built target check-libomp
llvmbot
commented
8 years ago It looks like all of the handling of add_to_library_groups is done in utils/llvm-build/llvmbuild/componentinfo.py and utils/llvm-build/llvmbuild/main.py so that is likely unusable outside of defining the association with a given LibraryGroup in explicit library declarations in LLVMBuild.txt files.
llvmbot
commented
8 years ago My reading of the documentation is that LibraryGroup's can only be defined with a 'add_to_library_groups' entry in each Library's LLVMBuild.txt. I don't see anything about associating individual libraries to common library group outside of the library definitions in LLVMBuild.txt files.
llvmbot
commented
8 years ago Index: cmake/modules/AddLLVM.cmake
--- cmake/modules/AddLLVM.cmake (revision 259799) +++ cmake/modules/AddLLVM.cmake (working copy) @@ -475,13 +475,15 @@
property has been set to an empty value.
get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIBDEPS${name})
- if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_STATIC AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
- set(llvm_libs LLVM)
- else()
- llvm_map_components_to_libnames(llvm_libs
- ${ARG_LINK_COMPONENTS}
- ${LLVM_LINK_COMPONENTS}
- )
- if (DEFINED LLVM_LINK_COMPONENTS OR DEFINED ARG_LINK_COMPONENTS)
- if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
- set(llvm_libs LLVM)
- else()
- llvm_map_components_to_libnames(llvm_libs
- ${ARG_LINK_COMPONENTS}
- ${LLVM_LINK_COMPONENTS}
- )
endif() endif()
if(CMAKE_VERSION VERSION_LESS 2.8.12) @@ -888,7 +890,6 @@ target_link_libraries(${test_name} gtest gtest_main
LLVMSupport # gtest needs it for raw_ostream. )
add_dependencies(${test_suite} ${test_name}) Index: cmake/modules/LLVM-Config.cmake
--- cmake/modules/LLVM-Config.cmake (revision 259799) +++ cmake/modules/LLVM-Config.cmake (working copy) @@ -40,10 +40,17 @@
done in case libLLVM does not contain all of the components
the target requires.
#
TODO strip LLVM_DYLIB_COMPONENTS out of link_components.
Strip LLVM_DYLIB_COMPONENTS out of link_components.
To do this, we need special handling for "all", since that
may imply linking to libraries that are not included in
libLLVM.
- if (DEFINED link_components AND DEFINED LLVM_DYLIB_COMPONENTS)
- if("${LLVM_DYLIB_COMPONENTS}" STREQUAL "all")
- set(link_components "")
- else()
- list(REMOVE_ITEM link_components ${LLVM_DYLIB_COMPONENTS})
- endif()
- endif()
target_link_libraries(${executable} LLVM) endif()Index: utils/unittest/CMakeLists.txt
--- utils/unittest/CMakeLists.txt (revision 259799) +++ utils/unittest/CMakeLists.txt (working copy) @@ -32,10 +32,6 @@ add_definitions( -DGTEST_HAS_PTHREAD=0 ) endif()
-set(LIBS
- LLVMSupport # Depends on llvm::raw_ostream -)
find_library(PTHREAD_LIBRARY_PATH pthread) if (PTHREAD_LIBRARY_PATH) list(APPEND LIBS pthread) @@ -45,6 +41,8 @@ googletest/src/gtest-all.cc
LINK_LIBS
- LLVM_COMPONENTS
- Support ${LIBS} )
Index: utils/unittest/UnitTestMain/CMakeLists.txt
--- utils/unittest/UnitTestMain/CMakeLists.txt (revision 259799) +++ utils/unittest/UnitTestMain/CMakeLists.txt (working copy) @@ -3,5 +3,4 @@
LINK_LIBS gtest
- LLVMSupport # Depends on llvm::cl )
doesn't work...
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 38
Unless I also use...
Indeed, the suggested change was unrelated to this bit.
Index: utils/unittest/LLVMBuild.txt
--- utils/unittest/LLVMBuild.txt (revision 259799) +++ utils/unittest/LLVMBuild.txt (working copy) @@ -19,7 +19,6 @@ type = Library name = gtest parent = Libraries -required_libraries = Support installed = 0
[component_1]
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 1
I don't see how you can ever leave 'required_libraries = Support' in that file. My understanding is that required_libraries refers to the base names of files and cmake has no way to know that LLVM contains the contents of Support. It just sees them as two different files.
That is how it currently works, I'm suggesting that we change that. Having "required_libraries = Support" should be equivalent to:
set(LLVM_LINK_COMPONENTS Support)
add_llvm_library(gtest ...)(which would link with libLLVM if LLVM_LINK_LLVM_DYLIB=ON)
but in fact it is doing:
add_llvm_library(gtest ...)
target_link_libraries(gtest LLVMSupport)(which will link with libLLVMSupport regardless of LLVM_LINK_LLVM_DYLIB).
I'm not sure what the correct way to do that is. I tried modifying llvm-build to just output the component names, and then add that to the llvm_map_components_to_libnames call in llvm_add_library, but now I'm getting circular dependency errors.
llvmbot
commented
8 years ago If I understand this correctly, your pruning of the instances of LLVMSupport would require us to find where the libLLVM is defined with a...
[component_0] type = Library name = LLVM
and add a
add_to_library_groups = containsSupport
then the file utils/unittest/LLVMBuild.txt would be changed from...
required_libraries = Support
to
required_libraries = containsSupport
llvmbot
commented
8 years ago docs/LLVMBuild.rst has...
LLVMBuild files are expected to define a strict set of sections and properties. A typical component description file for a library component would look like the following example:
.. code-block:: ini
[component_0] type = Library name = Linker parent = Libraries required_libraries = Archive BitReader Core Support TransformUtils ...
required_libraries [optional]
If given, a list of the names of Library or LibraryGroup
components which must also be linked in whenever this library is
used. That is, the link time dependencies for this component. When
tools are built, the build system will include the transitive closure
of all required_libraries for the components the tool needs.
Perhaps things could be refactored to use...
add_to_library_groups [optional]
If given, a list of the names of LibraryGroup components which
this component is also part of. This allows nesting groups of
components. For example, the X86 target might define a library
group for all of the X86 components. That library group might
then be included in the all-targets library group.
but that probably is a lot of work.
llvmbot
commented
8 years ago Also, my understanding of the previous complete patch was that all those instance of explicitly passing LLVMSupport that you want to prune were in fact making the use of that lib certain for gtest when 'required_libraries = Support' is removed from utils/unittest/LLVMBuild.txt. As I suspected, when 'required_libraries = Support' is removed from when the changes in comment #79 in order to avoid the 37 extra linkages on libSupport under -DLLVM_LINK_LLVM_DYLIB:BOOL=ON, the -DBUILD_SHARED_LIBS:BOOL=ON build no longer passes libLLVMSupport with libgtest...
/sw/src/fink.build/llvm39-3.9.0-1/opt-bin/ccclang++ -fno-common -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wdelete-non-virtual-dtor -std=c++11 -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -L/sw/lib -Wl,-dead_strip CMakeFiles/ClangApplyReplacementsTests.dir/ReformattingTest.cpp.o -o ClangApplyReplacementsTests ../../../../../../lib/libgtest.dylib ../../../../../../lib/libgtest_main.dylib ../../../../../../lib/libclangApplyReplacements.dylib ../../../../../../lib/libclangToolingCore.dylib -Wl,-rpath,@executable_path/../lib
llvmbot
commented
8 years ago Also, my understanding of the previous complete patch was that all those instance of explicitly passing LLVMSupport that you want to prune were in fact making the use of that lib certain for gtest when 'required_libraries = Support' is removed from utils/unittest/LLVMBuild.txt. As I suspected, when 'required_libraries = Support' is removed from when the changes in comment #79 in order to avoid the 37 extra linkages on libSupport under -DLLVM_LINK_LLVM_DYLIB:BOOL=ON, the -DBUILD_SHARED_LIBS:BOOL=ON build no longer passes libLLVMSupport with libgtest...
/sw/src/fink.build/llvm39-3.9.0-1/opt-bin/ccclang++ -fno-common -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wdelete-non-virtual-dtor -std=c++11 -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -L/sw/lib -Wl,-dead_strip CMakeFiles/ClangApplyReplacementsTests.dir/ReformattingTest.cpp.o -o ClangApplyReplacementsTests ../../../../../../lib/libgtest.dylib ../../../../../../lib/libgtest_main.dylib ../../../../../../lib/libclangApplyReplacements.dylib ../../../../../../lib/libclangToolingCore.dylib -Wl,-rpath,@executable_path/../lib
llvmbot
commented
8 years ago --- cmake/modules/AddLLVM.cmake (revision 259799) +++ cmake/modules/AddLLVM.cmake (working copy) @@ -475,13 +475,15 @@
get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIBDEPS${name})
endif() endif()
if(CMAKE_VERSION VERSION_LESS 2.8.12) @@ -888,7 +890,6 @@ target_link_libraries(${test_name} gtest gtest_main
LLVMSupport # gtest needs it for raw_ostream. )
--- cmake/modules/LLVM-Config.cmake (revision 259799) +++ cmake/modules/LLVM-Config.cmake (working copy) @@ -40,10 +40,17 @@
#
--- utils/unittest/CMakeLists.txt (revision 259799) +++ utils/unittest/CMakeLists.txt (working copy) @@ -32,10 +32,6 @@ add_definitions( -DGTEST_HAS_PTHREAD=0 ) endif()
-set(LIBS
find_library(PTHREAD_LIBRARY_PATH pthread) if (PTHREAD_LIBRARY_PATH) list(APPEND LIBS pthread) @@ -45,6 +41,8 @@ googletest/src/gtest-all.cc
LINK_LIBS
--- utils/unittest/UnitTestMain/CMakeLists.txt (revision 259799) +++ utils/unittest/UnitTestMain/CMakeLists.txt (working copy) @@ -3,5 +3,4 @@
LINK_LIBS gtest
doesn't work...
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 38
Unless I also use...
--- utils/unittest/LLVMBuild.txt (revision 259799) +++ utils/unittest/LLVMBuild.txt (working copy) @@ -19,7 +19,6 @@ type = Library name = gtest parent = Libraries -required_libraries = Support installed = 0
[component_1]
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 1
I don't see how you can ever leave 'required_libraries = Support' in that file. My understanding is that required_libraries refers to the base names of files and cmake has no way to know that LLVM contains the contents of Support. It just sees them as two different files.
llvmbot
commented
8 years ago Index: utils/unittest/CMakeLists.txt
--- utils/unittest/CMakeLists.txt (revision 259799) +++ utils/unittest/CMakeLists.txt (working copy) @@ -32,9 +32,11 @@ add_definitions( -DGTEST_HAS_PTHREAD=0 ) endif()
-set(LIBS
- LLVMSupport # Depends on llvm::raw_ostream -) +if (NOT LLVM_LINK_LLVM_DYLIB)
- set(LIBS
- LLVMSupport # Depends on llvm::raw_ostream
- ) +endif()
Rather than just dropping it altogether, instead add the following to the "add_llvm_library" below: ... LLVM_COMPONENTS Support ...
I don't understand what you want here. Do you mean?
add_llvm_library(gtest googletest/src/gtest-all.cc
LLVM_COMPONENTS Support ${LIBS} }
Not quite, see diff below (also, LLVM_COMPONENTS was a typo -- should be LINK_COMPONENTS).
instead of
add_llvm_library(gtest googletest/src/gtest-all.cc
LINK_LIBS ${LIBS} )
in utils/unittest/CMakeLists.txt? And what about the preceding instance of...
set(LIBS LLVMSupport # Depends on llvm::raw_ostream )
--- utils/unittest/CMakeLists.txt (revision 259475) +++ utils/unittest/CMakeLists.txt (working copy) @@ -32,10 +32,6 @@ add_definitions( -DGTEST_HAS_PTHREAD=0 ) endif()
-set(LIBS
find_library(PTHREAD_LIBRARY_PATH pthread) if (PTHREAD_LIBRARY_PATH) list(APPEND LIBS pthread) @@ -46,6 +42,9 @@
LINK_LIBS ${LIBS}
llvmbot
commented
8 years ago Eureka! This permutation works...
Great! Please see my comments below.
Index: cmake/modules/AddLLVM.cmake
--- cmake/modules/AddLLVM.cmake (revision 259799) +++ cmake/modules/AddLLVM.cmake (working copy) @@ -475,13 +475,15 @@
property has been set to an empty value.
get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIBDEPS${name})
- if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_STATIC AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
- set(llvm_libs LLVM)
- else()
- llvm_map_components_to_libnames(llvm_libs
- ${ARG_LINK_COMPONENTS}
- ${LLVM_LINK_COMPONENTS}
- )
- if (DEFINED LLVM_LINK_COMPONENTS OR DEFINED ARG_LINK_COMPONENTS)
- if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
- set(llvm_libs LLVM)
- else()
- llvm_map_components_to_libnames(llvm_libs
- ${ARG_LINK_COMPONENTS}
- ${LLVM_LINK_COMPONENTS}
- )
endif() endif()
if(CMAKE_VERSION VERSION_LESS 2.8.12) @@ -885,11 +887,18 @@ add_llvm_executable(${test_name} IGNORE_EXTERNALIZE_DEBUGINFO ${ARGN}) set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}) set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
- target_link_libraries(${test_name}
- gtest
- gtest_main
- LLVMSupport # gtest needs it for raw_ostream.
- )
- if (LLVM_LINK_LLVM_DYLIB)
- target_link_libraries(${test_name}
- gtest
- gtest_main
- )
- else()
- target_link_libraries(${test_name}
- gtest
- gtest_main
- LLVMSupport # gtest needs it for raw_ostream.
- )
- endif()
IIANM, it should be OK to unconditionally drop LLVMSupport here. gtest should depend on either libLLVM or libLLVMSupport, and adding gtest should add the dependencies.
add_dependencies(${test_suite} ${test_name}) get_target_property(test_suite_folder ${test_suite} FOLDER) Index: cmake/modules/LLVM-Config.cmake
--- cmake/modules/LLVM-Config.cmake (revision 259799) +++ cmake/modules/LLVM-Config.cmake (working copy) @@ -40,10 +40,17 @@
done in case libLLVM does not contain all of the components
# the target requires. #
TODO strip LLVM_DYLIB_COMPONENTS out of link_components.
Strip LLVM_DYLIB_COMPONENTS out of link_components.
To do this, we need special handling for "all", since that
may imply linking to libraries that are not included in
libLLVM.
- if (DEFINED link_components AND DEFINED LLVM_DYLIB_COMPONENTS)
- if("${LLVM_DYLIB_COMPONENTS}" STREQUAL "all")
- set(link_components "")
- else()
- list(REMOVE_ITEM link_components ${LLVM_DYLIB_COMPONENTS})
- endif()
- endif()
target_link_libraries(${executable} LLVM) endif()Index: utils/unittest/CMakeLists.txt
--- utils/unittest/CMakeLists.txt (revision 259799) +++ utils/unittest/CMakeLists.txt (working copy) @@ -32,9 +32,11 @@ add_definitions( -DGTEST_HAS_PTHREAD=0 ) endif()
-set(LIBS
- LLVMSupport # Depends on llvm::raw_ostream -) +if (NOT LLVM_LINK_LLVM_DYLIB)
- set(LIBS
- LLVMSupport # Depends on llvm::raw_ostream
- ) +endif()
Rather than just dropping it altogether, instead add the following to the "add_llvm_library" below: ... LLVM_COMPONENTS Support ...
I don't understand what you want here. Do you mean?
add_llvm_library(gtest googletest/src/gtest-all.cc
LLVM_COMPONENTS Support ${LIBS} }
instead of
add_llvm_library(gtest googletest/src/gtest-all.cc
LINK_LIBS ${LIBS} )
in utils/unittest/CMakeLists.txt? And what about the preceding instance of...
set(LIBS LLVMSupport # Depends on llvm::raw_ostream )
find_library(PTHREAD_LIBRARY_PATH pthread) if (PTHREAD_LIBRARY_PATH) Index: utils/unittest/LLVMBuild.txt
--- utils/unittest/LLVMBuild.txt (revision 259799) +++ utils/unittest/LLVMBuild.txt (working copy) @@ -19,7 +19,6 @@ type = Library name = gtest parent = Libraries -required_libraries = Support installed = 0
[component_1]
This bit I'm not so sure about. I know that changing this has the immediately desired effect, but I'm not sure if it's the right change.
gtest does require LLVMSupport, so I think instead of removing it here, we should change the llvm-build and CMake bits to do the libLLVM/component-lib thing.
Index: utils/unittest/UnitTestMain/CMakeLists.txt
--- utils/unittest/UnitTestMain/CMakeLists.txt (revision 259799) +++ utils/unittest/UnitTestMain/CMakeLists.txt (working copy) @@ -1,7 +1,16 @@ -add_llvm_library(gtest_main
TestMain.cpp +if (LLVM_LINK_LLVM_DYLIB)
add_llvm_library(gtest_main
TestMain.cpp
LINK_LIBS
gtest
LLVMSupport # Depends on llvm::cl
)
LINK_LIBS
gtest
) +else()
add_llvm_library(gtest_main
TestMain.cpp
LINK_LIBS
gtest
LLVMSupport # Depends on llvm::cl
) +endif()
Again, I think we should just be able to remove LLVMSupport here and rely on CMake to add the transitive dependencies.
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 1
llvmbot
commented
8 years ago Eureka! This permutation works...
Great! Please see my comments below.
Index: cmake/modules/AddLLVM.cmake
--- cmake/modules/AddLLVM.cmake (revision 259799) +++ cmake/modules/AddLLVM.cmake (working copy) @@ -475,13 +475,15 @@
property has been set to an empty value.
get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIBDEPS${name})
- if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_STATIC AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
- set(llvm_libs LLVM)
- else()
- llvm_map_components_to_libnames(llvm_libs
- ${ARG_LINK_COMPONENTS}
- ${LLVM_LINK_COMPONENTS}
- )
- if (DEFINED LLVM_LINK_COMPONENTS OR DEFINED ARG_LINK_COMPONENTS)
- if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
- set(llvm_libs LLVM)
- else()
- llvm_map_components_to_libnames(llvm_libs
- ${ARG_LINK_COMPONENTS}
- ${LLVM_LINK_COMPONENTS}
- )
endif() endif()
if(CMAKE_VERSION VERSION_LESS 2.8.12) @@ -885,11 +887,18 @@ add_llvm_executable(${test_name} IGNORE_EXTERNALIZE_DEBUGINFO ${ARGN}) set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}) set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
- target_link_libraries(${test_name}
- gtest
- gtest_main
- LLVMSupport # gtest needs it for raw_ostream.
- )
- if (LLVM_LINK_LLVM_DYLIB)
- target_link_libraries(${test_name}
- gtest
- gtest_main
- )
- else()
- target_link_libraries(${test_name}
- gtest
- gtest_main
- LLVMSupport # gtest needs it for raw_ostream.
- )
- endif()
IIANM, it should be OK to unconditionally drop LLVMSupport here. gtest should depend on either libLLVM or libLLVMSupport, and adding gtest should add the dependencies.
add_dependencies(${test_suite} ${test_name}) get_target_property(test_suite_folder ${test_suite} FOLDER) Index: cmake/modules/LLVM-Config.cmake
--- cmake/modules/LLVM-Config.cmake (revision 259799) +++ cmake/modules/LLVM-Config.cmake (working copy) @@ -40,10 +40,17 @@
done in case libLLVM does not contain all of the components
# the target requires. #
TODO strip LLVM_DYLIB_COMPONENTS out of link_components.
Strip LLVM_DYLIB_COMPONENTS out of link_components.
To do this, we need special handling for "all", since that
may imply linking to libraries that are not included in
libLLVM.
- if (DEFINED link_components AND DEFINED LLVM_DYLIB_COMPONENTS)
- if("${LLVM_DYLIB_COMPONENTS}" STREQUAL "all")
- set(link_components "")
- else()
- list(REMOVE_ITEM link_components ${LLVM_DYLIB_COMPONENTS})
- endif()
- endif()
target_link_libraries(${executable} LLVM) endif()Index: utils/unittest/CMakeLists.txt
--- utils/unittest/CMakeLists.txt (revision 259799) +++ utils/unittest/CMakeLists.txt (working copy) @@ -32,9 +32,11 @@ add_definitions( -DGTEST_HAS_PTHREAD=0 ) endif()
-set(LIBS
- LLVMSupport # Depends on llvm::raw_ostream -) +if (NOT LLVM_LINK_LLVM_DYLIB)
- set(LIBS
- LLVMSupport # Depends on llvm::raw_ostream
- ) +endif()
Rather than just dropping it altogether, instead add the following to the "add_llvm_library" below: ... LLVM_COMPONENTS Support ...
find_library(PTHREAD_LIBRARY_PATH pthread) if (PTHREAD_LIBRARY_PATH) Index: utils/unittest/LLVMBuild.txt
--- utils/unittest/LLVMBuild.txt (revision 259799) +++ utils/unittest/LLVMBuild.txt (working copy) @@ -19,7 +19,6 @@ type = Library name = gtest parent = Libraries -required_libraries = Support installed = 0
[component_1]
This bit I'm not so sure about. I know that changing this has the immediately desired effect, but I'm not sure if it's the right change.
gtest does require LLVMSupport, so I think instead of removing it here, we should change the llvm-build and CMake bits to do the libLLVM/component-lib thing.
Index: utils/unittest/UnitTestMain/CMakeLists.txt
--- utils/unittest/UnitTestMain/CMakeLists.txt (revision 259799) +++ utils/unittest/UnitTestMain/CMakeLists.txt (working copy) @@ -1,7 +1,16 @@ -add_llvm_library(gtest_main
TestMain.cpp +if (LLVM_LINK_LLVM_DYLIB)
add_llvm_library(gtest_main
TestMain.cpp
LINK_LIBS
gtest
LLVMSupport # Depends on llvm::cl
)
LINK_LIBS
gtest
) +else()
add_llvm_library(gtest_main
TestMain.cpp
LINK_LIBS
gtest
LLVMSupport # Depends on llvm::cl
) +endif()
Again, I think we should just be able to remove LLVMSupport here and rely on CMake to add the transitive dependencies.
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 1
llvmbot
commented
8 years ago Also confirmed that a 3-stage bootstrap (with stage2/stage3 file comparisons), using current llvm/clang/clang-tools-extras/compiler-rt/polly/openmp/libc++ from trunk and the complete llvm/llvm-project#26767 _v5.patch, on x86_64-apple-darwin15 produces identical test suite results for the stock static build as was seen with -DBUILD_SHARED_LIBS:BOOL=ON and -.DLLVM_LINK_LLVM_DYLIB:BOOL=ON builds (comment #73).
llvmbot
commented
8 years ago A quick check using the same trunk pull and the complete patch on x86_64-apple-darwin13 but with -DBUILD_SHARED_LIBS:BOOL=ON instead of -DLLVM_LINK_LLVM_DYLIB:BOOL=ON suggests that the removal of 'required_libraries = Support' from utils/unittest/LLVMBuild.txt isn't an issue...
/sw/src/fink.build/llvm39-3.9.0-1/opt-bin/ccclang++ -fno-common -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wdelete-non-virtual-dtor -std=c++11 -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -L/sw/lib -Wl,-dead_strip CMakeFiles/ClangApplyReplacementsTests.dir/ReformattingTest.cpp.o -o ClangApplyReplacementsTests ../../../../../../lib/libgtest.dylib ../../../../../../lib/libgtest_main.dylib ../../../../../../lib/libLLVMSupport.dylib ../../../../../../lib/libclangApplyReplacements.dylib ../../../../../../lib/libclangToolingCore.dylib -Wl,-rpath,@executable_path/../lib
as the unit tests have no problems in retaining the LLVMSupport linkage.
Same test of trunk with the complete patch on x86_64-apple-darwin15 with stock build (no -DBUILD_SHARED_LIBS:BOOL=ON or -DLLVM_LINK_LLVM_DYLIB:BOOL=ON options) shows the expected libLLVMSupport static linkages...
/sw/src/fink.build/llvm39-3.9.0-1/opt-bin/ccclang++ -fno-common -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -std=c++11 -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -L/sw/lib -Wl,-dead_strip CMakeFiles/ClangApplyReplacementsTests.dir/ReformattingTest.cpp.o -o ClangApplyReplacementsTests ../../../../../../lib/libgtest.a ../../../../../../lib/libgtest_main.a ../../../../../../lib/libLLVMSupport.a ../../../../../../lib/libclangApplyReplacements.a ../../../../../../lib/libclangToolingCore.a ../../../../../../lib/libgtest.a ../../../../../../lib/libclangAST.a ../../../../../../lib/libclangRewrite.a ../../../../../../lib/libclangLex.a ../../../../../../lib/libclangBasic.a ../../../../../../lib/libLLVMCore.a ../../../../../../lib/libLLVMMC.a ../../../../../../lib/libLLVMSupport.a -lcurses -lpthread -lz -lm -Wl,-rpath,@executable_path/../lib
llvmbot
commented
8 years ago A 3-stage bootstrap (with stage2/stage3 file comparisons), using current llvm/clang/clang-tools-extras/compiler-rt/polly/openmp/libc++ from trunk and the complete llvm/llvm-project#26767 _v5.patch, on x86_64-apple-darwin13 using -DBUILD_SHARED_LIBS:BOOL=ON and on x86_64-apple-darwin15 using -DLLVM_LINK_LLVM_DYLIB:BOOL=ON both produce identical test suite results of...
Failing Tests (3): LLVM :: tools/lto/hide-linkonce-odr.ll LLVM :: tools/lto/opt-level.ll LLVM :: tools/sanstats/elf.test
Expected Passes : 11765 Expected Failures : 55 Unsupported Tests : 3852 Unexpected Failures: 3
Expected Passes : 8875 Expected Failures : 16 Unsupported Tests : 113 [100%] Built target check-clang
Expected Passes : 238 [100%] Built target check-clang-tools
Expected Passes : 581 Expected Failures : 17 Unsupported Tests : 7 [100%] Built target check-polly
Expected Passes : 5013 Expected Failures : 12 Unsupported Tests : 10 [100%] Built target check-libcxx
Expected Passes : 67 [100%] Built target check-libomp
llvmbot
commented
8 years ago Perhaps we ought to put a comment in utils/unittest/LLVMBuild.txt to remind folks that 'required_libraries = Support' shouldn't be reinserted.
llvmbot
commented
8 years ago A quick check using the same trunk pull and the complete patch on x86_64-apple-darwin13 but with -DBUILD_SHARED_LIBS:BOOL=ON instead of -DLLVM_LINK_LLVM_DYLIB:BOOL=ON suggests that the removal of 'required_libraries = Support' from utils/unittest/LLVMBuild.txt isn't an issue...
/sw/src/fink.build/llvm39-3.9.0-1/opt-bin/ccclang++ -fno-common -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wdelete-non-virtual-dtor -std=c++11 -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -L/sw/lib -Wl,-dead_strip CMakeFiles/ClangApplyReplacementsTests.dir/ReformattingTest.cpp.o -o ClangApplyReplacementsTests ../../../../../../lib/libgtest.dylib ../../../../../../lib/libgtest_main.dylib ../../../../../../lib/libLLVMSupport.dylib ../../../../../../lib/libclangApplyReplacements.dylib ../../../../../../lib/libclangToolingCore.dylib -Wl,-rpath,@executable_path/../lib
as the unit tests have no problems in retaining the LLVMSupport linkage.
llvmbot
commented
8 years ago llvmbot
commented
8 years ago Eureka! This permutation works...
I did a few permutations pruning of each the new sections out of the working patch and I am pretty sure that we need every part of it to eliminate the stray libLLVMSupport.a linkages.
Reconfirmed that the section of the patch...
--- utils/unittest/LLVMBuild.txt (revision 259799) +++ utils/unittest/LLVMBuild.txt (working copy) @@ -19,7 +19,6 @@ type = Library name = gtest parent = Libraries -required_libraries = Support installed = 0
[component_1]
is essential. Removing goes back to...
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 38
llvmbot
commented
8 years ago Eureka! This permutation works...
I did a few permutations pruning of each the new sections out of the working patch and I am pretty sure that we need every part of it to eliminate the stray libLLVMSupport.a linkages.
llvmbot
commented
8 years ago Eureka! This permutation works...
--- cmake/modules/AddLLVM.cmake (revision 259799) +++ cmake/modules/AddLLVM.cmake (working copy) @@ -475,13 +475,15 @@
get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIBDEPS${name})
endif() endif()
if(CMAKE_VERSION VERSION_LESS 2.8.12) @@ -885,11 +887,18 @@ add_llvm_executable(${test_name} IGNORE_EXTERNALIZE_DEBUGINFO ${ARGN}) set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}) set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
endif()
--- cmake/modules/LLVM-Config.cmake (revision 259799) +++ cmake/modules/LLVM-Config.cmake (working copy) @@ -40,10 +40,17 @@
#
--- utils/unittest/CMakeLists.txt (revision 259799) +++ utils/unittest/CMakeLists.txt (working copy) @@ -32,9 +32,11 @@ add_definitions( -DGTEST_HAS_PTHREAD=0 ) endif()
-set(LIBS
LLVMSupport # Depends on llvm::raw_ostream -) +if (NOT LLVM_LINK_LLVM_DYLIB)
set(LIBS
LLVMSupport # Depends on llvm::raw_ostream
) +endif()
--- utils/unittest/LLVMBuild.txt (revision 259799) +++ utils/unittest/LLVMBuild.txt (working copy) @@ -19,7 +19,6 @@ type = Library name = gtest parent = Libraries -required_libraries = Support installed = 0
--- utils/unittest/UnitTestMain/CMakeLists.txt (revision 259799) +++ utils/unittest/UnitTestMain/CMakeLists.txt (working copy) @@ -1,7 +1,16 @@ -add_llvm_library(gtest_main
TestMain.cpp +if (LLVM_LINK_LLVM_DYLIB)
add_llvm_library(gtest_main
TestMain.cpp
LINK_LIBS
gtest
LLVMSupport # Depends on llvm::cl
)
LINK_LIBS
gtest
) +else()
add_llvm_library(gtest_main
TestMain.cpp
LINK_LIBS
gtest
LLVMSupport # Depends on llvm::cl
) +endif()
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 1
llvmbot
commented
8 years ago Andrew, perhaps you should just go ahead and commit the current patch from comment #50 into trunk and we can open a new bug report for this unpruned linkage of libLLVMSupport.a when libgtest is emitted? The handling of libgtest and its LLVM support lib dependency in cmake looks like a real rat's nest.
llvmbot
commented
8 years ago Andrew, what happens when you run cmake with the -DLLVM_LINK_LLVM_DYLIB:BOOL=ON option and then execute 'find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport' (or find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.so | grep -c libLLVMSupport on linux)? Do you really see a reduction in the count of libLLVM linkages that also pass libLLVMSupport.a with your proposed changes?
llvmbot
commented
8 years ago are due to this shift in libLLVM.dylib to the back of the linkage exposing the latent issue of the extra linkage of libLLVMSupport.a with libgtest.a. Won't the symbols in LLVMSupport be resolved from the libLLVM.dylib when it is linked in before the static libLLVMSupport.a while moving the linkage of libLLVM.dylib to the end would favor the symbols from the static lib?
This sounds plausible.
When I run "check-clang", all is well. But...
$ LD_LIBRARY_PATH=lib ./tools/clang/unittests/Tooling/ToolingTests
ToolingTests: /home/andrew/code/src/llvm.org/llvm/lib/Support/CommandLine.cpp:202: void {anonymous}::CommandLineParser::registerCategory(llvm::cl::OptionCategory): Assertion `std::count_if(RegisteredOptionCategories.begin(), RegisteredOptionCategories.end(), [cat](const OptionCategory Category) { return cat->getName() == Category->getName(); }) == 0 && "Duplicate option categories"' failed. Aborted (core dumped)If I rebuild ToolingTests without libLLVMSupport.a, then it runs fine. I don't know why check-clang is happy.
I found why libLLVMSupport.a is being added. In AddLLVM.cmake, in llvm_add_library, "lib_deps" is added as dependencies of built libraries. The value of lib_deps is extracted from LLVMBuild.txt. In utils/unittests/LLVMBuild.txt, gtest has "required_libraries: Support". Removing that line will remove libLLVMSupport.a from the link. Can you try removing that to see if it gets the tests to pass?
I don't enough about LLVMBuild.txt and its users to know what the right thing to do is, though. There seems to be a lot of duplication of dependencies between CMake and LLVMBuild.txt. Just removing the lib_deps code from AddLLVM.cmake doesn't work, though; that causes build failures.
Removing 'required_libraries = Support' from utils/unittest/LLVMBuild.txt unfortunately doesn't impact the results...
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 38
nor does also adding in...
@@ -885,11 +887,18 @@ add_llvm_executable(${test_name} IGNORE_EXTERNALIZE_DEBUGINFO ${ARGN}) set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}) set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
llvmbot
commented
8 years ago I would also point out the the patch from comment #46, which places libLLVM.dylib in the front, and the patch from comment #50, which places libLLVM.dylib at the end of the linkage, both produce identical levels of pruning of LLVM static libs according to...
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 38
My assumption here is that the failures I am now seeing on x86_64 darwin in...
Clang-Unit ::Tooling/ToolingTests/addTargetAndModeForProgramName.AddsTargetAndMode Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.IgnoresExistingMode Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.IgnoresExistingTarget Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.PathIgnored
are due to this shift in libLLVM.dylib to the back of the linkage exposing the latent issue of the extra linkage of libLLVMSupport.a with libgtest.a. Won't the symbols in LLVMSupport be resolved from the libLLVM.dylib when it is linked in before the static libLLVMSupport.a while moving the linkage of libLLVM.dylib to the end would favor the symbols from the static lib?
This sounds plausible.
When I run "check-clang", all is well. But...
$ LD_LIBRARY_PATH=lib ./tools/clang/unittests/Tooling/ToolingTests ToolingTests: /home/andrew/code/src/llvm.org/llvm/lib/Support/CommandLine.cpp:202: void {anonymous}::CommandLineParser::registerCategory(llvm::cl::OptionCategory): Assertion `std::count_if(RegisteredOptionCategories.begin(), RegisteredOptionCategories.end(), [cat](const OptionCategory Category) { return cat->getName() == Category->getName(); }) == 0 && "Duplicate option categories"' failed. Aborted (core dumped)
If I rebuild ToolingTests without libLLVMSupport.a, then it runs fine. I don't know why check-clang is happy.
I found why libLLVMSupport.a is being added. In AddLLVM.cmake, in llvm_add_library, "lib_deps" is added as dependencies of built libraries. The value of lib_deps is extracted from LLVMBuild.txt. In utils/unittests/LLVMBuild.txt, gtest has "required_libraries: Support". Removing that line will remove libLLVMSupport.a from the link. Can you try removing that to see if it gets the tests to pass?
I don't enough about LLVMBuild.txt and its users to know what the right thing to do is, though. There seems to be a lot of duplication of dependencies between CMake and LLVMBuild.txt. Just removing the lib_deps code from AddLLVM.cmake doesn't work, though; that causes build failures.
llvmbot
commented
8 years ago I would also point out the the patch from comment #46, which places libLLVM.dylib in the front, and the patch from comment #50, which places libLLVM.dylib at the end of the linkage, both produce identical levels of pruning of LLVM static libs according to...
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 38
llvmbot
commented
8 years ago My assumption here is that the failures I am now seeing on x86_64 darwin in...
Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.AddsTargetAndMode
Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.IgnoresExistingMode
Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.IgnoresExistingTarget
Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.PathIgnoredare due to this shift in libLLVM.dylib to the back of the linkage exposing the latent issue of the extra linkage of libLLVMSupport.a with libgtest.a. Won't the symbols in LLVMSupport be resolved from the libLLVM.dylib when it is linked in before the static libLLVMSupport.a while moving the linkage of libLLVM.dylib to the end would favor the symbols from the static lib?
llvmbot
commented
8 years ago Also looking at unpatched trunk built using -DBUILD_SHARED_LIBS:BOOL=ON, the clang driver linkage still has the LLVM shared libs placed before those of the clang shared libs.
llvmbot
commented
8 years ago putting the AND NOT ARG_STATIC back in...
--- cmake/modules/AddLLVM.cmake (revision 259743) +++ cmake/modules/AddLLVM.cmake (working copy) @@ -475,13 +475,15 @@
get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIBDEPS${name})
endif() endif()
if(CMAKE_VERSION VERSION_LESS 2.8.12)
moves the linkage of libLLVM,dylib back up to the front but results in 14 instances of missed pruning of static libs...
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 52 $ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep libgtest | grep -c libLLVMSupport 37
llvmbot
commented
8 years ago I compared it to the clang driver linkage from 3.7.1 built with -DBUILD_SHARED_LIBS:BOOL=ON...
/sw/src/fink.build/llvm37-3.7.1-1/opt-bin/ccclang++ -fno-common -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -std=c++11 -fno-common -Woverloaded-virtual -fno-strict-aliasing -Wno-nested-anon-types -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -L/sw/lib CMakeFiles/clang.dir/driver.cpp.o CMakeFiles/clang.dir/cc1_main.cpp.o CMakeFiles/clang.dir/cc1as_main.cpp.o -o ../../../../bin/clang-3.7 ../../../../lib/libLLVMX86CodeGen.3.7.1.dylib ../../../../lib/libLLVMX86AsmPrinter.3.7.1.dylib ../../../../lib/libLLVMX86AsmParser.3.7.1.dylib ../../../../lib/libLLVMX86Desc.3.7.1.dylib ../../../../lib/libLLVMX86Info.3.7.1.dylib ../../../../lib/libLLVMX86Disassembler.3.7.1.dylib ../../../../lib/libLLVMPowerPCCodeGen.3.7.1.dylib ../../../../lib/libLLVMPowerPCAsmPrinter.3.7.1.dylib ../../../../lib/libLLVMPowerPCAsmParser.3.7.1.dylib ../../../../lib/libLLVMPowerPCDesc.3.7.1.dylib ../../../../lib/libLLVMPowerPCInfo.3.7.1.dylib ../../../../lib/libLLVMPowerPCDisassembler.3.7.1.dylib ../../../../lib/libLLVMARMCodeGen.3.7.1.dylib ../../../../lib/libLLVMARMAsmPrinter.3.7.1.dylib ../../../../lib/libLLVMARMAsmParser.3.7.1.dylib ../../../../lib/libLLVMARMDesc.3.7.1.dylib ../../../../lib/libLLVMARMInfo.3.7.1.dylib ../../../../lib/libLLVMARMDisassembler.3.7.1.dylib ../../../../lib/libLLVMAnalysis.3.7.1.dylib ../../../../lib/libLLVMCodeGen.3.7.1.dylib ../../../../lib/libLLVMCore.3.7.1.dylib ../../../../lib/libLLVMipa.3.7.1.dylib ../../../../lib/libLLVMipo.3.7.1.dylib ../../../../lib/libLLVMInstCombine.3.7.1.dylib ../../../../lib/libLLVMInstrumentation.3.7.1.dylib ../../../../lib/libLLVMMC.3.7.1.dylib ../../../../lib/libLLVMMCParser.3.7.1.dylib ../../../../lib/libLLVMObjCARCOpts.3.7.1.dylib ../../../../lib/libLLVMOption.3.7.1.dylib ../../../../lib/libLLVMScalarOpts.3.7.1.dylib ../../../../lib/libLLVMSupport.3.7.1.dylib ../../../../lib/libLLVMTransformUtils.3.7.1.dylib ../../../../lib/libLLVMVectorize.3.7.1.dylib ../../../../lib/libclangBasic.3.7.1.dylib ../../../../lib/libclangCodeGen.3.7.1.dylib ../../../../lib/libclangDriver.3.7.1.dylib ../../../../lib/libclangFrontend.3.7.1.dylib ../../../../lib/libclangFrontendTool.3.7.1.dylib -Wl,-sectcreate,TEXT,info_plist,/sw/src/fink.build/llvm37-3.7.1-1/build/stage1/tools/clang/tools/driver/Info.plist -Wl,-rpath,@executable_path/../lib
which has the LLVM components in front of the clang ones.
llvmbot
commented
8 years ago So the current permutation is okay for trunk (its already in stage 2 of the bootstrap here)?
LGTM. I think it's ready to commit.
Unfortunately, there seems to be a glitch in this latest revision of the patch which I noticed due to regressions...
Clang-Unit ::Tooling/ToolingTests/addTargetAndModeForProgramName.AddsTargetAndMode Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.IgnoresExistingMode Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.IgnoresExistingTarget Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.PathIgnored
Expected Passes : 8869 Expected Failures : 16 Unsupported Tests : 113 Unexpected Failures: 4
The previous patch from comment #46, which produced no clang test suite regressions, has the compiler linked with properly libLLVM.dylib in the front of the linkage...
/sw/src/fink.build/llvm39-3.9.0-1/opt-bin/ccclang++ -fno-common -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -std=c++11 -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -L/sw/lib -rdynamic CMakeFiles/clang.dir/driver.cpp.o CMakeFiles/clang.dir/cc1_main.cpp.o CMakeFiles/clang.dir/cc1as_main.cpp.o -o ../../../../bin/clang-3.9 ../../../../lib/libLLVM.dylib ../../../../lib/libclangBasic.a ../../../../lib/libclangCodeGen.a ../../../../lib/libclangDriver.a ../../../../lib/libclangFrontend.a ../../../../lib/libclangFrontendTool.a -Wl,-sectcreate,TEXT,info_plist,/sw/src/fink.build/llvm39-3.9.0-1/build/ stage1/tools/clang/tools/driver/Info.plist ../../../../lib/libclangCodeGen.a ../../../../lib/libclangRewriteFrontend.a ../../../../lib/libclangARCMigrate.a ../../../../lib/libclangStaticAnalyzerFrontend.a ../../../../lib/libclangFrontend.a ../../../../lib/libclangDriver.a ../../../../lib/libclangParse.a ../../../../lib/libclangSerialization.a ../../../../lib/libclangSema.a ../../../../lib/libclangEdit.a ../../../../lib/libclangStaticAnalyzerCheckers.a ../../../../lib/libclangStaticAnalyzerCore.a ../../../../lib/libclangAnalysis.a ../../../../lib/libclangAST.a ../../../../lib/libclangRewrite.a ../../../../lib/libclangLex.a ../../../../lib/libclangBasic.a -Wl,-rpath,@executable_path/../lib
whereas this new version of the patch, using the change from comment #50, has the linkage of libLLVN.dylib incorrectly moved to the end of the linkage as...
Why "incorrectly"? libclangX depend on the LLVM libraries, and dependencies go after the dependents on the link line? Can't see why that would cause these failures, and can't reproduce on Linux, sorry.
/sw/src/fink.build/llvm39-3.9.0-1/opt-bin/ccclang++ -fno-common -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -std=c++11 -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -L/sw/lib -rdynamic CMakeFiles/clang.dir/driver.cpp.o CMakeFiles/clang.dir/cc1_main.cpp.o CMakeFiles/clang.dir/cc1as_main.cpp.o -o ../../../../bin/clang-3.9 ../../../../lib/libclangBasic.a ../../../../lib/libclangCodeGen.a ../../../../lib/libclangDriver.a ../../../../lib/libclangFrontend.a ../../../../lib/libclangFrontendTool.a -Wl,-sectcreate,TEXT,info_plist,/sw/src/fink.build/llvm39-3.9.0-1/build/ stage1/tools/clang/tools/driver/Info.plist ../../../../lib/libclangCodeGen.a ../../../../lib/libclangRewriteFrontend.a ../../../../lib/libclangARCMigrate.a ../../../../lib/libclangStaticAnalyzerFrontend.a ../../../../lib/libclangFrontend.a ../../../../lib/libclangDriver.a ../../../../lib/libclangParse.a ../../../../lib/libclangSerialization.a ../../../../lib/libclangSema.a ../../../../lib/libclangEdit.a ../../../../lib/libclangStaticAnalyzerCheckers.a ../../../../lib/libclangStaticAnalyzerCore.a ../../../../lib/libclangAnalysis.a ../../../../lib/libclangAST.a ../../../../lib/libclangRewrite.a ../../../../lib/libclangLex.a ../../../../lib/libclangBasic.a ../../../../lib/libLLVM.dylib -Wl,-rpath,@executable_path/../lib
llvmbot
commented
8 years ago So the current permutation is okay for trunk (its already in stage 2 of the bootstrap here)?
LGTM. I think it's ready to commit.
Unfortunately, there seems to be a glitch in this latest revision of the patch which I noticed due to regressions...
Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.AddsTargetAndMode
Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.IgnoresExistingMode
Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.IgnoresExistingTarget
Clang-Unit :: Tooling/ToolingTests/addTargetAndModeForProgramName.PathIgnoredExpected Passes : 8869 Expected Failures : 16 Unsupported Tests : 113 Unexpected Failures: 4
The previous patch from comment #46, which produced no clang test suite regressions, has the compiler linked with properly libLLVM.dylib in the front of the linkage...
/sw/src/fink.build/llvm39-3.9.0-1/opt-bin/ccclang++ -fno-common -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -std=c++11 -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -L/sw/lib -rdynamic CMakeFiles/clang.dir/driver.cpp.o CMakeFiles/clang.dir/cc1_main.cpp.o CMakeFiles/clang.dir/cc1as_main.cpp.o -o ../../../../bin/clang-3.9 ../../../../lib/libLLVM.dylib ../../../../lib/libclangBasic.a ../../../../lib/libclangCodeGen.a ../../../../lib/libclangDriver.a ../../../../lib/libclangFrontend.a ../../../../lib/libclangFrontendTool.a -Wl,-sectcreate,TEXT,info_plist,/sw/src/fink.build/llvm39-3.9.0-1/build/stage1/tools/clang/tools/driver/Info.plist ../../../../lib/libclangCodeGen.a ../../../../lib/libclangRewriteFrontend.a ../../../../lib/libclangARCMigrate.a ../../../../lib/libclangStaticAnalyzerFrontend.a ../../../../lib/libclangFrontend.a ../../../../lib/libclangDriver.a ../../../../lib/libclangParse.a ../../../../lib/libclangSerialization.a ../../../../lib/libclangSema.a ../../../../lib/libclangEdit.a ../../../../lib/libclangStaticAnalyzerCheckers.a ../../../../lib/libclangStaticAnalyzerCore.a ../../../../lib/libclangAnalysis.a ../../../../lib/libclangAST.a ../../../../lib/libclangRewrite.a ../../../../lib/libclangLex.a ../../../../lib/libclangBasic.a -Wl,-rpath,@executable_path/../lib
whereas this new version of the patch, using the change from comment #50, has the linkage of libLLVN.dylib incorrectly moved to the end of the linkage as...
/sw/src/fink.build/llvm39-3.9.0-1/opt-bin/ccclang++ -fno-common -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -std=c++11 -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -Wl,-search_paths_first -Wl,-headerpad_max_install_names -L/sw/lib -rdynamic CMakeFiles/clang.dir/driver.cpp.o CMakeFiles/clang.dir/cc1_main.cpp.o CMakeFiles/clang.dir/cc1as_main.cpp.o -o ../../../../bin/clang-3.9 ../../../../lib/libclangBasic.a ../../../../lib/libclangCodeGen.a ../../../../lib/libclangDriver.a ../../../../lib/libclangFrontend.a ../../../../lib/libclangFrontendTool.a -Wl,-sectcreate,TEXT,info_plist,/sw/src/fink.build/llvm39-3.9.0-1/build/stage1/tools/clang/tools/driver/Info.plist ../../../../lib/libclangCodeGen.a ../../../../lib/libclangRewriteFrontend.a ../../../../lib/libclangARCMigrate.a ../../../../lib/libclangStaticAnalyzerFrontend.a ../../../../lib/libclangFrontend.a ../../../../lib/libclangDriver.a ../../../../lib/libclangParse.a ../../../../lib/libclangSerialization.a ../../../../lib/libclangSema.a ../../../../lib/libclangEdit.a ../../../../lib/libclangStaticAnalyzerCheckers.a ../../../../lib/libclangStaticAnalyzerCore.a ../../../../lib/libclangAnalysis.a ../../../../lib/libclangAST.a ../../../../lib/libclangRewrite.a ../../../../lib/libclangLex.a ../../../../lib/libclangBasic.a ../../../../lib/libLLVM.dylib -Wl,-rpath,@executable_path/../lib
llvmbot
commented
8 years ago So the current permutation is okay for trunk (its already in stage 2 of the bootstrap here)?
LGTM. I think it's ready to commit.
Index: cmake/modules/AddLLVM.cmake
--- cmake/modules/AddLLVM.cmake (revision 259743) +++ cmake/modules/AddLLVM.cmake (working copy) @@ -475,13 +475,15 @@
property has been set to an empty value.
get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIBDEPS${name})
- if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_STATIC AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
- set(llvm_libs LLVM)
- else()
- llvm_map_components_to_libnames(llvm_libs
- ${ARG_LINK_COMPONENTS}
- ${LLVM_LINK_COMPONENTS}
- )
- if (DEFINED LLVM_LINK_COMPONENTS OR DEFINED ARG_LINK_COMPONENTS)
- if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
- set(llvm_libs LLVM)
- else()
- llvm_map_components_to_libnames(llvm_libs
- ${ARG_LINK_COMPONENTS}
- ${LLVM_LINK_COMPONENTS}
- )
endif() endif()
if(CMAKE_VERSION VERSION_LESS 2.8.12) Index: cmake/modules/LLVM-Config.cmake
--- cmake/modules/LLVM-Config.cmake (revision 259743) +++ cmake/modules/LLVM-Config.cmake (working copy) @@ -40,10 +40,17 @@
done in case libLLVM does not contain all of the components
the target requires.
#
TODO strip LLVM_DYLIB_COMPONENTS out of link_components.
Strip LLVM_DYLIB_COMPONENTS out of link_components.
To do this, we need special handling for "all", since that
may imply linking to libraries that are not included in
libLLVM.
- if (DEFINED link_components AND DEFINED LLVM_DYLIB_COMPONENTS)
- if("${LLVM_DYLIB_COMPONENTS}" STREQUAL "all")
- set(link_components "")
- else()
- list(REMOVE_ITEM link_components ${LLVM_DYLIB_COMPONENTS})
- endif()
- endif()
target_link_libraries(${executable} LLVM) endif()
llvmbot
commented
8 years ago So the current permutation is okay for trunk (its already in stage 2 of the bootstrap here)?
--- cmake/modules/AddLLVM.cmake (revision 259743) +++ cmake/modules/AddLLVM.cmake (working copy) @@ -475,13 +475,15 @@
get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIBDEPS${name})
endif() endif()
--- cmake/modules/LLVM-Config.cmake (revision 259743) +++ cmake/modules/LLVM-Config.cmake (working copy) @@ -40,10 +40,17 @@
#
llvmbot
commented
8 years ago Okay. The extra linkage on libLLVMSupport for libclang.dylib no now gone with the revised patch.
We still have 37 instances of linkages of libLLVMSupport.a with libLLVM.dylib libgtest.a . Is it possible that the similar adjustments would solve that for the conditional in...
No, I've just had a look and this is due to a few things:
and
Having said that, I've made those changes and they're still in there. I don't have time to keep investigating this right now -- I think this should be tackled separately, since it's not causing any issues right now.
Index: cmake/modules/LLVM-Config.cmake
--- cmake/modules/LLVM-Config.cmake (revision 259743) +++ cmake/modules/LLVM-Config.cmake (working copy) @@ -40,10 +40,17 @@
done in case libLLVM does not contain all of the components
# the target requires. #
TODO strip LLVM_DYLIB_COMPONENTS out of link_components.
Strip LLVM_DYLIB_COMPONENTS out of link_components.
To do this, we need special handling for "all", since that
may imply linking to libraries that are not included in
libLLVM.
- if (DEFINED link_components AND DEFINED LLVM_DYLIB_COMPONENTS)
- if("${LLVM_DYLIB_COMPONENTS}" STREQUAL "all")
- set(link_components "")
- else()
- list(REMOVE_ITEM link_components ${LLVM_DYLIB_COMPONENTS})
- endif()
- endif()
target_link_libraries(${executable} LLVM) endif()
llvmbot
commented
8 years ago llvmbot
commented
8 years ago Okay. The extra linkage on libLLVMSupport for libclang.dylib no now gone with the revised patch.
We still have 37 instances of linkages of libLLVMSupport.a with libLLVM.dylib libgtest.a . Is it possible that the similar adjustments would solve that for the conditional in...
--- cmake/modules/LLVM-Config.cmake (revision 259743) +++ cmake/modules/LLVM-Config.cmake (working copy) @@ -40,10 +40,17 @@
# the target requires.
#Testing here on x86_64-apple-darwin15 using current trunk, thefollowing patch, currently attached in https://llvm.org/bugs/show_bug.cgi?id=26393#c40, eliminates all of the clang test suite regressions on trunk by eliminating the LLVM static lib linkages on its tools...
ndex: cmake/modules/AddLLVM.cmake
--- cmake/modules/AddLLVM.cmake (revision 259614) +++ cmake/modules/AddLLVM.cmake (working copy) @@ -477,7 +477,7 @@
if (LLVM_LINK_LLVM_DYLIB AND NOT ARG_STATIC AND NOT ARG_DISABLE_LLVM_LINK_LLVM_DYLIB) set(llvm_libs LLVM)
- else()
- elseif(NOT LLVM_LINK_LLVM_DYLIB OR ARG_DISABLE_LLVM_LINK_LLVM_DYLIB)
This change isn't quite right. Doing this means that we will add the component libraries as dependencies of static libraries only if LLVM_LINK_LLVM_DYLIB is disabled. If you have LLVM_LINK_LLVM_DYLIB enabled, dependent libraries will not be added to the interface of other static libraries.
e.g. If I require libclangBasic, I should also get libLLVMCore, libLLVMMC, and libLLVMSupport according to LLVM_LINK_COMPONENTS in clang/lib/Basic/CMakeLists.txt. That won't happen with this change.
Can you please test my suggestion in Comment 34? The ARG_STATIC in the "if(...)" above was my misguided approach to breaking the circular dependency of core component libraries and libLLVM. Instead of doing this, we can check if LLVM_LINK_COMPONENTS is set; only if it is set do we link to libLLVM. It is never set for the core component libraries.
The proposed change produces the same pruning of LLVM_LINK_COMPONENTS as the previous patch with the one change. My previous proposed patch produced...
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 38
and your proposed modification produces... $ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -c libLLVMSupport 39
So we have recovered one instance of libLLVMSupport being linked with this change.
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep libgtest | grep -c libLLVMSupport
37shows that all but two of these residual linkages against libLLVMSupport are due to the gtest linkages.
$ find . -name link.txt -print0 | xargs -0 cat | grep libLLVM.dylib | grep -v libgtest | grep libLLVMSupport
shows that the recovered linkage on libLLVMSupport is in the creation of libclang.dylib which makes sense as this change was required to handled clang's linkages on LLVM components.
Actually, I think that's a problem, and I see why now. My suggestion was incomplete: it needs to check for both ARG_LINK_COMPONENTS and LLVM_LINK_COMPONENTS. I'd just change it to:
--- cmake/modules/AddLLVM.cmake (revision 259475) +++ cmake/modules/AddLLVM.cmake (working copy) @@ -475,13 +475,15 @@
get_property(lib_deps GLOBAL PROPERTY LLVMBUILD_LIBDEPS${name})
So we have restrained the pruning by one instance of libLLVMSupport. The
llvm_map_components_to_libnames(llvm_libs ${ARG_LINK_COMPONENTS} ${LLVM_LINK_COMPONENTS}Index: cmake/modules/LLVM-Config.cmake
--- cmake/modules/LLVM-Config.cmake (revision 259614) +++ cmake/modules/LLVM-Config.cmake (working copy) @@ -40,10 +40,17 @@
done in case libLLVM does not contain all of the components
# the target requires. #
TODO strip LLVM_DYLIB_COMPONENTS out of link_components.
Strip LLVM_DYLIB_COMPONENTS out of link_components.
To do this, we need special handling for "all", since that
may imply linking to libraries that are not included in
libLLVM.
- if (DEFINED link_components AND DEFINED LLVM_DYLIB_COMPONENTS)
- if("${LLVM_DYLIB_COMPONENTS}" STREQUAL "all")
- set(link_components "")
- else()
- list(REMOVE_ITEM link_components ${LLVM_DYLIB_COMPONENTS})
- endif()
- endif()
target_link_libraries(${executable} LLVM) endif()The only remaining linkages of static LLVM components in executables are those of libLLVMSupport.a associated with the libgtest.a linkages. Since those aren't currently causing regressions, adding a TODO entry in cmake/modules/AddLLVM.cmake to either suppress the LLVMSupport on LLVM_LINK_LLVM_DYLIB or suppress LLVM for these particular test linkages should suffice.
Extended Description
Building llvm/clang/compiler-rt/polly/libc++/openmp with -DLLVM_LINK_LLVM_DYLIB:BOOL=ON on x86_64 results in many llvm test suite regressions.
Expected Passes : 7605 Expected Failures : 51 Unsupported Tests : 3841 Unexpected Failures: 3853
These all seem to be some variation of opt reporting an unknown command line argument being passed to it such as...
FAIL: LLVM :: Analysis/BasicAA/2003-02-26-AccessSizeTest.ll (1 of 15350) **** TEST 'LLVM :: Analysis/BasicAA/2003-02-26-AccessSizeTest.ll' FAILED **** Script:
gtimeout 1m /sw/src/fink.build/llvm39-3.9.0-1/build/stage3/./bin/opt < /sw/src/fink.build/llvm39-3.9.0-1/llvm-3.9.0.src/test/Analysis/BasicAA/2003-02-26-AccessSizeTest.ll -basicaa -gvn -instcombine -S | /sw/src/fink.build/llvm39-3.9.0-1/build/stage3/./bin/FileCheck /sw/src/fink.build/llvm39-3.9.0-1/llvm-3.9.0.src/test/Analysis/BasicAA/2003-02-26-AccessSizeTest.ll
Exit Code: 2
Command Output (stderr):
opt: Unknown command line argument '-basicaa'. Try: '/sw/src/fink.build/llvm39-3.9.0-1/build/stage3/./bin/opt -help' opt: Did you mean '-passes'? opt: Unknown command line argument '-gvn'. Try: '/sw/src/fink.build/llvm39-3.9.0-1/build/stage3/./bin/opt -help' opt: Did you mean '-Os'? opt: Unknown command line argument '-instcombine'. Try: '/sw/src/fink.build/llvm39-3.9.0-1/build/stage3/./bin/opt -help' opt: Did you mean '-print-options'? FileCheck error: '-' is empty.
--