Skip to content

CMake modernization#1391

Merged
hainest merged 240 commits intomasterfrom
cmake_modernization
Apr 25, 2023
Merged

CMake modernization#1391
hainest merged 240 commits intomasterfrom
cmake_modernization

Conversation

@hainest
Copy link
Copy Markdown
Contributor

@hainest hainest commented Mar 1, 2023

Fixes #1295
Fixes #1210
Fixes #1178
Fixes #1076
Closes #1341
Closes #1116
Closes #707
Closes #1100

Changes to building Dyninst

  • Minimum CMake version is 3.14.0
  • STERILE_BUILD is now deprecated
  • ENABLE_LTO was renamed to DYNINST_ENABLE_LTO
  • CMAKE_EXPORT_COMPILE_COMMANDS is no longer set
  • Platform detection is done natively in CMake
    • full support for Linux on x86, AMD64, ppc64le, and aarch64/ARMv8
    • experimental support for 32-bit FreeBSD and Windows on x86
  • Custom install targets <target>-install have been removed
  • Installation subpaths (bin, lib, include, etc.) are no longer user-configurable
  • Static versions of Dyninst libraries now depend on other static Dyninst libraries
    • For example, libDynElf.a now depends on libcommon.a, not libcommon.so
  • Libraries that cannot build with symlight now warn when LIGHTWEIGHT_SYMTAB=ON
  • User build options passed via CMAKE_<LANG>_FLAGS are correctly preserved and override the builtin options
  • <PackageName>_ROOT_DIR now implies <PackageName>_NO_SYSTEM_PATHS and sets <PackageName>_ROOT
    • This forces CMake to find the package at the given location or in CMAKE_PREFIX_PATH
    • When using CMake >= 3.16, searching CMAKE_PREFIX_PATH can be disabled with CMAKE_FIND_USE_CMAKE_PATH=OFF
    • For example, -DElfUtils_ROOT_DIR=/some/path sets ElfUtils_NO_SYSTEM_PATHS=ON and ElfUtils_ROOT=/some/path

RPATH handling

  • CMP0060 is active and so libraries are linked by their full paths even in implicit directories (e.g., /usr/lib/foo.so instead of -lfoo)
  • Populate RPATHs for binaries in the build tree: set(CMAKE_SKIP_BUILD_RPATH FALSE)
  • Do not use the install path as the RPATH: set(CMAKE_BUILD_WITH_INSTALL_RPATH FALSE)
    • $ORIGIN is used instead
  • Add paths to any directories outside the project that are in the linker search path or contain linked library files: set(CMAKE_INSTALL_RPATH_USE_LINK_PATH TRUE)

Third-party libraries (tpl)

Changes to consuming Dyninst as a CMake project

  • All targets are in the Dyninst namespace; e.g., symtabAPI is now Dyninst::symtabAPI
  • Each third-party library has an associated imported interface named Dyninst::<library>, e.g., Dyninst::Boost
    • Users are encouraged to use these, if the same library is need in their applications
  • DYNINST_LIBRARIES has been removed
  • DYNINST_INCLUDE_DIR is now deprecated and will be removed in a future version
    • Use the provided targets, instead
  • DYNINST_INTERNAL_DEFINES is now deprecated and will be removed in a future version
  • DYNINST_PLATFORM is now deprecated and will be removed in a future version
  • find_package(Dyninst ... COMPONENTS ...) now works correctly
  • Version constraints for find_package(Dyninst X.Y.Z) now work correctly
    • Dyninst only guarantees ABI compatibility between major releases, so only the same major versions are compatible

Users who are not ready to fully migrate to the new Dyninst CMake package may use the following to preserve backward compatibility:

if(TARGET Dyninst::common)
  foreach(t common symtabapi ...)
    add_library(${t} INTERFACE IMPORTED)
    target_link_libraries(${t} INTERFACE Dyninst::${t})
  endforeach()
endif()

