-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Unify some cmake function definitions #33716
Changes from 3 commits
27eef50
5324f02
4fc15c2
2264fa0
f193079
ee03fc0
b861b05
1b2b5c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -124,7 +124,7 @@ function(preprocess_compile_asm) | |||
set(oneValueArgs OUTPUT_OBJECTS) | ||||
set(multiValueArgs ASM_FILES) | ||||
cmake_parse_arguments(PARSE_ARGV 0 COMPILE_ASM "${options}" "${oneValueArgs}" "${multiValueArgs}") | ||||
|
||||
get_include_directories_asm(ASM_INCLUDE_DIRECTORIES) | ||||
|
||||
set (ASSEMBLED_OBJECTS "") | ||||
|
@@ -259,7 +259,18 @@ function(strip_symbols targetName outputFilename skipStrip) | |||
set(strip_destination_file ${strip_source_file}.dwarf) | ||||
|
||||
if(NOT ${skipStrip}) | ||||
add_custom_command( | ||||
# Ensure that dsymutil and strip are present | ||||
find_program(DSYMUTIL dsymutil) | ||||
if (DSYMUTIL STREQUAL "DSYMUTIL-NOTFOUND") | ||||
message(FATAL_ERROR "dsymutil not found") | ||||
endif() | ||||
|
||||
find_program(STRIP strip) | ||||
if (STRIP STREQUAL "STRIP-NOTFOUND") | ||||
message(FATAL_ERROR "strip not found") | ||||
endif() | ||||
|
||||
add_custom_command( | ||||
TARGET ${targetName} | ||||
POST_BUILD | ||||
VERBATIM | ||||
|
@@ -289,6 +300,20 @@ function(strip_symbols targetName outputFilename skipStrip) | |||
endif(CLR_CMAKE_HOST_UNIX) | ||||
endfunction() | ||||
|
||||
function(install_symbols targetName destination_path) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is weird that we always strip symbols for libraries and strip them conditionally for coreclr. Since you are touching this code, I would take this opportunity to make the stripping unconditional. It should be fine (debugging should not be affected, I believe neither lldb nor gdb cares whether the symbols are in the .so files or in separate files) and it would also fix #32957 on OSX where debugging a core dump from checked / debug build is currently broken due to missing symbols. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we start by removing the top-level option: runtime/eng/native/build-commons.sh Line 190 in 8db7154
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. II would do it in 3 steps in this order:
The point is that we don't want to have intermediate period where we would not be stripping the symbols and we also cannot pass in the -stripsymbols from the CI / build lab scripts after we remove support for that option from the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Part 1 is pushed f193079. I will make followup PRs for 2 and 3. Thanks! |
||||
install_symbols_with_skip(${targetName} ${destination_path} NO) | ||||
endfunction() | ||||
|
||||
function(install_symbols_with_skip targetName destination_path skipStrip) | ||||
strip_symbols(${targetName} strip_destination_file ${skipStrip}) | ||||
|
||||
if(CLR_CMAKE_TARGET_WIN32) | ||||
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>/${targetName}.pdb DESTINATION ${destination_path}/PDB) | ||||
else() | ||||
install(FILES ${strip_destination_file} DESTINATION ${destination_path}) | ||||
endif() | ||||
endfunction() | ||||
|
||||
# install_clr(TARGETS TARGETS targetName [targetName2 ...] [DESTINATION destination] [SKIP_STRIP]) | ||||
function(install_clr) | ||||
set(options SKIP_STRIP) | ||||
|
@@ -310,18 +335,13 @@ function(install_clr) | |||
if("${INSTALL_CLR_SKIP_STRIP}" STREQUAL "") | ||||
set(INSTALL_CLR_SKIP_STRIP FALSE) | ||||
endif() | ||||
strip_symbols(${targetName} strip_destination_file ${INSTALL_CLR_SKIP_STRIP}) | ||||
|
||||
install_symbols_with_skip(${targetName} ${INSTALL_CLR_DESTINATION} ${INSTALL_CLR_SKIP_STRIP}) | ||||
|
||||
# We don't need to install the export libraries for our DLLs | ||||
# since they won't be directly linked against. | ||||
install(PROGRAMS $<TARGET_FILE:${targetName}> DESTINATION ${INSTALL_CLR_DESTINATION}) | ||||
if(WIN32) | ||||
# We can't use the $<TARGET_PDB_FILE> generator expression here since | ||||
# the generator expression isn't supported on resource DLLs. | ||||
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>/${targetName}.pdb DESTINATION ${INSTALL_CLR_DESTINATION}/PDB) | ||||
else() | ||||
install(FILES ${strip_destination_file} DESTINATION ${INSTALL_CLR_DESTINATION}) | ||||
endif() | ||||
|
||||
if(CLR_CMAKE_PGO_INSTRUMENT) | ||||
if(WIN32) | ||||
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>/${targetName}.pgd DESTINATION ${INSTALL_CLR_DESTINATION}/PGD OPTIONAL) | ||||
|
@@ -339,6 +359,11 @@ endfunction() | |||
# - creating executable pages from anonymous memory, | ||||
# - making read-only-after-relocations (RELRO) data pages writable again. | ||||
function(disable_pax_mprotect targetName) | ||||
# Try to locate the paxctl tool. Failure to find it is not fatal, | ||||
# but the generated executables won't work on a system where PAX is set | ||||
# to prevent applications to create executable memory mappings. | ||||
find_program(PAXCTL paxctl) | ||||
|
||||
if (NOT PAXCTL STREQUAL "PAXCTL-NOTFOUND") | ||||
add_custom_command( | ||||
TARGET ${targetName} | ||||
|
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.
Is there a problem with using
/Gz
everywhere?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.
With
/Gz
in shared location, we get the following error in installer x86 build:I think it is because this function requires
__cdecl
?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.
Yes, looks like some places are missing
HOSTPOLICY_CALLTYPE
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.
That also worked, now /Gz is back in shared location: 2264fa0.
__cdecl
is defined as empty for other platforms in pal.h, so I just used __cdecl or HOSTPOLICY_CALLTYPE, whichever was available via included headers.