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
Open

Add build support for PIE code generation for executables #1871

Show file tree
Hide file tree
Changes from 3 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
10 changes: 9 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,16 @@ 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 AND NOT CMAKE_C_LINK_PIE_SUPPORTED)
message(FATAL_ERROR "PIE is not supported at link time.\n")
else()
set(CMAKE_POSITION_INDEPENDENT_CODE ON)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this logic what you intend? I think this means that if PIE linkage is supported, you are going to turn on CMAKE_POSITION_INDEPENDENT_CODE unconditionally, regardless of how OCIO_BUILD_POSITION_INDEPENDENT_EXECUTABLES was set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, looks like I committed a bit of a noob logic error. Serves me right for not testing on a machine that would fail the 2nd sub expression


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

Expand Down
4 changes: 4 additions & 0 deletions docs/quick_start/installation.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
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 Acheive this code generated needs to be position independent.
To support thie please add ``-DOCIO_BUILD_POSITION_INDEPENDENT_EXECUTABLES=ON`` to your cmake
configuration step.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: Some typos in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, I'll hold off fixing this till we decide on the explicit option vs just passing in CMAKE_POSITION_INDEPENDENT_CODE


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