Fix strict aliasing violation from conditional typedef of wchar_t by building entire project as C++ for Unix

[choikwa]

This does bug does not affect Windows as there is a flag in cmake that makes MS compiler treat C files as C++. There is conditional typedef of wchar_t as either 'unsigned short' or 'char16_t' depending on whether compiler defines '__cplusplus'. This coupled with high opt with -fstrict-aliasing (default for -O2, -O3, -Os) allows for code motion across types by the compiler optimizations.

The most conservative fix is to compile C files as C++ files. This entails changes to the CMakeFiles and fixes from errors arise from compiling C files as C++, such as, but not limited to, const-correctness and more strict type system.

[choikwa]

I see the last failure in OS X jenkins is from

/Users/dotnet-bot/j/workspace/dotnet_coreclr/master/checked_osx_prtest/src/pal/inc/pal_mstypes.h:218:17: error: typedef redefinition with different types ('long' vs 'long long')
15:07:59 typedef __int64 int64_t;
15:07:59 ^
15:07:59 /usr/include/sys/_types/_int64_t.h:30:20: note: previous definition is here
15:07:59 typedef long long int64_t;

Does anyone know if it is valid to redefine fixed width integer types in OS X?

[choikwa]

The failures appear to be infra-related. Could anyone take a look and rerun them?

[choikwa]

#6746 appears to be related. "CodeTaskFactory" could not be loaded.

[choikwa]

@dotnet-bot test Ubuntu x64 Checked Build and Test please

[choikwa]

@dotnet-bot test Linux ARM Emulator Cross Release Build please
@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test please
@dotnet-bot test WIndows_NT x86 ryujit Checked Build and Test please

[choikwa]

@dotnet-bot test Linux ARM Emulator Cross Release Build please
@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test please

[choikwa]

@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test please

[choikwa]

@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test please

[choikwa]

Failure could be dup from #6857

[choikwa]

@janvorli could I have your code review please?

[choikwa]

Rerunning test since #6857 is fixed
@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test please

[jkotas]

Should the INPUTFN signature be fixed as well?

[choikwa]

It should already be correct signature. I thought only wide char needed consistency.

[jkotas]

@rahku @ramarag Is this the best to achieve the desired effect in cmake?

[jkotas]

@choikwa Is this fixing a theoretical bug, or have you actually seen case of something not working before this fix and working fine after this fix?

[ramarag]

i am just curious why where you not able to add ADD_DEFINITIONS (-xc++) or add_compile_options(-xc++) to top level CMAKE files ?

[choikwa]

@jkotas The bug was exposed by enabling LTO build (-flto) to the release build configuration. The failing testcase was PAL_wcsncat. I've observed in LLVM that the types "unsigned short" and "char16_t", which are typedefs for "wchar_t" do not alias. This was the precursor to GVN pass performing invalid code motion and making the test fail the assert that tests for the null byte at certain position.
@ramarag Adding top level option was explored, but I recall running into some issues related to CMake. I will update tomorrow.

[rahku]

I don't think changing every cmake file to add properties on sources is the correct way to fix this. Please find a better way.

[choikwa]

@ramarag The problem with adding top level option is that -std=C11 is incompatible with -xc++.

error: invalid argument '-std=c11' not allowed with 'C++/ObjC++'

And forcing CFLAG as -std=c++11 fails during CMake check steps where it compiles a dummy C file.

Building C object
CMakeFiles/cmTryCompileExec3984733045.dir/testCCompiler.c.o

/usr/local/bin/clang-3.5 -Wall -std=c++11 -o
CMakeFiles/cmTryCompileExec3984733045.dir/testCCompiler.c.o -c
/home/kchoi/git/coreclr/bin/obj/Linux.x64.Release/CMakeFiles/CMakeTmp/testCCompiler.c

error: invalid argument '-std=c++11' not allowed with 'C/ObjC'

[ghost]

there is a flag in cmake that makes MS compiler treat C files as C++

I think Windows builds are not using CMake, they have hand-rolled vcxproj files for Windows. CMake configurations for Win32 are not used anywhere. See build.cmd at root of repo.

forcing CFLAG as -std=c++11 fails during CMake check steps where it compiles a dummy C file.

Can we work out the w_char issue differently instead of building the whole project as C++, where building some objects as plain C has advantages (i.e. no libstdc++ link between obj and libc -> all the better).

[rahku]

@jasonwilliams200OK This is not correct. windows builds use the same cmake files as non-windows builds. It does not use hand-rolled vcxproj files.

