-
Notifications
You must be signed in to change notification settings - Fork 693
Use cmake POSITION_INDEPENDENT_CODE instead of hardcoding -pie -fPIE #1598
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
Conversation
@@ -107,8 +107,6 @@ set (WAMR_ROOT_DIR ${CMAKE_CURRENT_SOURCE_DIR}) | |||
|
|||
include (${WAMR_ROOT_DIR}/build-scripts/runtime_lib.cmake) | |||
|
|||
set (CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,--gc-sections -pie -fPIE") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i removed this because there is no executable target in this file.
@@ -1,7 +1,9 @@ | |||
# Copyright (C) 2019 Intel Corporation. All rights reserved. | |||
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | |||
|
|||
cmake_minimum_required (VERSION 2.9) | |||
cmake_minimum_required (VERSION 3.14) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little afraid whether it is friendly to upgrade the cmake version, maybe we can resolve the pie -fPIE
issue by checking whether the compiler is clang, for example for the CMakeLists.txt of wamrc:
--- a/wamr-compiler/CMakeLists.txt
+++ b/wamr-compiler/CMakeLists.txt
@@ -237,7 +237,7 @@ endif()
if (NOT MSVC)
add_definitions(-D_FORTIFY_SOURCE=2)
set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -ftrapv")
- if (NOT WIN32)
+ if (NOT CMAKE_C_COMPILER MATCHES ".*clang.*" AND NOT CMAKE_C_COMPILER_ID MATCHES ".*Clang")
set (CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -pie -fPIE")
endif()
endif()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok with the change of cmake requirement.
But how about other CMakeLists.txt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with 3.14 as well, it's already 3 years old and it comes with lots of good features (compared to 2.9) so I think it's worth taking that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok.
BTW, is there anyway to merge these dupped code into one ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good, thanks. @yamt, could you resolve the conflict so that I can merge the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rebased
This fixes unused option warnings on -pie for macOS. (On macOS cmake produces "-fPIE -Xlinker -pie") Bump required cmake version to 3.14 for CheckPIESupported. References: https://cmake.org/cmake/help/latest/prop_tgt/POSITION_INDEPENDENT_CODE.html https://cmake.org/cmake/help/latest/module/CheckPIESupported.html#module:CheckPIESupported
…ytecodealliance#1598) This fixes unused option warnings on -pie for macOS. (On macOS cmake produces "-fPIE -Xlinker -pie") Bump required cmake version to 3.14 for CheckPIESupported. References: https://cmake.org/cmake/help/latest/prop_tgt/POSITION_INDEPENDENT_CODE.html https://cmake.org/cmake/help/latest/module/CheckPIESupported.html#module:CheckPIESupported
This fixes unused option warnings on -pie for macOS. (On macOS cmake produces "-fPIE -Xlinker -pie")
Bump required cmake version to 3.14 for CheckPIESupported.
References:
https://cmake.org/cmake/help/latest/prop_tgt/POSITION_INDEPENDENT_CODE.html https://cmake.org/cmake/help/latest/module/CheckPIESupported.html#module:CheckPIESupported