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 refactor #105

Merged
merged 44 commits into from
Aug 22, 2022
Merged

CMake refactor #105

merged 44 commits into from
Aug 22, 2022

Conversation

ekilmer
Copy link
Contributor

@ekilmer ekilmer commented Jun 26, 2022

This is a major CMake refactor that I got carried away with. There are a couple breaking changes listed as follows:

  • Renaming of targets
    • Internal targets, which should not be used, are prefixed with sleigh_ to avoid potential conflicts in projects that include this project's whole directory
    • Removal of _opt suffix on some targets to be more consistent with export target names. The suffix _dbg is added only for Debug targets
    • decomp_opt target executable is now decompiler to avoid conflict with decomp library target
  • Removal of sleigh_settings target
    • Common target settings are now added through a macro
  • Add sleigh_support target
    • This target is now used for
  • Created Support and ExtraTools COMPONENTS for sleigh install config file to differentiate between what Ghidra includes and what ToB includes.
  • Created Specs COMPONENTS for sleigh install config to ensure sleigh specs are available.
  • Simplification of CMake options
    • Support for multi-config generators with correct compiler definitions using CMake genex
    • There is now sleigh_DEVELOPER_MODE option to include options to build tests and documentation
  • Documentation must be built manually and must exist before running install if you've chosen to build documentation through sleigh_BUILD_DOCUMENTATION CMake option. This is a bit unergonomic but there isn't a good way to do incremental doc builds with our current setup.
    • cmake --build build --target all --target docs will build everything and then you can cmake --install ...

Other changes that I would consider improvements to the project:

  • CMake presets file. See the CMake docs for more info https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html
    • This is used in CI to build sleigh
    • Includes an easy way to compile with sanitizers
  • Directory layout changes
    • Move include directory to support directory to make it clear that those includes do not come from Ghidra/sleigh
    • Move some CMake logic into its own file(s)
    • Upstream tools get their own directories. This makes it easier to read the CMakeLists.txt file and also separates them for use as standalone projects if wanted.
      • This also makes the top-level CMakeLists.txt file smaller and only necessarily includes the library targets
    • Each subdirectory (aside from support) is its own project that can build itself and bootstrap the necessary sleigh libraries as needed.
  • Support for cross-compiling
  • We now build documentation on Windows
  • We now use ccache on Windows CI
  • We now test sleighLift executable
  • Updated READMEs for current directions on this branch
  • Only build relevant packaging artifacts based on host OS
    • This might not be totally accurate for cross-compilations

closes #95
closes #93
closes #57

@tetsuo-cpp tetsuo-cpp self-requested a review June 28, 2022 13:25
@tetsuo-cpp
Copy link
Contributor

@ekilmer This looks great. Is this ready to start reviewing?

@ekilmer ekilmer mentioned this pull request Jul 6, 2022
6 tasks
@ekilmer ekilmer marked this pull request as ready for review July 7, 2022 02:11
@ekilmer
Copy link
Contributor Author

ekilmer commented Jul 7, 2022

@tetsuo-cpp This will need some testing in all the projects that currently use sleigh.

I've taken a stab at integrating sleigh into vcpkg in trailofbits/maat#128 (see the files in this directory for vcpkg sleigh https://github.com/trailofbits/maat/tree/ekilmer/vcpkg-integration/ports/sleigh) so that Maat only builds what it needs---libsla and the sleigh spec compiler (libdecomp is also built but that's fine). Note that the vcpkg definition and this CMake refactor allows a user to skip building and installing the compiled sleigh files. The installation of compiled sleigh files and the other data takes up a lot of space, so it's nice if a project doesn't need it, it has the option to not build it through vcpkg.

More testing of the different option combinations (which I think can be tested with vcpkg features) are also good. Additionally, testing each tool as a top-level project to make sure I've connected everything correctly is also something to look at.

@tetsuo-cpp
Copy link
Contributor

@tetsuo-cpp This will need some testing in all the projects that currently use sleigh.

I've taken a stab at integrating sleigh into vcpkg in trailofbits/maat#128 (see the files in this directory for vcpkg sleigh https://github.com/trailofbits/maat/tree/ekilmer/vcpkg-integration/ports/sleigh) so that Maat only builds what it needs---libsla and the sleigh spec compiler (libdecomp is also built but that's fine). Note that the vcpkg definition and this CMake refactor allows a user to skip building and installing the compiled sleigh files. The installation of compiled sleigh files and the other data takes up a lot of space, so it's nice if a project doesn't need it, it has the option to not build it through vcpkg.

More testing of the different option combinations (which I think can be tested with vcpkg features) are also good. Additionally, testing each tool as a top-level project to make sure I've connected everything correctly is also something to look at.

I can try moving Ian's Remill branch over to this version of Sleigh if that's useful. Also what did you mean by "testing each tool as a top-level project"?

@ekilmer
Copy link
Contributor Author

ekilmer commented Jul 7, 2022

I can try moving Ian's Remill branch over to this version of Sleigh if that's useful

Yes, please make sure remill is able to use this branch easily (they will probably only want to build certain parts of this project, so hopefully they can compile only that minimal set)

testing each tool as a top-level project

Like this:

cmake -S tools/decompiler -B build-decompiler
cmake --build build-decompiler
cmake --install build-decompiler --prefix decompiler-install

and also for tools/ghidra, tools/spec-compiler. And also with -Dsleigh_GHIDRA_RELEASE_TYPE=HEAD. They probably all work but just want to make sure it makes sense to someone else but me 😄

