Refactor native build to remove the compiler-override files #23897

jkoritzinsky · 2019-04-11T16:59:01Z

Refactor the native build to remove the compiler-override files
Use CMake 3.0 on our systems that have 2.X and 3.0 installed.
Set CMAKE_USER_MAKE_RULES_OVERRIDE to an empty string to update the CMake cache so CMake doesn't go looking for the compiler-override files.

Move CMAKE_EXPORT_COMPILE_COMMANDS to top level instead of in gen-buildsys-*. Define the CMake install prefix in gen-buildsys-* instead of pulling from an environment variable. Define C++ standard as CMake property instead of as flag. Move CLR_DEFINES_*_INIT out of overrides and into configurecompiler.cmake Move flags that generate debug info into configurecompiler.cmake Remove the CMAKE_USER_RULES_OVERRIDE files. Add cmake version output for determining what cmake versions each CI system has. Fix syntax in gen-buildsys-clang. Change add_compile_definitions back to add_definitions Add -D prefix for adding definitions via add_definitions Remove extraneous double-quote Change default config definition adding to the syntax in master Switch back to old CMAKE_<LANG>_FLAGS way of setting the language standards and try to go back to 2.8.12 minimum Switch back setting compile definitions for non-Windows branch too. Use SET with CMAKE_<LANG>_FLAGS. Convert some usages of appending to CMAKE_<LANG>_FLAGS to add_compile_options where possible. Set CMAKE_<LANG>_FLAGS_INIT instead of CMAKE_<LANG>_FLAGS Make sure configureopimitzation.cmake is included correctly in test build. Try to add brackets to get the Linux ARM compilation working correctly. Define standard language version in configurecompiler.cmake instead of root CMakeLists (so tests get it) Try to move langauge standard check to configure.cmake define language standard in each root CMakeLists.txt Fix off-Windows test build. Set CMAKE_EXPORT_COMPILE_COMMANDS after the project() call

…lds after building on a branch that had it set to a path.

…-cleanup

jkoritzinsky · 2019-05-15T00:33:06Z

@janvorli @am11 can you take a look when you have a chance?

CMakeLists.txt

src/ToolBox/SOS/lldbplugin/CMakeLists.txt

src/pal/tools/gen-buildsys-clang.sh

…about on C files.

…to add_compile_options

configurecompiler.cmake

franksinankaya · 2019-05-15T23:08:22Z

I think I want to do a build with gcc on this pull request when it comes to finalizing stage. Please let me know.

jkoritzinsky · 2019-05-16T17:38:30Z

@franksinankaya can you do a build with GCC?

franksinankaya · 2019-05-16T18:29:36Z

96% compiled successfully.

"-Werror=conversion-null" doesn't seem to propagate for some reason.

src/ToolBox/SOS/Strike/strike.cpp:14788:24: error: converting to non-pointer type ‘TADDR {aka long unsigned int}’ from NULL [-Werror=conversion-null]
TADDR taLTOH = NULL;

You can clone this repo and build with ./build.sh -gcc

https://github.com/franksinankaya/coreclr/tree/cmake-cleanup

to reproduce.

jkoritzinsky · 2019-05-16T20:01:15Z

@franksinankaya looks like that one was another precedence issue between CMAKE_CXX_FLAGS and add_compile_options. I've pushed out a fix. Let me know if it works for you.

franksinankaya · 2019-05-16T20:41:49Z

Thanks this version worked.

jkoritzinsky · 2019-05-20T23:29:20Z

@janvorli can you take another review pass?

am11 · 2019-05-21T03:21:00Z

tests/CMakeLists.txt

-include(${CMAKE_CURRENT_SOURCE_DIR}/../configurecompiler.cmake)
+if (NOT WIN32)
+  set(CMAKE_CXX_FLAGS_INIT "${CMAKE_CXX_FLAGS_INIT} -std=c++11"  CACHE STRING "Flags used by the compiler during all build types.")
+  set(CMAKE_C_FLAGS_INIT "${CMAKE_C_FLAGS_INIT} -std=c11"  CACHE STRING "Flags used by the compiler during all build types.")


The only C11 feature used in CoreCLR is _Static_assert in src/corefx/System.Globalization.Native/pal_compiler.h. The rest of libunwind, Globalization and few other C source files are gnu99 compatible (c99 plus GNU extensions: named variadic macros and statement expressions etc.). However, -pedantically, I think we need -std=gnu11 -pedantic -Wno-gnu -Wno-variadic-macros -Wno-format-pedantic -Wno-pointer-arith -Wno-keyword-macro for C in CoreCLR.

CoreFX uses gnu99 (the initial reason was: mono compatibility) but the code has C11's _Generic as well as _Static_assert, which makes it non-conforming to the compiler's -std flag. IMO, we should select the minimal standard that is actually used by the code (-pedantic highlights where we violate the conformance).

The _Static_assert is optional in both coreclr and corefx (we check if it exists and if not, we use another way). As for the _Generic in corefx, you are right.
I would not change this stuff in this change though.

Sure, btw I have this fixed in ongoing solaris port branch, I will rebase once this PR is merged.
gcc 7 on SmartOS amd64 throws the following error on building Globalization target with -std=c11, which gets fixed with -std=gnu99 or -std=gnu11 flag (without changing the code or other flags):

/datadrive/projects/coreclr/src/corefx/System.Globalization.Native/pal_calendarData.c:84:9: error: implicit declaration of function 'strcasecmp'; did you mean 'u_strcasecmp'? [-Werror=implicit-function-declaration] if (strcasecmp(calendarName, GREGORIAN_NAME) == 0) ^~~~~~~~~~ u_strcasecmp

That warning I would think is actually a problem you should look at fixing (the declaration of strcasecmp isn't found anywhere) even though the "implicit definition" behavior happens to match the signature.

configureoptimization.cmake

src/jit/CMakeLists.txt

src/pal/tools/gen-buildsys-clang.sh

tests/CMakeLists.txt

… into cmake-cleanup

…-cleanup

jkoritzinsky · 2019-05-29T21:35:17Z

@janvorli any more feedback on this PR?

janvorli

LGTM, thank you!

…oreclr#23897) * On systems that have both cmake 2 and cmake 3, use cmake 3. Move CMAKE_EXPORT_COMPILE_COMMANDS to top level instead of in gen-buildsys-*. Define the CMake install prefix in gen-buildsys-* instead of pulling from an environment variable. Define C++ standard as CMake property instead of as flag. Move CLR_DEFINES_*_INIT out of overrides and into configurecompiler.cmake Move flags that generate debug info into configurecompiler.cmake Remove the CMAKE_USER_RULES_OVERRIDE files. Add cmake version output for determining what cmake versions each CI system has. Fix syntax in gen-buildsys-clang. Change add_compile_definitions back to add_definitions Add -D prefix for adding definitions via add_definitions Remove extraneous double-quote Change default config definition adding to the syntax in master Switch back to old CMAKE_<LANG>_FLAGS way of setting the language standards and try to go back to 2.8.12 minimum Switch back setting compile definitions for non-Windows branch too. Use SET with CMAKE_<LANG>_FLAGS. Convert some usages of appending to CMAKE_<LANG>_FLAGS to add_compile_options where possible. Set CMAKE_<LANG>_FLAGS_INIT instead of CMAKE_<LANG>_FLAGS Make sure configureopimitzation.cmake is included correctly in test build. Try to add brackets to get the Linux ARM compilation working correctly. Define standard language version in configurecompiler.cmake instead of root CMakeLists (so tests get it) Try to move langauge standard check to configure.cmake define language standard in each root CMakeLists.txt Fix off-Windows test build. Set CMAKE_EXPORT_COMPILE_COMMANDS after the project() call * Set CMAKE_USER_MAKE_RULES_OVERRIDE to "" to not break incremental builds after building on a branch that had it set to a path. * Remove CMake version output. * Move comment outside of multiline command. * Retry setting CMAKE_USER_MAKE_RULES_OVERRIDE * Remove unnecessary variable wrappers. * Simplify cmake 3 resolution. * Explicitly use CMAKE_CXX_FLAGS for C++-only flags that GCC complains about on C files. * Set -Wall via CMAKE_<LANG>_FLAGS until we can move all flag settings to add_compile_options * Fix typos * Another temporary precedence issue. * include configureoptimization.cmake in configurecompiler.cmake * Move setting CMAKE_EXPORT_COMPILE_COMMANDS to configurecompiler.cmake. * Rename configure.cmake -> verify_lto.cmake. * Fix path to verify-lto * Try using CMAKE_<LANG>_FLAGS instead of CMAKE_<LANG>_FLAGS_INIT. * Revert name change to configure.camek Commit migrated from dotnet/coreclr@38f121b

jkoritzinsky added the area-Build label Apr 11, 2019

jkoritzinsky force-pushed the cmake-cleanup branch 3 times, most recently from a6bd683 to 3fe5254 Compare April 11, 2019 19:06

jkoritzinsky force-pushed the cmake-cleanup branch from bf730d5 to 7bb015e Compare April 12, 2019 21:51

jkoritzinsky added 4 commits April 15, 2019 11:54

Set CMAKE_USER_MAKE_RULES_OVERRIDE to "" to not break incremental bui…

61dda81

…lds after building on a branch that had it set to a path.

Remove CMake version output.

6b052fc

Move comment outside of multiline command.

d9513d6

Retry setting CMAKE_USER_MAKE_RULES_OVERRIDE

0ba405c

jkoritzinsky changed the title ~~WIP Refactor native build to remove the compiler-override files~~ Refactor native build to remove the compiler-override files Apr 16, 2019

jkoritzinsky marked this pull request as ready for review April 16, 2019 20:30

jkoritzinsky requested a review from janvorli April 16, 2019 20:30

Merge branch 'master' of https://github.com/dotnet/coreclr into cmake…

a8c8534

…-cleanup

janvorli reviewed May 15, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

janvorli reviewed May 15, 2019

View reviewed changes

src/ToolBox/SOS/lldbplugin/CMakeLists.txt Outdated Show resolved Hide resolved

Remove unnecessary variable wrappers.

9a4d6c9

janvorli reviewed May 15, 2019

View reviewed changes

src/pal/tools/gen-buildsys-clang.sh Outdated Show resolved Hide resolved

jkoritzinsky added 3 commits May 14, 2019 17:45

Simplify cmake 3 resolution.

4482c15

Explicitly use CMAKE_CXX_FLAGS for C++-only flags that GCC complains …

cc32d03

…about on C files.

Set -Wall via CMAKE_<LANG>_FLAGS until we can move all flag settings …

d7ddc40

…to add_compile_options

franksinankaya reviewed May 15, 2019

View reviewed changes

configurecompiler.cmake Outdated Show resolved Hide resolved

Fix typos

f541842

Another temporary precedence issue.

0684dcc

am11 reviewed May 21, 2019

View reviewed changes

janvorli reviewed May 23, 2019

View reviewed changes

configureoptimization.cmake Show resolved Hide resolved

src/jit/CMakeLists.txt Show resolved Hide resolved

src/pal/tools/gen-buildsys-clang.sh Show resolved Hide resolved

tests/CMakeLists.txt Outdated Show resolved Hide resolved

tests/CMakeLists.txt Outdated Show resolved Hide resolved

jkoritzinsky added 8 commits May 28, 2019 12:08

include configureoptimization.cmake in configurecompiler.cmake

a30321c

Move setting CMAKE_EXPORT_COMPILE_COMMANDS to configurecompiler.cmake.

aff5a49

Rename configure.cmake -> verify_lto.cmake.

a501d88

Merge branch 'cmake-cleanup' of https://github.com/jkoritzinsky/coreclr…

0ba74b4

… into cmake-cleanup

Fix path to verify-lto

735016e

Try using CMAKE_<LANG>_FLAGS instead of CMAKE_<LANG>_FLAGS_INIT.

276a26f

Merge branch 'master' of https://github.com/dotnet/coreclr into cmake…

e374ffd

…-cleanup

Revert name change to configure.camek

5d38198

janvorli approved these changes May 29, 2019

View reviewed changes

jkoritzinsky merged commit 38f121b into dotnet:master May 29, 2019

jkoritzinsky deleted the cmake-cleanup branch May 29, 2019 21:40

jashook added area-Infrastructure-coreclr and removed area-Build labels Nov 11, 2019

brianrob mentioned this pull request Jan 31, 2020

Windows PGO Optimization Broken Due to #23897 dotnet/runtime#12786

Closed

janvorli mentioned this pull request Jun 8, 2021

VC++ compiler optimization switches dotnet/runtime#53849

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor native build to remove the compiler-override files #23897

Refactor native build to remove the compiler-override files #23897

jkoritzinsky commented Apr 11, 2019 •

edited

Loading

jkoritzinsky commented May 15, 2019

franksinankaya commented May 15, 2019

jkoritzinsky commented May 16, 2019

franksinankaya commented May 16, 2019 •

edited

Loading

jkoritzinsky commented May 16, 2019

franksinankaya commented May 16, 2019

jkoritzinsky commented May 20, 2019

am11 May 21, 2019 •

edited

Loading

janvorli May 23, 2019

am11 May 25, 2019 •

edited

Loading

jkoritzinsky May 28, 2019

jkoritzinsky commented May 29, 2019

janvorli left a comment

Refactor native build to remove the compiler-override files #23897

Refactor native build to remove the compiler-override files #23897

Conversation

jkoritzinsky commented Apr 11, 2019 • edited Loading

jkoritzinsky commented May 15, 2019

franksinankaya commented May 15, 2019

jkoritzinsky commented May 16, 2019

franksinankaya commented May 16, 2019 • edited Loading

jkoritzinsky commented May 16, 2019

franksinankaya commented May 16, 2019

jkoritzinsky commented May 20, 2019

am11 May 21, 2019 • edited Loading

Choose a reason for hiding this comment

janvorli May 23, 2019

Choose a reason for hiding this comment

am11 May 25, 2019 • edited Loading

Choose a reason for hiding this comment

jkoritzinsky May 28, 2019

Choose a reason for hiding this comment

jkoritzinsky commented May 29, 2019

janvorli left a comment

Choose a reason for hiding this comment

jkoritzinsky commented Apr 11, 2019 •

edited

Loading

franksinankaya commented May 16, 2019 •

edited

Loading

am11 May 21, 2019 •

edited

Loading

am11 May 25, 2019 •

edited

Loading