[ghost]

@rahku, has it changed recently? Going by this conversation: https://github.com/dotnet/coreclr/issues/6642#issuecomment-238690742, I thought build.cmd doesn't actually make use of cmake.

[jkotas]

Windows C/C++ build is 100% cmake. It just does not take advantage of cmake configurations (autoconf equivalent).

[rahku]

windows msvc compiler(cl.exe) has a param /TP (https://msdn.microsoft.com/en-us/library/032xwy55.aspx) to compile c as c++ . This is being set for windows build https://github.com/dotnet/coreclr/blob/master/compileoptions.cmake#L72. It seems -xc++ is the corresponding option in clang http://www.keil.com/support/man/docs/armclang_intro/armclang_intro_chr1375778205120.htm . It says that all subsequent files irrespective of extension will be compiled as c++. Note that it only affects subsequent files. So in theory adding -xc++ as compile option should work for you.

[rahku]

Not sure about this but you could also try using CMAKE_CXX_SOURCE_FILE_EXTENSIONS variable.

[rahku]

If nothing else works you could use _add_library/_add_executable (https://github.com/dotnet/coreclr/blob/master/functions.cmake#L185) functions and set source file inside that function.

[choikwa]

@rahku (on latter comment), how does user know to add the C file to a list if s/he is adding one? This will be hard to maintain.

[rahku]

I meant in _add_library/_add_executable functions you could iterate over source list and set the required source file properties. _add_library/_add_executable are wrappers over add_library & add_executable. These functions(_add_library/_add_executable) are used in product cmakelists.txt instead of add_library & add_executable

how does user know to add the C file to a list if s/he is adding one?

wouldn't eventually the added C file be passed to _add_library/_add_executable it it needs compilation.

I think we should try other options before taking this route.

And forcing CFLAG as -std=c++11 fails during CMake check steps where it compiles a dummy C file.

maybe you should add -std=c++11 only for C++ flags and not for C

[choikwa]

@rahku I tried the first answer's two ways for CMAKE_CXX_SOURCE_FILE_EXTENSIONS, but ran into same '-std=c11 invalid for C++' issue.

It looks like _add_executable is searching from CLR_CROSS_COMPONENTS_LIST, and my fear was that if a user were to add a new C file, s/he would have to add it to a similar list. (Correct me if I totally misunderstood)

[choikwa]

@dotnet-bot test Linux ARM Emulator Cross Release Build
@dotnet-bot test OSX x64 Checked Build and Test
@dotnet-bot test Ubuntu x64 Checked Build and Test

[choikwa]

@dotnet-bot test Linux ARM Emulator Cross Release Build

[janvorli]

@choikwa Once the "throws()" is fixed properly as I've suggested in my previous comment, we can merge this change in.

[janvorli]

Thanks @choikwa for fixing the problem!

[swgillespie]

I'm getting a build error locally on OSX when building master 4149bd2070e0a8beec6dedb238e6fd832a39611b:

/Users/sean/Documents/workspace/clr/coreclr/src/pal/tests/palsuite/threading/ExitThread/test1/test1.cpp:51:12: error: cannot initialize a variable of type 'LPVOID' (aka 'void *') with an lvalue of type
      'LPTHREAD_START_ROUTINE' (aka 'unsigned int (*)(void *)')
    LPVOID lpParameter = lpStartAddress;
           ^             ~~~~~~~~~~~~~~
1 error generated.

It looks like it's happening in the CI too (http://dotnet-ci.cloudapp.net/job/dotnet_coreclr/job/master/job/checked_osx_prtest/5578/console), could that be related to this?

[janvorli]

That's strange - the CI tests were green when I've merged in the change.

[janvorli]

Interesting - the OSX build in this PR failed, but the CI has reported the job as green!
@mmitche any idea what could be wrong?

[mmitche]

@janvorli I don't see a failure. Can you point me to it?

[janvorli]

@mmitche click on "details" of the OSX build, then open the console output and click on the dotnet_coreclr » master » checked_osx_prtest link. You will see errors reported there.

[janvorli]

@mmitche ah, I am sorry, my mistake - I haven't noticed it doesn't show the related job's errors, but probably the last job's ones.

[mmitche]

Yes, that's correct. The overall job view is the last job that completed's failures.

[janvorli]

The machine where the job has completed ok has AppleClang 7.0.2.7000181, the one where it has failed has AppleClang 7.3.0.7030031. So the older clang was ignoring the need for cast.
I will send out PR for the build break fix in a second.