Disable C11 compat in libunwind

[agocke]

It looks like C11 and C++latest are incompatible in libunwind (possibly due to VS update). It doesn't appear that we need both flags and it's not clear what C11 compat would mean in this instance. This change removes it and just leaves C++latest.

Fixes #45763

[ghost]

Tagging subscribers to this area: @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

---

It looks like C11 and C++latest are incompatible in libunwind (possibly due to VS update). It doesn't appear that we need both flags and it's not clear what C11 compat would mean in this instance. This change removes it and just leaves C++latest.

Fixes #45763

Author:	agocke
Assignees:	-
Labels:	area-Infrastructure, area-PAL-coreclr
Milestone:	-

[am11]

libunwind is a C library, I think setting project(unwind C) on line 3 would be a more appropriate fix. cc @janvorli

[am11]

similar to

runtime/src/libraries/Native/Windows/System.IO.Compression.Native/zlib/CMakeLists.txt

Line 4 in 03059a9

project(zlib C)

and many other examples in the repo.

[agocke]

Hmm but if I delete C++latest I get C:\Users\andy\Code\runtime\src\coreclr\pal\src\exception\remote-unwind.cpp(1980): error C7555: use of designated initializers requires at least '/std:c++latest'

[janvorli]

We should be using c++11 everywhere and not c++ latest somewhere. Our policy has always been to move to a newer c++ standard if it was absolutely necessary. It is not clear to me how the c++latest got there, as the eng/native/configurecompiler.cmake that is the only place where such an option should be specified uses set(CMAKE_CXX_STANDARD 11). So I don't think we want to fix it the way this PR does it.

@am11 we have switched to compile everything in a single target executable as C++ in the past because of potential strict aliasing issues that could happen if C function got inlined into C++ one. Here is the PR where it was done: dotnet/coreclr#6801

[am11]

@janvorli, thanks for pointing out. I think what project(unwind C) (as opposed to project(unwind)) does is; it filters out the C++ flags (sub)project-wide, but I understand that this is not what we want to happen for coreclr.

Is this case different than:

runtime/src/libraries/Native/Unix/System.Globalization.Native/CMakeLists.txt

Line 1 in 3c0d0e6

project(System.Globalization.Native C)

now that we are also compiling globalization library as part of coreclr partition (without renaming sources .c to .cpp)? Looks like at the time of dotnet/coreclr#6801, we did not had libunwind sources in the project.

[janvorli]

The libunwind compiles fine as C++ and if I understand the issue correctly, the problem happens only with Ninja. So I'd prefer keeping libunwind being compiled as C++ and fix / workaround the problem that seems to be caused most likely by the CMake Ninja generator interpreting the set(CMAKE_CXX_STANDARD 11) incorrectly. If I read the issue description correctly, it happens only for Ninja and not for the msbuild generator.

The native libraries like System.Globalization.Native is a bit different case. Or maybe was a different case until the very recent change #44505. Before this change, even though the library was statically linked into the single exe host, the methods that were called from them were still exposed via PInvoke. So they wouldn't get inlined into C++ code. Actually even with the #44505, it still seems that inlining is unlikely to happen as the methods are invoked via function pointers in manually constructed tables. So while they are being compiled into the same executable, there is a clear separation between that code and the rest of the runtime. If that presumtion is weak and compiler would compile the table lookup aggressively into some kind of a switch with direct jumps to the target methods, we can always mark those methods as non-inlineable to make it 100% safe.

On the other hand, the unwind library methods are being called directly from PAL and they can get inlined into the other PAL code.

[sdmaclea]

@janvorli. I don't think this is a ninja problem but a problem that occurs on the latest VS dogfood compiler. But that is only my guess.

It is not clear to me how the c++latest got there

Probably my fault

The libunwind compiles fine as C++

I don't think it did for the Windows cross compile. It is possible/likely I fixed incorrectly...

[janvorli]

@sdmaclea So you can repro it with msbuild too? If that's the case, then I guess the fix might be to change set(CMAKE_CXX_STANDARD 11) to set(CMAKE_CXX_STANDARD 14) for Windows only, as it seems that cmake doesn't add any /std:xxx option to the compiler with the set(CMAKE_CXX_STANDARD 11), most likely due to the fact that there is no /std option for c++11, but there is one for c++14 and c++17. My guess is that Visual C++ was formerly using c++14 when no /std: option was specified and now it moved to using c++latest instead.

[sdmaclea]

@janvorli I don't have access to my Windows machine, So I haven't been able to repro it.

[sdmaclea]

My guess that it wasn't a ninja specific issue was influenced by my investigation for #42997 which was triggered by C11 support being introduced in the preview SDK.

[agocke]

This is definitely not a ninja problem -- we can no longer build on VS 2019 Preview

[agocke]

OK I checked -- this code absolutely requires c++latest (c++14 is not sufficient) or the code will not build with the new compiler. It cannot be combined with any C-specific language version.

[janvorli]

That's really strange since on Unix, it is built with c++11 just fine. What is the error you are hitting? I'd try myself, but I cannot currently reboot my machine which is needed to install the latest VS.

[agocke]

remote-unwind.cpp(1980): error C7555: use of designated initializers requires at least '/std:c++latest'

If this looks incorrect we should raise it with the C++ team that MSVC has a bug :)

[sdmaclea]

Based on @agocke comments I updated #45902 to add that option only to remote-unwind.cpp

That's really strange since on Unix, it is built with c++11

It could be code I added under #ifdef Windows...

[sdmaclea]

use of designated initializers

we should raise it with the C++ team that MSVC has a bug :)

Actually I this is a C/C++ extension that GCC/G++/Clang initially added for Unix. Looks like it was eventually standardized. I am guessing MSVC supports it in 'C' and in 'c++latest'. That is probably why I added the '/TC' option.

[agocke]

PR is obsolete, closiong