Enable Link Time Optimization (LTO) with clang #2920

Merged
merged 3 commits into from Jan 22, 2017

Conversation

Projects
None yet
5 participants
@artemdinaburg
Contributor

artemdinaburg commented Jan 18, 2017

  • Change the LLVM formula to build the LLVMgold plugin
  • Pass "-flto" for osquery and all dependencies when building with clang on Unix
  • Pass "-flto" to modules when they are built
  • Set ranlib and ar to their LLVM versions when building on Linux

In my quick tests, an LTO build results in an osqueryd binary thats about 2MiB smaller than the equivalent non-LTO build.

Enabling LTO is also a first step to turning on Control Flow Integrity.

Artem Dinaburg
Enable Link Time Optimization (LTO) with clang
* Change the LLVM formula to build the LLVMgold plugin
* Pass "-flto" for osquery and all dependencies when building with clang on Unix
* Pass "-flto" to modules when they are built
* Set `ranlib` and `ar` to their LLVM verions when building on Linux
@muffins

This comment has been minimized.

Show comment
Hide comment
@muffins

muffins Jan 19, 2017

Contributor

ok to test

Contributor

muffins commented Jan 19, 2017

ok to test

@osqueryer

This comment has been minimized.

Show comment
Hide comment
@osqueryer

osqueryer Jan 19, 2017

Collaborator

👎 The commit 32074b5 (Job results: 278) failed one or more tests (macOS/OS X).

Collaborator

osqueryer commented Jan 19, 2017

👎 The commit 32074b5 (Job results: 278) failed one or more tests (macOS/OS X).

@osqueryer

This comment has been minimized.

Show comment
Hide comment
@osqueryer

osqueryer Jan 19, 2017

Collaborator

👎 The commit 32074b5 (Job results: 4026) failed one or more tests (Linux).

Collaborator

osqueryer commented Jan 19, 2017

👎 The commit 32074b5 (Job results: 4026) failed one or more tests (Linux).

@osqueryer

This comment has been minimized.

Show comment
Hide comment
@osqueryer

osqueryer Jan 19, 2017

Collaborator

👎 The commit 32074b5 (Job results: 4027) failed one or more tests (Linux).

Collaborator

osqueryer commented Jan 19, 2017

👎 The commit 32074b5 (Job results: 4027) failed one or more tests (Linux).

Artem Dinaburg
Update revision on llvm forumula
Updat the revision on llvm formula in llvm.rb to force a rebuild.
Re-enable bottle code since rebuilds are now triggered by the revision change.
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment

@artemdinaburg updated the pull request - view changes

CMakeLists.txt
@@ -142,6 +148,20 @@ else()
endif()
endif()
+if(UNIX AND NOT APPLE)

This comment has been minimized.

@theopolis

theopolis Jan 20, 2017

Contributor

Maybe just if(LINUX)

@theopolis

theopolis Jan 20, 2017

Contributor

Maybe just if(LINUX)

This comment has been minimized.

@theopolis

theopolis Jan 20, 2017

Contributor

Could you add the CMAKE_CXX_COMPILER MATCHES "clang" to this conditional too? It seems more consistent with the rest of the CMake logic changes.

@theopolis

theopolis Jan 20, 2017

Contributor

Could you add the CMAKE_CXX_COMPILER MATCHES "clang" to this conditional too? It seems more consistent with the rest of the CMake logic changes.

This comment has been minimized.

@artemdinaburg

artemdinaburg Jan 21, 2017

Contributor

I kept it at UNIX AND NOT APPLE since I figured other platforms like FreeBSD can also build with LTO; although I have not tested. If FreeBSD and other UNIXes are not a worry, I can change it to IF(LINUX)

@artemdinaburg

artemdinaburg Jan 21, 2017

Contributor

I kept it at UNIX AND NOT APPLE since I figured other platforms like FreeBSD can also build with LTO; although I have not tested. If FreeBSD and other UNIXes are not a worry, I can change it to IF(LINUX)

This comment has been minimized.

@theopolis

theopolis Jan 21, 2017

Contributor

Right, there're too many conditionals using LINUX, other UNIXes have been left behind.

@theopolis

theopolis Jan 21, 2017

Contributor

Right, there're too many conditionals using LINUX, other UNIXes have been left behind.

osquery/CMakeLists.txt
@@ -33,6 +33,12 @@ else()
-Wno-unused-parameter
-Wno-gnu-case-range
)
+ if (CMAKE_CXX_COMPILER MATCHES "clang")

This comment has been minimized.

@theopolis

theopolis Jan 20, 2017

Contributor

You should be able to nix this.

@theopolis

theopolis Jan 20, 2017

Contributor

You should be able to nix this.

This comment has been minimized.

@artemdinaburg

artemdinaburg Jan 21, 2017

Contributor

Are we guaranteed to be on clang at this point? I added it since if someone were to set CC=gcc as an environment variable, -flto would cause issues.

@artemdinaburg

artemdinaburg Jan 21, 2017

Contributor

Are we guaranteed to be on clang at this point? I added it since if someone were to set CC=gcc as an environment variable, -flto would cause issues.

This comment has been minimized.

@theopolis

theopolis Jan 21, 2017

Contributor

Ah, I meant the whole body, the CMakeLists.txt in the top level should have added this compile option right?

@theopolis

theopolis Jan 21, 2017

Contributor

Ah, I meant the whole body, the CMakeLists.txt in the top level should have added this compile option right?

osquery/CMakeLists.txt
+ # When doing a clang on Unix LTO build, the link step also needs
+ # -flto on the comand line
+ ADD_OSQUERY_LINK_CORE("-flto")
+ ADD_OSQUERY_LINK_ADDITIONAL("-flto")

This comment has been minimized.

@theopolis

theopolis Jan 20, 2017

Contributor

You can nix this, core's links should be added to additional's IIRC.

@theopolis

theopolis Jan 20, 2017

Contributor

You can nix this, core's links should be added to additional's IIRC.

Artem Dinaburg
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment

@artemdinaburg updated the pull request - view changes

@theopolis theopolis merged commit 24513a3 into facebook:master Jan 22, 2017

4 checks passed

Code Audit Build finished.
Details
Linux Build finished.
Details
Windows Build finished.
Details
macOS/OS X Build finished.
Details

@artemdinaburg artemdinaburg deleted the artemdinaburg:clang_lto_pr branch Jan 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment