Add build support for PIE code generation for executables #1871
Conversation
As per TSC discussion, we will bump minimum cmake version to 3.15, this matches OpenImageIO and ASWF template project. Strictly speaking CMake added this in 3.14, but matching the other ASWF users was seen as a sensible option. For correct support of PIE objects being linked correctly we need to trigger cmake to check for pie support see: https://cmake.org/cmake/help/latest/module/CheckPIESupported.html https://cmake.org/cmake/help/latest/policy/CMP0083.html Signed-off-by: Kevin Wheatley <kevin.wheatley@framestore.com>
…dent Signed-off-by: Kevin Wheatley <kevin.wheatley@framestore.com>
Signed-off-by: Kevin Wheatley <kevin.wheatley@framestore.com>
KevinJW
changed the title
CMakeLists.txt
Outdated
| 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Signed-off-by: Kevin Wheatley <kevin.wheatley@framestore.com>
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Do we need to check CMAKE_CXX_LINK_PIE_SUPPORTED as well?
There was a problem hiding this comment.
maybe, in that case we should probably be explicit about the languages we are checking for in the check_pie_supported() call as well
docs/quick_start/installation.rst
Outdated
| 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. |
There was a problem hiding this comment.
Minor: Some typos in there.
There was a problem hiding this comment.
indeed, I'll hold off fixing this till we decide on the explicit option vs just passing in CMAKE_POSITION_INDEPENDENT_CODE
| @@ -5,7 +5,7 @@ | |||
| ############################################################################### | |||
| # CMake definition. | |||
|
|
|||
| cmake_minimum_required(VERSION 3.13) | |||
| cmake_minimum_required(VERSION 3.15) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Another TSC query or two?
- Should we change the version mandated by us for the consumer of the project? akin to PRIVATE, vs PUBLIC
- For files included from external sources should we update those?
- For CMake files used to build our dependencies
- For "tests" and other configuration setup where we don't need the feature should we match version requirement
| @@ -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) | |||
There was a problem hiding this comment.
Yeah let's discuss that next TSC, also seem we should try to align with other projects on this.
Signed-off-by: Kevin Wheatley <kevin.wheatley@framestore.com>
Signed-off-by: Kevin Wheatley <kevin.wheatley@framestore.com>
As per the discussion #1857 add a configuration option to build executables as PIE.
Note, I looked at applying the position independence at a specific cmake target level but after
tracing though the dependencies this top level adjustment is basically the same but fewer lines of
CMake, i.e. we need to set the flag for all object files used in a given executable.
Signed-off-by: Kevin Wheatley kevin.wheatley@framestore.com