ekilmer added 3 commits July 7, 2022 14:15
* master:
  Add missing headers (#107)
  Update Ghidra HEAD to commit d2883bbb8 (#102)
* master:
  More complete patch for all files with missing include guard (#116)
  Fix wrong CMake cache variable in CI (#115)
  Update to Ghidra stable 10.1.5 (#114)
  Update CMake minimum to 3.18
  Update libsleigh single-header include list
  Bump Ghidra HEAD commit 75ae8b3
  Fix SLEIGH rebuilds (#111)
  Allow external patches and expose more headers (#104)
@tetsuo-cpp
Copy link
Contributor

@ekilmer By the way, Ian's Sleigh branch is merged into Remill now. I'll take a stab at building it with this branch and see how I go.

@tetsuo-cpp
Copy link
Contributor

Hmm, Remill seems to build fine without any modifications. I was curious about what you said here:

they will probably only want to build certain parts of this project, so hopefully they can compile only that minimal set

My understanding is that by default, only the library components get built and not tools (or extra tools like sleigh-lift). Remill doesn't use any of the tools so these shouldn't be getting built. Do I need to do anything else to "opt out" of building unnecessary components?

@ekilmer
Copy link
Contributor Author

ekilmer commented Aug 3, 2022

My understanding is that by default, only the library components get built and not tools (or extra tools like sleigh-lift).

I might have said that at one point, but now that is not true. All components of the project are built by default. To see available options, after configuring with CMake, you can run ccmake <build_directory>, i.e.:

$ cmake -S . -B build
$ ccmake build

which will show a terminal UI with the options and their configured values. You could also use cmake-gui build if you want a GUI. The descriptions for the sleigh_-prefixed variables should hopefully tell you what you need to turn ON or OFF when including/excluding different parts of the project. For instance, to not build sleighLift, you should set -Dsleigh_BUILD_EXTRATOOLS=OFF during CMake configuration.

If a project is including this project through add_subdirectory, then it should look like this, to avoid building all targets in this project with the ALL target.

add_subdirectory(sleigh EXCLUDE_FROM_ALL)

or you could manually set the cache variables before including the project:

set(sleigh_BUILD_EXTRATOOLS OFF CACHE BOOL "")
add_subdirectory(sleigh)

to turn off only specific features, but I think I would recommend using the first method.

@ekilmer
Copy link
Contributor Author

ekilmer commented Aug 3, 2022

by default, only the library components get built and not tools (or extra tools like sleigh-lift)

@tetsuo-cpp Do you think this should be true before merging? I wasn't sure what would be best for user experience. Seems like the most simple way for a new person to get started is to have everything enabled and built.

@tetsuo-cpp
Copy link
Contributor

and also for tools/ghidra, tools/spec-compiler. And also with -Dsleigh_GHIDRA_RELEASE_TYPE=HEAD. They probably all work but just want to make sure it makes sense to someone else but me 😄

Everything under tools/ builds fine as a top-level project. However, I noticed that extra-tools/sleigh-lift doesn't build properly like this. If that's unexpected, I can take a look at fixing this.

@tetsuo-cpp
Copy link
Contributor

by default, only the library components get built and not tools (or extra tools like sleigh-lift)

@tetsuo-cpp Do you think this should be true before merging? I wasn't sure what would be best for user experience. Seems like the most simple way for a new person to get started is to have everything enabled and built.

I think your current approach seems sensible. Just build everything to start off with and give users the tools to optimize their builds if necessary. 👍

@ekilmer
Copy link
Contributor Author

ekilmer commented Aug 3, 2022

I know what the issue is. I'll fix it in a bit. Thank you for checking!

@ekilmer
Copy link
Contributor Author

ekilmer commented Aug 3, 2022

extra-tools/sleigh-lift now builds as a standalone project but it doesn't run as you might expect... The sleigh specs aren't built/compiled. Hmmmmmm. There are a couple of things that could happen that might improve usability in this way. I need to mull it over and play around with things.

* Also add "Specs" component to sleigh installation config

* Try to be better at bootstrapping subprojects (hopefully this doesn't
  bite us, but if it does, I'm happy to remove it and say "not
  supported")
@ekilmer
Copy link
Contributor Author

ekilmer commented Aug 4, 2022

@tetsuo-cpp I think this is ready for another review.

Also, I changed how this project works for projects that use add_subdirectory(sleigh) or FetchContent: When that happens, only the core libraries are built (sleigh::sla and sleigh::decomp) and you'll need to set the cache variables to include other things like sleigh specs and support library. I think this is a better way of doing things after I found out that add_subdirectory(sleigh EXCLUDE_FROM_ALL) disables installation commands in sleigh.

Let me know if you have any questions!

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
cmake/packaging.cmake Show resolved Hide resolved
tests/CMakeLists.txt Show resolved Hide resolved
* master:
  Update Ghidra HEAD to commit aad60ecdd (#122)
  Update Ghidra HEAD to commit cef30890e (#120)
@ekilmer ekilmer requested a review from tetsuo-cpp August 19, 2022 17:14
Copy link
Contributor

@tetsuo-cpp tetsuo-cpp left a comment

Choose a reason for hiding this comment

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

LGTM! Amazing work @ekilmer.

@tetsuo-cpp tetsuo-cpp merged commit 74838a3 into master Aug 22, 2022
@tetsuo-cpp tetsuo-cpp deleted the cmake-presets branch August 22, 2022 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants