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.

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

Merged
merged 8 commits into from
Mar 21, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
include(${CMAKE_CURRENT_LIST_DIR}/configuretools.cmake)

# Set initial flags for each configuration

set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
Expand All @@ -18,7 +20,7 @@ include(${CMAKE_CURRENT_LIST_DIR}/configureoptimization.cmake)
#-----------------------------------------------------

if(MSVC)
add_compile_options(/Zi /FC /Zc:strictStrings)
Copy link
Member

Choose a reason for hiding this comment

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

What happened to /Zc:strictStrings ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was causing an error in installer:

.\build.cmd -subsetcategory installer

           fxr_resolver.cpp
       6>C:\Users\adeel\Source\Repos\runtime2\src\installer\corehost\cli\hostmisc\pal.windows.cpp(230,51): error C2440: 'initializing': cannot convert from 'const unsigned short [18]' to 'pal::char_t *' [C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x64.Debug\corehost\cli\hostmisc\libhostmisc.vcxproj]
         C:\Users\adeel\Source\Repos\runtime2\src\installer\corehost\cli\hostmisc\pal.windows.cpp(230,26): message : Conversion from string literal loses const qualifier (see /Zc:strictStrings) [C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x64.Debug\corehost\cli\hostmisc\libhostmisc.vcxproj]
           longfile.windows.cpp
       6>C:\Users\adeel\Source\Repos\runtime2\src\installer\corehost\cli\hostmisc\pal.windows.cpp(280,52): error C2440: '=': cannot convert from 'const unsigned short [18]' to 'pal::char_t *' [C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x64.Debug\corehost\cli\hostmisc\libhostmisc.vcxproj]
         C:\Users\adeel\Source\Repos\runtime2\src\installer\corehost\cli\hostmisc\pal.windows.cpp(280,29): message : Conversion from string literal loses const qualifier (see /Zc:strictStrings) [C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x64.Debug\corehost\cli\hostmisc\libhostmisc.vcxproj]

I forgot to add it in CoreCLR, will do. Or maybe I should first try to make installer compatible with strictStrings. 💭

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it would be nice to sprinkle const as necessary to fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am running a clean Windows build on side, will get to this point soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Using const in few places fixed the build error withstrictStrings: 4fc15c2. Thanks. 👍

add_compile_options(/Zi /FC)
elseif (CLR_CMAKE_HOST_UNIX)
add_compile_options(-g)
add_compile_options(-Wall)
Expand All @@ -41,8 +43,6 @@ set(CMAKE_SHARED_LINKER_FLAGS_CHECKED "")
add_compile_definitions("$<$<OR:$<CONFIG:DEBUG>,$<CONFIG:CHECKED>>:DEBUG;_DEBUG;_DBG;URTBLDENV_FRIENDLY=Checked;BUILDENV_CHECKED=1>")
add_compile_definitions("$<$<OR:$<CONFIG:RELEASE>,$<CONFIG:RELWITHDEBINFO>>:NDEBUG;URTBLDENV_FRIENDLY=Retail>")

set(CMAKE_CXX_STANDARD_LIBRARIES "") # do not link against standard win32 libs i.e. kernel32, uuid, user32, etc.

if (MSVC)
add_link_options(/GUARD:CF)

Expand Down Expand Up @@ -373,6 +373,9 @@ if(CLR_CMAKE_TARGET_UNIX)
if(CLR_CMAKE_TARGET_NETBSD)
add_definitions(-DTARGET_NETBSD)
endif(CLR_CMAKE_TARGET_NETBSD)
if(CLR_CMAKE_TARGET_ANDROID)
add_definitions(-DTARGET_ANDROID)
endif()
else(CLR_CMAKE_TARGET_UNIX)
add_definitions(-DTARGET_WINDOWS)
endif(CLR_CMAKE_TARGET_UNIX)
Expand Down Expand Up @@ -467,10 +470,6 @@ if (MSVC)
add_compile_options(/ZH:SHA_256) # use SHA256 for generating hashes of compiler processed source files.
add_compile_options(/source-charset:utf-8) # Force MSVC to compile source as UTF-8.

if (CLR_CMAKE_HOST_ARCH_I386)
add_compile_options(/Gz)
Copy link
Member

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?

Copy link
Member Author

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:

.\build.cmd -subsetcategory installer -arch x86

...
         "C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x86.Debug\corehost\INSTALL.vcxproj" (rebuild target) (1) ->
         "C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x86.Debug\corehost\ALL_BUILD.vcxproj" (default target) (3:2) ->
         "C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x86.Debug\corehost\cli\comhost\comhost.vcxproj" (default target) (4:2) ->
           C:\Users\adeel\Source\Repos\runtime2\src\installer\corehost\cli\fxr_resolver.h(83,1): error C2664: 'propagate_error_writer_t::propagate_error_writer_t(const propagate_error_writer_t &)': cannot convert argument 1 from 'hostfxr_set_error_writer_fn' to 'propagate_error_writer_t::set_error_writer_fn' (compiling source file C:\Users\adeel\Source\Repos\runtime2\src\installer\corehost\cli\comhost\comhost.cpp) [C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x86.Debug\corehost\cli\comhost\comhost.vcxproj]

I think it is because this function requires __cdecl?

Copy link
Member

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

Copy link
Member Author

@am11 am11 Mar 20, 2020

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.

endif (CLR_CMAKE_HOST_ARCH_I386)

add_compile_options($<$<OR:$<CONFIG:Release>,$<CONFIG:Relwithdebinfo>>:/GL>)
add_compile_options($<$<OR:$<OR:$<CONFIG:Release>,$<CONFIG:Relwithdebinfo>>,$<CONFIG:Checked>>:/O1>)

Expand Down Expand Up @@ -523,3 +522,61 @@ endif(CLR_CMAKE_ENABLE_CODE_COVERAGE)
if (CMAKE_BUILD_TOOL STREQUAL nmake)
set(CMAKE_RC_CREATE_SHARED_LIBRARY "${CMAKE_CXX_CREATE_SHARED_LIBRARY}")
endif(CMAKE_BUILD_TOOL STREQUAL nmake)

# Ensure other tools are present
if (CLR_CMAKE_HOST_WIN32)
if(CLR_CMAKE_HOST_ARCH_ARM)

# Confirm that Windows SDK is present
if(NOT DEFINED CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION OR CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION STREQUAL "" )
message(FATAL_ERROR "Windows SDK is required for the Arm32 build.")
else()
message("Using Windows SDK version ${CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION}")
endif()

# Explicitly specify the assembler to be used for Arm32 compile
file(TO_CMAKE_PATH "$ENV{VCToolsInstallDir}\\bin\\HostX86\\arm\\armasm.exe" CMAKE_ASM_COMPILER)

set(CMAKE_ASM_MASM_COMPILER ${CMAKE_ASM_COMPILER})
message("CMAKE_ASM_MASM_COMPILER explicitly set to: ${CMAKE_ASM_MASM_COMPILER}")

# Enable generic assembly compilation to avoid CMake generate VS proj files that explicitly
# use ml[64].exe as the assembler.
enable_language(ASM)
elseif(CLR_CMAKE_HOST_ARCH_ARM64)

# Confirm that Windows SDK is present
if(NOT DEFINED CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION OR CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION STREQUAL "" )
message(FATAL_ERROR "Windows SDK is required for the ARM64 build.")
else()
message("Using Windows SDK version ${CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION}")
endif()

# Explicitly specify the assembler to be used for Arm64 compile
file(TO_CMAKE_PATH "$ENV{VCToolsInstallDir}\\bin\\HostX86\\arm64\\armasm64.exe" CMAKE_ASM_COMPILER)

set(CMAKE_ASM_MASM_COMPILER ${CMAKE_ASM_COMPILER})
message("CMAKE_ASM_MASM_COMPILER explicitly set to: ${CMAKE_ASM_MASM_COMPILER}")

# Enable generic assembly compilation to avoid CMake generate VS proj files that explicitly
# use ml[64].exe as the assembler.
enable_language(ASM)
else()
enable_language(ASM_MASM)
endif()

# Ensure that MC is present
find_program(MC mc)
if (MC STREQUAL "MC-NOTFOUND")
message(FATAL_ERROR "MC not found")
endif()

else (CLR_CMAKE_HOST_WIN32)
enable_language(ASM)

# Ensure that awk is present
find_program(AWK awk)
if (AWK STREQUAL "AWK-NOTFOUND")
message(FATAL_ERROR "AWK not found")
endif()
endif(CLR_CMAKE_HOST_WIN32)
45 changes: 35 additions & 10 deletions eng/native/functions.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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 "")
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -289,6 +300,20 @@ function(strip_symbols targetName outputFilename skipStrip)
endif(CLR_CMAKE_HOST_UNIX)
endfunction()

function(install_symbols targetName destination_path)
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we start by removing the top-level option:

echo "-stripsymbols: skip native image generation."

Copy link
Member

Choose a reason for hiding this comment

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

II would do it in 3 steps in this order:

  • Make stripping the symbols unconditional in the cmake files (essentially just ignore the -stripsymbols option as part of this PR
  • Remove passing in the -stripsymbols to the build scripts in the CI / build lab scripts in a follow up PR
  • Remove the -stripsymbols option support from the build-commons.sh

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 build-commons.sh

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Expand All @@ -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)
Expand All @@ -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}
Expand Down
File renamed without changes.
96 changes: 8 additions & 88 deletions src/coreclr/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,20 @@ cmake_policy(SET CMP0042 NEW)
# Set the project name
project(CoreCLR)

include(${CLR_ENG_NATIVE_DIR}/configuretools.cmake)
include(${CLR_ENG_NATIVE_DIR}/configurecompiler.cmake)

if (CLR_CMAKE_HOST_WIN32)
message(STATUS "VS_PLATFORM_TOOLSET is ${CMAKE_VS_PLATFORM_TOOLSET}")
message(STATUS "VS_PLATFORM_NAME is ${CMAKE_VS_PLATFORM_NAME}")
endif (CLR_CMAKE_HOST_WIN32)

if(MSVC)
set(CMAKE_CXX_STANDARD_LIBRARIES "") # do not link against standard win32 libs i.e. kernel32, uuid, user32, etc.
if (CLR_CMAKE_HOST_ARCH_I386)
add_compile_options(/Gz)
endif (CLR_CMAKE_HOST_ARCH_I386)
endif (MSVC)

# Set commonly used directory names
set(CLR_DIR ${CMAKE_CURRENT_SOURCE_DIR})
set(VM_DIR ${CMAKE_CURRENT_SOURCE_DIR}/src/vm)
Expand All @@ -31,93 +38,6 @@ endif(CORECLR_SET_RPATH)

OPTION(CLR_CMAKE_ENABLE_CODE_COVERAGE "Enable code coverage" OFF)

# Ensure other tools are present
if (CLR_CMAKE_HOST_WIN32)
if(CLR_CMAKE_HOST_ARCH_ARM)

# Confirm that Windows SDK is present
if(NOT DEFINED CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION OR CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION STREQUAL "" )
message(FATAL_ERROR "Windows SDK is required for the Arm32 build.")
else()
message("Using Windows SDK version ${CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION}")
endif()

# Explicitly specify the assembler to be used for Arm32 compile
file(TO_CMAKE_PATH "$ENV{VCToolsInstallDir}\\bin\\HostX86\\arm\\armasm.exe" CMAKE_ASM_COMPILER)

set(CMAKE_ASM_MASM_COMPILER ${CMAKE_ASM_COMPILER})
message("CMAKE_ASM_MASM_COMPILER explicitly set to: ${CMAKE_ASM_MASM_COMPILER}")

# Enable generic assembly compilation to avoid CMake generate VS proj files that explicitly
# use ml[64].exe as the assembler.
enable_language(ASM)
elseif(CLR_CMAKE_HOST_ARCH_ARM64)

# Confirm that Windows SDK is present
if(NOT DEFINED CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION OR CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION STREQUAL "" )
message(FATAL_ERROR "Windows SDK is required for the ARM64 build.")
else()
message("Using Windows SDK version ${CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION}")
endif()

# Explicitly specify the assembler to be used for Arm64 compile
file(TO_CMAKE_PATH "$ENV{VCToolsInstallDir}\\bin\\HostX86\\arm64\\armasm64.exe" CMAKE_ASM_COMPILER)

set(CMAKE_ASM_MASM_COMPILER ${CMAKE_ASM_COMPILER})
message("CMAKE_ASM_MASM_COMPILER explicitly set to: ${CMAKE_ASM_MASM_COMPILER}")

# Enable generic assembly compilation to avoid CMake generate VS proj files that explicitly
# use ml[64].exe as the assembler.
enable_language(ASM)
else()
enable_language(ASM_MASM)
endif()

# Ensure that MC is present
find_program(MC mc)
if (MC STREQUAL "MC-NOTFOUND")
message(FATAL_ERROR "MC not found")
endif()

else (CLR_CMAKE_HOST_WIN32)
enable_language(ASM)

# Ensure that awk is present
find_program(AWK awk)
if (AWK STREQUAL "AWK-NOTFOUND")
message(FATAL_ERROR "AWK not found")
endif()

# 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 (CLR_CMAKE_HOST_DARWIN)

# 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()

endif()
endif(CLR_CMAKE_HOST_WIN32)

if(CLR_CMAKE_TARGET_ANDROID)
add_definitions(-DTARGET_ANDROID)
endif()

#----------------------------------------------------
# Configure compiler settings for environment
#----------------------------------------------------
include(configurecompiler.cmake)

#----------------------------------------------------
# Cross target Component build specific configuration
#----------------------------------------------------
Expand Down
7 changes: 4 additions & 3 deletions src/coreclr/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,16 @@ cmake_minimum_required(VERSION 3.14.2)
cmake_policy(SET CMP0042 NEW)
project(Tests)

include(${CLR_ENG_NATIVE_DIR}/configuretools.cmake)
include(${CLR_ENG_NATIVE_DIR}/configurecompiler.cmake)

set(INC_PLATFORM_DIR ${CMAKE_CURRENT_SOURCE_DIR}/src/Common/Platform)
if (CLR_CMAKE_TARGET_WIN32)
add_definitions(-DWINDOWS)
if (CLR_CMAKE_HOST_ARCH_I386)
add_compile_options(/Gz)
endif (CLR_CMAKE_HOST_ARCH_I386)
endif()

# Include global configure settings
include(${CMAKE_CURRENT_SOURCE_DIR}/../configurecompiler.cmake)
# Compile options

if (CLR_CMAKE_HOST_WIN32)
Expand Down
9 changes: 6 additions & 3 deletions src/installer/corehost/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ cmake_minimum_required(VERSION 3.14.2)

project(corehost)

include(${CLR_ENG_NATIVE_DIR}/configuretools.cmake)
include(../settings.cmake)
include(../functions.cmake)
include(${CLR_ENG_NATIVE_DIR}/configurecompiler.cmake)

if(MSVC)
add_compile_options(/W1)
endif()

add_subdirectory(cli)
4 changes: 2 additions & 2 deletions src/installer/corehost/cli/hostmisc/pal.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
#define LIB_PREFIX
#define MAKE_LIBNAME(NAME) (_X(NAME) _X(".dll"))
#define FALLBACK_HOST_RID _X("win10")
#elif defined(__APPLE__)
#elif defined(TARGET_DARWIN)
#define LIB_PREFIX _X("lib")
#define MAKE_LIBNAME(NAME) (LIB_PREFIX _X(NAME) _X(".dylib"))
#define FALLBACK_HOST_RID _X("osx.10.12")
Expand Down Expand Up @@ -176,7 +176,7 @@ namespace pal

#define __cdecl /* nothing */
#define __stdcall /* nothing */
#if !defined(__FreeBSD__)
#if !defined(TARGET_FREEBSD)
#define __fastcall /* nothing */
#else
#include <sys/types.h>
Expand Down
Loading