hainest and others added 30 commits February 28, 2023 00:56
When consumed as a subproject, the CMake files could be imported into the parent project where the filenames could collide.
This should silence the warning about Thread_Db versus Thread_DB
* Make libdl/dbghelp private linkage

* Remove TBB flags from toolkits that don't use TBB

* Remove FindTBB.cmake

All supported TBB versions ship as CMake packages, so this is no longer
needed.

* Remove from-source build option

* Create an imported target for TBB used by Dyninst

This is needed to force the include directories to be considered 'system' directories so that compiler warnings from TBB sources are ignored

* Rename cmake/ThreadingBuildingBlocks.cmake -> cmake/tpls/DyninstTBB.cmake

This is needed to keep the namespace clean for DyninstConfig.cmake

* Export TBB as part of the Dyninst CMake package

This is required by the CMake guidelines:
  https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html

In particular,
  "All required dependencies of a package must also be found in the package configuration file"

* Format DyninstTBB.cmake
* Remove FindBoost.cmake

Use the one provided by CMake so we don't have to maintain this one.

* Remove from-source build

* Remove user-configurable version

* Force use of multithreaded libraries

We don't need to include Threads here. Boost will do that.

* Disable use of statically-linked runtime

* Remove Boost_DEBUG

* Always enable searching system paths

* Unify path calculations, pass to find_package as hints

* Clean up comments

* Use include_guard

* Allow using Boost's CMake package

It is now provided by default since 1.70.0 which is the current minimum
acceptable version.

* Remove rest of cache variables

* Remove MSVC-specific template define

We can add this back, if we find there are newer VCs still affected.

* Make Dyninst::Boost imported interface target

This is needed to make the include directories be "system" directories
so that warnings in their headers do not propagate into Dyninst.

* Do not add Boost as a dependency for all libs

* Make find_package QUIET

* Use Boost_* variables instead of calculating includes, libs, etc.

* Make a header-only wrapper target

* Add to Dyninst package

* Update CMakeLists

* Whitespace formatting

* Set Boost_NO_WARN_NEW_VERSIONS

* Bump minimum version to 1.71.0
* Rename FindLibDwarf -> FindLibDW

* Update FindLibDW

* Rename FindLibElf -> FindLibELF

* Update FindLibELF

* Create FindElfutils

* Update FindLibDebuginfod

* Update DyninstElfUtils

* Update the CMakeLists to use new targets

* Use CMP0074 in updated Find modules

This enables use of <Package>_ROOT variables when find_package is
invoked.

* Provide default dummy interface target for ElfUtils::ElfUtils

Needed for non-Unix platforms.

* Fix rebase bug in CMakeLists.txt

* Whitespace

* Export DyninstElfUtils

* Forward QUIET flag to pkg_check_modules

* Forward version to pkg_check_modules

* Use lib from pkg-config, if found

* Clean up internal variables

* Simplify cache variable handling

* Whitespace

* Use full linkage name for libs returned by pkg-config

* Separate out dependent libraries in FindLibDW

Some platforms include libelf as a dependency, but IMPORTED_LOCATION accepts only a single entry. Store the rest in IMPORTED_LINK_DEPENDENT_LIBRARIES.

* Fix quoting bug in FindLibDW

* Fix lib check in FindLibDW

* Manually set PC_<XXX>_INCLUDE_DIRS when FindPkgConfig misses it

FindPkgConfig uses the output from `pkg-config --cflags-only-I <lib>` to set PC_<XXX>_INCLUDE_DIRS. Because libelf is usually in a system directory, pkg-config will return nothing for this. FindPkgConfig stores the actual includedir variable from the PC file, so we can fetch it from there.
* LibIberty cmake modernization

* Use INCLUDE_DIRS directly
* Update FindThread_DB

* Update thread_db

* Update docs URL
This also provides a dummy target so we don't have to do any additional checking when USE_OpenMP=OFF. We only use OpenMP_CXX, so I didn't create a target for the other languages (C,Fortran).
* Update valgrind

