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.

Sign up for GitHub

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

Jump to bottom

Add build support for PIE code generation for executables #1871

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+19 −2
Open

Add build support for PIE code generation for executables #1871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8379cfa
Bump CMake minimum version to 3.15, allow support for PIE linker flags
KevinJW Sep 22, 2023
5a4a6e5
Support CMake option for building all executables as position indepen…
KevinJW Sep 22, 2023
a0dc5f0
Add documentation note to build instructions for PIE generation
KevinJW Sep 22, 2023
9d67a78
Fix logic error when PIE code generation is not available
KevinJW Sep 25, 2023
0f95d4c
fix some minor typos in documentation.
KevinJW Sep 25, 2023
f9685f7
Change documentation to refer to CMake 3.15 requirement for building
KevinJW Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
###############################################################################
# CMake definition.

cmake_minimum_required(VERSION 3.13)
cmake_minimum_required(VERSION 3.15)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Forgot to mention but we should also update the minimum version in other places, half a dozen or so references came up when looking for 3.13 in the code base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another TSC query or two?

  1. Should we change the version mandated by us for the consumer of the project? akin to PRIVATE, vs PUBLIC
  2. For files included from external sources should we update those?
  3. For CMake files used to build our dependencies
  4. For "tests" and other configuration setup where we don't need the feature should we match version requirement


set(CMAKE_MODULE_PATH
${CMAKE_MODULE_PATH}
Expand Down Expand Up @@ -341,8 +341,18 @@ endif()
###############################################################################
# Define compilation and link flags

option(OCIO_BUILD_POSITION_INDEPENDENT_EXECUTABLES "Set to ON to build executables as PIE" OFF)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Do we need a new option switch for this or should we just look at the user provided CMAKE_POSITION_INDEPENDENT_CODE and print a warning if PIE is not supported? I tend to prefer just using the regular CMake variables and not add layering on top.

As a related observation, we seem to already force this for some targets, as can be found searching for POSITION_INDEPENDENT_CODE in the code base.

Copy link
Contributor Author

@KevinJW KevinJW Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sure we force it for any shared objects, which by definition need to be relocatable,

The new option I was considering the question of how do we know the intention of what adding the option on the command line is supposed to mean. the named option was about communicating their intent. I guess we could do a simple vote next TSC about what people prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah let's discuss that next TSC, also seem we should try to align with other projects on this.


include(CompilerFlags)

if(OCIO_BUILD_POSITION_INDEPENDENT_EXECUTABLES)
if(NOT CMAKE_C_LINK_PIE_SUPPORTED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to check CMAKE_CXX_LINK_PIE_SUPPORTED as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe, in that case we should probably be explicit about the languages we are checking for in the check_pie_supported() call as well

message(FATAL_ERROR "PIE is not supported at link time.\n")
endif()
message(STATUS "Enabling PIE generation.\n")
set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()

###############################################################################
# External linking options

Expand Down
6 changes: 5 additions & 1 deletion docs/quick_start/installation.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ items manually:
Required components:

- C++ 11-17 compiler (gcc, clang, msvc)
- CMake >= 3.13
- CMake >= 3.15
- \*Expat >= 2.4.1 (XML parser for CDL/CLF/CTF)
- \*yaml-cpp >= 0.7.0 (YAML parser for Configs)
- \*Imath >= 3.0 (for half domain LUTs)
Expand Down Expand Up @@ -316,6 +316,10 @@ build chains.)
The CMake output prints information regarding which image library will be used for the
command-line tools (as well as a lot of other info about the build configuration).

Some OS distributions may require executables to be able to be executed using address space
layout randomisation (ASLR). To achieve this code generated needs to be position independent.
To support this please add ``-DOCIO_BUILD_POSITION_INDEPENDENT_EXECUTABLES=ON`` to your CMake
configuration step.

Documentation
+++++++++++++
Expand Down
3 changes: 3 additions & 0 deletions share/cmake/utils/CompilerFlags.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ endif()
###############################################################################
# Compile flags

include(CheckPIESupported)
check_pie_supported()

if(USE_MSVC)

set(PLATFORM_COMPILE_OPTIONS "${PLATFORM_COMPILE_OPTIONS};/DUSE_MSVC")
Expand Down