Enable checked builds of CoreCLR.

[erozenfeld]

In checked builds coreclr, mscorlib, and the test are built optimized
but assertion checking is on. This adds additional coverage (the jit is
optimizing and assertion checking is on), speeds up testing compared to debug,
and allows testing JIT stress modes.

This doesn't affect CoreFX.

Several tests are currently failing in checked configuration due to newly
discovered bugs (JIT asserts). We didn't see these asserts in debug mode
because by default JIT is in minopt mode; we didn't see these bugs in release
mode because assertion checking is off. I will file the bugs once checked build
changes are in.

/cc @dotnet/jit-contrib

[erozenfeld]

@mmitche PTAL

[janvorli]

@Maoni0, @jkotas , is this change is ok even for desktop CLR?

[jkotas]

This change is ok. But is there a good reason why you are not enabling WRITE_BARRIER_CHECK for checked builds?

It is enabled for internal checked builds. If you disable it for checked builds, COMPlus_HeapVerify=2 stress mode will be no-op.

[erozenfeld]

WRITE_BARRIER_CHECK is NOT defined for internal checked builds and that is the reason I didn't define it for checked builds here.

[jkotas]

It is defined for internal checked builds. Otherwise the build break you have run into and that you have fixed by correcting this ifdef would show up in the internal checked builds as well.

WRITE_BARRIER_CHECK is defined in two places for the internal builds:

• clr.props that defines it for no opt builds only. You have probably looked at just this one that confused you.
• vm.settings that defines it for both internal debug and checked builds

The seconds one is super set of the first one. It would be nice if you can delete the redundant definition of WRITE_BARRIER_CHECK in clr.props and move the WRITE_BARRIER_CHECK definition for CMake build to be under src\vm so that it is in sync with the internal build scripts.

[erozenfeld]

Thank you for the explanation.
I removed the redundant definition of WRITE_BARRIER_CHECK in clr.props.
I couldn't figure out how to add the WRITE_BARRIER_CHECK definition for CMake build to be under src\vm so I just updated src/pal/tools/gen-buildsys-clang.sh and src/pal/tools/windows-compiler-override.txt to define WRITE_BARRIER_CHECK the same way as for debug builds.

[LLITCHEV]

Adding a definition under src/vm for CMake basically means to add something like the following for the right configurations - CHECKED, DEBUG maybe:
if (CLR_CMAKE_PLATFORM_LINUX)
add_definitions(-DFEATURE_EVENTSOURCE_XPLAT=1)
endif (CLR_CMAKE_PLATFORM_LINUX)
(Note this is just an example taken from the top level CMakeLists.txt (in your top clone directory.)
For your case it would be something like:
if (UPPERCASE_CMAKE_BUILD_TYPE STREQUAL DEBUG)
# Maybe needs a check also for CHECKED?
add_definitions(-DWRITE_BARRIER_CHECK=1)
endif((UPPERCASE_CMAKE_BUILD_TYPE STREQUAL DEBUG)
I think that would be better than polluting all the other project's build definitions.
Otherwise this looks fine to me. Thanks for adding it!

[erozenfeld]

The question is how to check the build type correctly so that it works both on windows and linux. I don't think what you are suggesting will work for multi-configuration toolset. There is no UPPERCASE_CMAKE_BUILD_TYPE there.

[LLITCHEV]

I see what you are saying. Took a look. The top level CMakeLists.txt defines the UPPERCASE_CMAKE_BUILD_TYPE. It is defined only for non-Windows platforms.
elseif (CLR_CMAKE_PLATFORM_UNIX)
..............

Use uppercase CMAKE_BUILD_TYPE for the string comparisons below

string(TOUPPER ${CMAKE_BUILD_TYPE} UPPERCASE_CMAKE_BUILD_TYPE)

You could probably define it for all the builds. Or you could use the $CMAKE_BUILD_TYPE. CMAKE_BUILD_TYPE should be always defined for single configuration generators - like makefiles.
This is described here:
https://cmake.org/Wiki/CMake_Useful_Variables
For Windows builds you can use code similar to the one at line 201 in the top level CMakeLists.txt to append the definitions for Checked/Debug configurations, I think. Maybe something like this:
if (WIN32)

For multi-configuration toolset (as Visual Studio)

set the different configuration defines.

foreach (Config DEBUG CHECKED)
set_property(DIRECTORY APPEND PROPERTY COMPILE_DEFINITIONS $<$CONFIG:${Config}:-DWRITE_BARRIER_CHECK=1>)
endforeach (Config)

[erozenfeld]

Ok, I made the change to define WRITE_BARRIER_CHECK for both debug and checked but only under src\vm.

[jkotas]

LGTM

[mmitche]

Is there a reason there is NOVCFEATURE here for Checked but not for Release?

[erozenfeld]

@mmitche The only info on /NOVCFEATURE that I found is "/NOVCFEATURE forces linker to emit old .pdb format. It is required for scan.exe tool to work". Looks like scan.exe is a static contract analyzer. Maybe we are not running it on release binaries? Let me know if you know more about /NOVCFEATURE or scan.exe or think that /NOVCFEATURE should be passed when linking release binaries. For checked binaries I just matched what we do for debug binaries.

[jkotas]

The contract analyzer (scan.exe) works on debug/checked binaries only. We do not run it on release binaries.

[mmitche]

Aside from the one question LGTM