* Add version check in Find module

* Remove Valgrind_LIBRARIES

They are versioned by architecture, so are hard to nail down with
find_library. We also don't need them (at least not yet).

* Make dummy when ADD_VALGRIND_ANNOTATIONS=OFF

* Add compile defs

* Update CMakeLists.txt

* Make the dummy IMPORTED

* Whitespace
This is so the CI version check will work uniformly
We just support the usual configs.
@hainest
Copy link
Copy Markdown
Contributor Author

hainest commented Mar 21, 2023

@kupsch Fixed the dynintAPI_RT.a issue. Let me know if you have any other troubles. I also merged in recent master.

@hainest
Copy link
Copy Markdown
Contributor Author

hainest commented Mar 21, 2023

Hosts

host arch OS Dyninst Test Suite Test Regressions Notes
perlmutter amd64 SLES15 ✔️ ✔️ Built with E4S 2022-11, gcc
mahir amd64 CentOS8
ufront.cs.rice.edu amd64 RHEL 8.6 ✔️ ✔️ 0 static rewriting is failing on master and this branch
crusher.ornl.gov amd64
frontier.ornl.gov amd64
coriander x86 Ubuntu-20.04 🔶 ✔️ 0 Won't build with /opt/intel-tbb due to missing CMake config files
quartz.llnl.gov x86
theta.alcf.anl.gov x86
sunspot.alcf.anl.gov x86
zeroah aarch64 Ubuntu-20.04 🔶 ✔️ 0 Won't build with /opt/intel-tbb due to missing CMake config files
wombat.ornl.gov aarch64
ulna.llnl.gov aarch64
llnl.cs.rice.edu ppc64le RHEL7 ✔️ ✔️ 0 No system software stack; have to use custom spack
summit.ornl.gov ppc64le RHEL 8.2 ✔️ ✔️ 0
lassen.llnl.gov ppc64le

Containers

Container Status Notes
boost ✔️ 1.71.0 to 1.81.0
cmake ✔️ 3.14.0 to 3.26.2
elfutils ✔️ 0.186 to 0.189
tbb ✔️ 2019_U9 to v2021.8.0
deps ✔️

repos

repo Status Notes
examples ✔️ dyninst/examples#32
external-tests
tools

hainest added 14 commits March 22, 2023 18:40
Setting this flag forces the linker to use RUNPATH instead of RPATH.
This is most useful for working with older RedHat distros.
This reduces the number of transitive links that have to be done by
binaries linking against Dyninst.
It's needed in the Module.h public header.
When building from source, versions before 2019.9 incorrectly set the
version in TBBConfigVersion.cmake. For example, 2018.6 sets the version
to 2018.0 because it uses the TBB_{MAJOR,MINOR}_VERSION from tbb_stddef.h
instead of doing the calculation based on the engineering version.

This also unifies the versions required when compiling with gcc and clang.

TBB 2018.6 was released in Oct 2018 and 2019.9 was released in Oct 2019,
so this just bumps the requirement by just a year even though there are
at least 9 releases in between.
The 3.13.* family requires every 'install' to specify a "LIBRARY
DESTINATION". We don't need or want that in the custome parseAPI install
(line ~130) for exporting the public headers.
@hainest hainest mentioned this pull request Apr 6, 2023
@jrmadsen
Copy link
Copy Markdown
Contributor

Any estimate for when this will be merged?

@hainest
Copy link
Copy Markdown
Contributor Author

hainest commented Apr 13, 2023

Any estimate for when this will be merged?

Very soon. I'm out of the office this week, but I would like to finish the testing on this by end of next week. This PR has revealed a host of bugs in Dyninst, so I'm trying to be as thorough as possible with the testing.

hainest added 4 commits April 21, 2023 15:07
The headers from symtabAPI are still required, even when building with symLite.
@hainest hainest merged commit 739ad57 into master Apr 25, 2023
@hainest hainest deleted the cmake_modernization branch April 25, 2023 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment