Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: add more LLVM components #686

Closed
wants to merge 1 commit into from
Closed

Conversation

jirislaby
Copy link
Contributor

With -DUSE_CMAKE_FIND_PACKAGE_LLVM=ON, I have to specify these as they
are used by our libModule.

ipo
linker
mc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What part of mc do we depend on?

Copy link
Contributor Author

@jirislaby jirislaby Jun 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, you are right. I blindly added everything what linker screamed is missing. So non-llvm-config configuration is more broken than llvm-config?

Really, llvm_map_components_to_libnames returns less what it really should (opposing to llvm-config), it seems. No transitive closure of internal LLVM dependencies there. I'm at LLVM 4.

@@ -23,9 +23,14 @@ set(LLVM_COMPONENTS
bitreader
bitwriter
codegen
instcombine
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is wrong here. Both the llvm-config and CMake approach should see that transformutils is a dependency of instcombine.

If you take a look at the installed LLVMExports.cmake file you'll see something like

# Create imported target LLVMInstCombine
add_library(LLVMInstCombine STATIC IMPORTED)

set_target_properties(LLVMInstCombine PROPERTIES
  INTERFACE_LINK_LIBRARIES "LLVMAnalysis;LLVMCore;LLVMSupport;LLVMTransformUtils"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the difference is:

llvm_map_components_to_libraries(LLVM_LIBS1 instcombine)
llvm_map_components_to_libnames(LLVM_LIBS2 instcombine)

message("old ${LLVM_LIBS1}")
message("new ${LLVM_LIBS2}")
old LLVMInstCombine;LLVMTransformUtils;LLVMAnalysis;LLVMProfileData;LLVMObject;LLVMMCParser;LLVMMC;LLVMBitReader;LLVMCore;LLVMSupport;LLVMDemangle
new LLVMInstCombine

So we should use llvm_map_components_to_libnames differently, apparently :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works much better:

llvm_map_components_to_libraries(LLVM_LIBS1 instcombine)
llvm_map_components_to_libnames(LLVM_LIBS2_T instcombine)
llvm_expand_dependencies(LLVM_LIBS2 ${LLVM_LIBS2_T})

message("old ${LLVM_LIBS1}")
message("new ${LLVM_LIBS2}")
old LLVMInstCombine;LLVMTransformUtils;LLVMAnalysis;LLVMProfileData;LLVMObject;LLVMMCParser;LLVMMC;LLVMBitReader;LLVMCore;LLVMSupport;LLVMDemangle
new LLVMInstCombine;LLVMTransformUtils;LLVMAnalysis;LLVMProfileData;LLVMObject;LLVMMCParser;LLVMMC;LLVMBitReader;LLVMCore;LLVMSupport;LLVMDemangle

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this works this is the wrong approach. llvm_map_components_to_libnames() is the way you used above will only give back LLVMInstCombine. The LLVMInstCombine library will have INTERFACE dependencies which should transitively cover all the dependent libraries. If it does not then there is a bug somewhere which we should report upstream.

Using llvm_expand_dependencies() is fine for a temporary fix but a comment should be added stating such and it should point at the relevant LLVM bugs

I have another project which uses LLVM 4.0 via CMake on my system so I'll try to take a look today at where the problem is and report it upstream if I can figure out what's going on.

When we use -DUSE_CMAKE_FIND_PACKAGE_LLVM=ON, we do not expand
dependecies and build fails with linker errors like:
/usr/bin/ld: lib/libkleeModule.a(ModuleUtil.cpp.o): undefined reference to symbol '_ZNK4llvm6object7Archive5Child7getNameEv'
/usr/lib64/../lib64/libLLVMObject.so.4: error adding symbols: DSO missing from command line
clang-4.0.0: error: linker command failed with exit code 1 (use -v to see invocation)

So let LLVM compute also the transitive close as it used to do so in
LLVM < 3.5.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
@delcypher
Copy link
Contributor

Hmm LLVMObject is in the list of transitively expanded dependencies.

Adding some hacky code to walk the dependencies.

cmake_policy(SET CMP0057 NEW)
llvm_map_components_to_libnames(LLVM_DEP_HACK instcombine)
message(STATUS "LLVM_DEP_HACK: ${LLVM_DEP_HACK}")
set(_work_list ${LLVM_DEP_HACK})
set(_final_deps "")
set(_changed TRUE)
while(${_changed})
  set(_new_work_list "")
  set(_changed FALSE)
  foreach(dep ${_work_list})
    if (TARGET "${dep}")
      message(STATUS "Adding dep ${dep}")
      list(APPEND _final_deps "${dep}")
      # Get INTERFACE dependencies
      set(dep_interface_deps "")
      message(STATUS "Getting interface deps of target \"${dep}\"")
      get_target_property(dep_interface_deps ${dep} INTERFACE_LINK_LIBRARIES)
      message(STATUS "Deps are ${dep_interface_deps}")
      if (dep_interface_deps)
        list(LENGTH dep_interface_deps length_dep_interface_deps)
        if ("${length_dep_interface_deps}" GREATER 0)
          # Only append dependencies that we haven't seen bfore
          #message(STATUS "Append deps ${dep_interface_deps} from ${dep}")
          foreach(new_dep ${dep_interface_deps})
            if (TARGET ${new_dep})
              if (NOT ("${new_dep}" IN_LIST _final_deps))
                if (NOT ("${new_dep}" IN_LIST _new_work_list))
                  if (NOT ("${new_dep}" IN_LIST _work_list))
                    list(APPEND _new_work_list ${new_dep})
                    message(STATUS "Appending ${new_dep} to work list")
                    set(_changed TRUE)
                  endif()
                endif()
              endif()
            endif()
          endforeach()
        endif()
      endif()
    endif()
  endforeach()
  set(_work_list ${_new_work_list})
  unset(_new_work_list)
endwhile()
message(STATUS "transitive dependencies: ${_final_deps}")
-- LLVM_DEP_HACK: LLVMInstCombine
-- Adding dep LLVMInstCombine
-- Getting interface deps of target "LLVMInstCombine"
-- Deps are LLVMAnalysis;LLVMCore;LLVMSupport;LLVMTransformUtils
-- Appending LLVMAnalysis to work list
-- Appending LLVMCore to work list
-- Appending LLVMSupport to work list
-- Appending LLVMTransformUtils to work list
-- Adding dep LLVMAnalysis
-- Getting interface deps of target "LLVMAnalysis"
-- Deps are LLVMCore;LLVMObject;LLVMProfileData;LLVMSupport
-- Appending LLVMObject to work list
-- Appending LLVMProfileData to work list
-- Adding dep LLVMCore
-- Getting interface deps of target "LLVMCore"
-- Deps are LLVMSupport
-- Adding dep LLVMSupport
-- Getting interface deps of target "LLVMSupport"
-- Deps are rt;dl;curses;-lpthread;z;m;LLVMDemangle
-- Appending LLVMDemangle to work list
-- Adding dep LLVMTransformUtils
-- Getting interface deps of target "LLVMTransformUtils"
-- Deps are LLVMAnalysis;LLVMCore;LLVMSupport
-- Adding dep LLVMObject
-- Getting interface deps of target "LLVMObject"
-- Deps are LLVMBitReader;LLVMCore;LLVMMC;LLVMMCParser;LLVMSupport
-- Appending LLVMBitReader to work list
-- Appending LLVMMC to work list
-- Appending LLVMMCParser to work list
-- Adding dep LLVMProfileData
-- Getting interface deps of target "LLVMProfileData"
-- Deps are LLVMCore;LLVMSupport
-- Adding dep LLVMDemangle
-- Getting interface deps of target "LLVMDemangle"
-- Deps are dep_interface_deps-NOTFOUND
-- Adding dep LLVMBitReader
-- Getting interface deps of target "LLVMBitReader"
-- Deps are LLVMCore;LLVMSupport
-- Adding dep LLVMMC
-- Getting interface deps of target "LLVMMC"
-- Deps are LLVMSupport
-- Adding dep LLVMMCParser
-- Getting interface deps of target "LLVMMCParser"
-- Deps are LLVMMC;LLVMSupport
-- transitive dependencies: LLVMInstCombine;LLVMAnalysis;LLVMCore;LLVMSupport;LLVMTransformUtils;LLVMObject;LLVMProfileData;LLVMDemangle;LLVMBitReader;LLVMMC;LLVMMCParser

I need to see the link line that the generated build system uses when linking fails. Either libLLVMObject.a is missing entirely or there is an ordering problem.

@jirislaby
Copy link
Contributor Author

Should I provide the output?

-- LLVM_DEP_HACK: LLVMInstCombine
-- Adding dep LLVMInstCombine
-- Getting interface deps of target "LLVMInstCombine"
-- Deps are dep_interface_deps-NOTFOUND
-- transitive dependencies: LLVMInstCombine

@jirislaby
Copy link
Contributor Author

These are properties of LLVMInstCombine:

LLVMInstCombine BINARY_DIR = /tmp
LLVMInstCombine BINARY_DIR = /tmp
LLVMInstCombine BUILD_WITH_INSTALL_RPATH = OFF
LLVMInstCombine IMPORTED = TRUE
LLVMInstCombine IMPORTED_CONFIGURATIONS = RELWITHDEBINFO
LLVMInstCombine INSTALL_RPATH = 
LLVMInstCombine INSTALL_RPATH_USE_LINK_PATH = OFF
LLVMInstCombine NAME = LLVMInstCombine
LLVMInstCombine POSITION_INDEPENDENT_CODE = True
LLVMInstCombine SKIP_BUILD_RPATH = OFF
LLVMInstCombine SOURCE_DIR = /tmp
LLVMInstCombine SOURCE_DIR = /tmp
LLVMInstCombine TYPE = SHARED_LIBRARY
LLVMInstCombine TYPE = SHARED_LIBRARY

So there is no INTERFACE_LINK_LIBRARIES.

@delcypher
Copy link
Contributor

So there is no INTERFACE_LINK_LIBRARIES.
@jirislaby

Something about your set up is very different to mine.

I have a build of LLVM 4.0 on my system and in the build tree there is the file

lib/cmake/llvm/LLVMExports.cmake

and in that there are the following lines

# Create imported target LLVMInstCombine
add_library(LLVMInstCombine STATIC IMPORTED)

set_target_properties(LLVMInstCombine PROPERTIES
  INTERFACE_LINK_LIBRARIES "LLVMAnalysis;LLVMCore;LLVMSupport;LLVMTransformUtils"
)

the LLVMExports.cmake file is include by the lib/cmake/llvm/LLVMConfig.cmake file which access by doing find_package(LLVM CONFIG) from inside KLEE's build system.

Is your set up like this? If not where are the differences?

@jirislaby
Copy link
Contributor Author

I have this:

# Create imported target LLVMInstCombine
add_library(LLVMInstCombine SHARED IMPORTED)

I.e. my SHARED vs. your STATIC. And no set_target_properties in here.

@delcypher
Copy link
Contributor

delcypher commented Jul 20, 2017

I.e. my SHARED vs. your STATIC. And no set_target_properties in here.

Oh. So you're building LLVM with -DBUILD_SHARED_LIBS=ON. You know that's not recommended right? LLVM's own docs say so.

http://llvm.org/docs/CMake.html#llvm-specific-variables

BUILD_SHARED_LIBS is only recommended for use by LLVM developers.
If you want to build LLVM as a shared library, you should use the
LLVM_BUILD_LLVM_DYLIB option.

LLVM_BUILD_LLVM_DYLIB is preferred because that builds a single monolithic shared library rather than individual shared libraries.

This configuration (individual components built as individual shared libraries) probably doesn't get much testing. So to me this seems like a bug in LLVM's build system. The dependency information is missing from the exported targets which is not something we can really fix on KLEE's side. I suggest you report this bug upstream and cc @bradking (Brad King), myself, and @llvm-beanz (Chris Bieneman).

@jirislaby
Copy link
Contributor Author

jirislaby commented Jul 20, 2017

So you're building LLVM

It's not actually me. It is LLVM distributed by openSUSE :).

I suggest you report this bug upstream

Ok, thanks.

@delcypher
Copy link
Contributor

It's not actually me. It is LLVM distributed by openSUSE :).

Eurgh. They really shouldn't be doing that. So looks like you have two bug reports, one for openSUSE and one for upstream LLVM :)

Maybe openSUSE have a good reason for doing this but going against a project's recommended practice seems like a bad idea.

@jirislaby
Copy link
Contributor Author

@llvm-beanz
Copy link

When BUILD_SHARED_LIBS is on the LLVM build system changes interface dependencies to PRIVATE dependencies because shared objects track their dependencies inside themselves.

This can cause problems if the header files for an LLVM component have inlined code that calls into another component, and in that situation our transitive dependency information would be incomplete.

You can file a bug on LLVM.org to investigate fixing that, however it is unlikely that much attention will be given to the BUILD_SHARED_LIBS build because it is strongly discouraged.

@delcypher
Copy link
Contributor

@llvm-beanz Thanks for the confirmation. I'll leave it up to @jirislaby whether or not to file a bug for this. To me it seems like the problem is on the openSUSE side for going against recommended practice.

@jirislaby
Copy link
Contributor Author

The reply from openSUSE LLVM maintainers:

This can't be done until https://bugs.llvm.org/show_bug.cgi?id=22952 is fixed.

So LLVM_BUILD_LLVM_DYLIB is more broken than BUILD_SHARED_LIBS, apparently.

@llvm-beanz
Copy link

The change that caused that bug was altered, and while the issue with cl::opt exists, that problem shouldn't exist in any recent LLVM release or on trunk. So... don't think that is a legitimate answer from openSUSE unless they have modifications to LLVM that are causing other bugs which haven't been reported.

@jirislaby
Copy link
Contributor Author

openSUSE switched to LLVM_BUILD_LLVM_DYLIB finally.

@jirislaby jirislaby closed this Oct 10, 2017
@jirislaby jirislaby deleted the llvm-comp branch October 10, 2017 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants