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

Enable /clr C++20 support #3194

Merged
merged 38 commits into from
Nov 8, 2022
Merged

Conversation

StephanTLavavej
Copy link
Member

Issues

Fixes #838, fixes #955, fixes #980.

Tracking issue for /clr compiler bugs: #3193

Summary

The compiler team is working on adding support for C++20 to the /clr compiler option, with @tgani-msft leading this effort. While this is not yet supported for production use (and requires an undocumented/unsupported compiler front-end option), we need to add test coverage to the STL now, in order to prevent regressions during the ongoing compiler work and as we continue to change the library.

Additionally, because we're going to receive /clr compiler bugfixes, and because a lot has changed since we added C++11 multithreading and C++17 parallel algorithms (notably, the removal of our ConcRT dependencies), I'm enabling /clr support for headers and classes where we previously blocked it. Ultimately, this should result in a significant simplification of our codebase, as we'll need far fewer special cases for /clr. (The original idea was "it just works".) In the near term, the simplification is partial - we can drop some product code workarounds but need to add others, and we can drop 3 "native matrices" completely but need to add numerous test workarounds.

A few notes:

  • Only C++20 is being enabled at this time, not C++23. Thus, there should be relatively little impact to our ongoing C++23 work.
  • /clr:pure will not be changed at all.
  • Using some features will cause the compiler to fallback to native codegen (invisibly, if everything works). The relevant cases for the STL are:
    • alignas
    • C-style varargs
    • Compiler intrinsics
    • Non-__clrcall virtual function call thunks (affects P0896R4_ranges_algorithm_machinery)
    • Coroutines
  • Header units and modules will not be supported for /clr at this time.

I've structured this PR as a series of fine-grained commits for easier reviewing.

Commits

  • Enable product code for /clr.
    • Things that were blocked for _M_CEE (i.e. /clr or /clr:pure) are now blocked for _M_CEE_PURE only.
  • Use _EmptyLockit for /clr:pure only.
    • I can't imagine how skipping _Locinfo::_Lock could have been correct, so let's enable it for /clr.
    • There should be no mix-and-match issues at link time, as only /clr:pure performs the "metadata merge" that rejects such data member changes.
    • There should be no mix-and-match issues at runtime, as old TUs weren't synchronizing at all.
    • Defining _EmptyLockit for /clr:pure only shouldn't affect the export surface - I don't see any exports in msvcp, or symbols in msvcm, presumably because this class was trivial.
  • Avoid warning LNK4248 for _Mtx_t/_Cnd_t.
    • /clr dislikes pointers to forward-declared classes. We can use void*, similar to what <execution> already does (but we don't need to exactly imitate it, since we don't mention _Mtx_internal_imp_t/_Cnd_internal_imp_t otherwise):

      STL/stl/inc/execution

      Lines 32 to 44 in c873cf0

      #ifdef _M_CEE
      using __std_TP_WORK = void;
      using __std_TP_CALLBACK_INSTANCE = void;
      using __std_TP_CALLBACK_ENVIRON = void;
      #else // ^^^ _M_CEE ^^^ // vvv !_M_CEE vvv
      struct __std_TP_WORK; // not defined
      struct __std_TP_CALLBACK_INSTANCE; // not defined
      struct __std_TP_CALLBACK_ENVIRON; // not defined
      #endif // _M_CEE
      using __std_PTP_WORK = __std_TP_WORK*;
      using __std_PTP_CALLBACK_INSTANCE = __std_TP_CALLBACK_INSTANCE*;
      using __std_PTP_CALLBACK_ENVIRON = __std_TP_CALLBACK_ENVIRON*;
    • There's no bincompat impact because the affected separately compiled signatures are extern "C".
    • Cleanup for native code: avoid elaborated-type-specifiers. (We conventionally avoid introducing forward declarations while forming other types.)
  • Cleanup: Define _RELIABILITY_CONTRACT for _CRTBLD only.
    • This is needed by xlock.cpp and xmtx.cpp, but nowhere else.
  • Replace native_matrix.lst with impure_matrix.lst.
  • Replace native_17_matrix.lst with usual_17_matrix.lst (which has /clr but not /clr:pure).
  • Replace env_threads.lst with env_minus_pure.lst.
  • Enable /clr coverage in tests.
    • We can use __cpp_lib_execution instead of manually defining HAS_PARALLEL_ALGORITHMS.
    • P0433R2_deduction_guides can use things unconditionally because it uses usual_17_matrix.lst, which doesn't test /clr:pure.
  • Reported VSO-1658184 "/clr silent bad codegen with std::_Signed128 multiplication".
  • Reported VSO-1659383 '/clr atomic and thread emit error C2711 "this function cannot be compiled as managed" instead of falling back to native codegen'.
  • Reported VSO-1659408 '/clr alignas emits error C2711 "this function cannot be compiled as managed" instead of falling back to native codegen'.
    • P0040R3_extending_memory_management_tools has always worked around this bug; now it can be properly commented.
    • In range_algorithm_support.hpp, I'm extracting the often-repeated holder type, and applying a similar workaround.
    • All of P0660R10_stop_token and P1135R6_atomic_flag_test need to be skipped.
  • Reported VSO-1659496 "/clr emits bogus error C2079 "uses undefined struct 'Incomplete'" despite std::variant's ADL defenses".
  • Reported VSO-1659511 "/clr runtime hang when calling std::async in a global variable's constructor".
  • Reported VSO-1659489 "/clr System.AccessViolationException with parallel std::mismatch".
    • We need to skip this test.
  • Reported VSO-1659695 "/clr x86 runtime assertions/crashes with parallel algorithms".
    • We also need to skip these tests.
  • Reported VSO-1663104 "/clr rejects the __ceilf/__floorf intrinsics with error LNK2020/LNK2001 instead of accepting or falling back to native codegen".
  • Upgrade test coverage from /clr /std:c++17 to /clr /std:c++20 /d1clrcxxstd.
    • Note that we've never spent resources on testing multiple Standard modes with /clr. The idea is that we already have extensive test coverage of 14/17/20/23 modes for native code, so for /clr we can just use the highest mode where the most stuff is enabled. (This is fine because very little code changes meaning under different Standard modes.)
  • Prepare by extracting /EHsc from C++20 matrices.
    • Makes the next commit simpler.
  • Add /clr /std:c++20 /d1clrcxxstd to C++20 and coroutine matrices.
  • Reported VSO-1663233 "/clr C++20 rejects <coroutine> with error C3861 "'__builtin_coro_done': identifier not found" instead of falling back to native codegen".
  • Reported VSO-1663257 "/clr C++20 rejects P0428R2 Familiar template syntax for generic lambdas".
    • These product code workarounds aren't too bad, but the original code was nicer, so this isn't a perma-workaround.
  • Reported VSO-1664293 "/clr C++20 chk assertion failed: rhs.is_lvalue() in constexpr.cpp".
  • Reported VSO-1664341 "/clr C++20 System.NullReferenceException when calling ranges algorithms with PMD projections".
    • We can use lambdas instead of PMDs in product and test code.
  • Reported VSO-1664382 "/clr C++20 emits bogus error C2127 "illegal initialization of 'constinit' entity with a non-constant expression"".
  • Reported VSO-1664463 "/clr C++20 alignas emits error C3821 'managed type or function cannot be used in an unmanaged function' instead of falling back to native codegen".
    • We have to comment out these /clr C++20 configurations entirely (no targeted workaround was possible).
  • Reported VSO-1665481 "/clr C++20 silent bad codegen for complex<float> division".
  • Reported VSO-1665606 "/clr C++20 backend ICE with counted_iterator's _Same_sequence".
    • Workaround: Disable this IDL check for /clr.
    • Cleanup that also applies to native code: Guard _Same_sequence with IDL != 0 (as it is otherwise unused).
  • Reported VSO-1665654 "/clr C++20 System.AccessViolationException with atomic<shared_ptr>".
  • Reported VSO-1665663 "/clr C++20 source_location has incorrect column information within a lambda".
  • Reported VSO-1665670 "/clr C++20 incorrectly reports input_iterator etc. concepts as being true for 'bad' iterators".
    • We also need to silence warning C4793, which is by design here.
  • Reported VSO-1665674 "/clr C++20 has incorrect results for various ranges concepts in P0896R4_ranges_iterator_machinery".
  • Reported VSO-1666161 "/clr C++20 silent bad codegen for bit_cast<float>(nan_uint)".
  • Reported VSO-1666178 "/clr C++20 x64 /O2 backend assertion SY_ISUSERVAR(sym) || SY_ISARGUMENT(sym)".
  • Reported VSO-1666180 "/clr C++20 x64 System.AccessViolationException with views::iota and views::transform".
  • Perma-workaround VSO-1663301 "/clr C++20 emits bogus error C2131 'expression did not evaluate to a constant' when a lambda tests a constexpr variable defined outside".
    • This is permanent because it's strictly better for _Is_ymd to be an indoor kitty.
  • Avoid warnings C4561 and C4575 for __fastcall and __vectorcall.
    • These are by design.
  • Avoid using <cfenv> macros when they aren't available for /clr.
    • This is a UCRT restriction; I'm unsure if it's absolutely fundamental, but it isn't changing any time soon. Most of the <cfenv> usage is guarded by _M_FP_STRICT (which this test isn't activating for /clr), so just a bit more restructuring is necessary.
  • Comment why we aren't testing P0088R3_variant with /clr C++20.

P0088R3_variant compiler memory consumption

This test is already extremely stressful for native C++20. /clr pushes it over the edge and causes obnoxious test timeouts that the internal test harness doesn't recover from (they also wipe out the results for unrelated tests that happen to be running at the same time).

I have not yet filed an issue about decomposing this test, which probably makes me a bad kitty yet again. However, the reason is at least commented here.

My measurements with the internal x86chk compiler (peak working set is reported by the undocumented/unsupported option /d1reportMemorySummary):

Mode Std EXE (bytes) OBJ (bytes) Peak working set (bytes)
Native C++17 594,944 9,113,398 948,101,120
Native C++20 2,481,664 33,528,885 3,033,104,384
/clr C++17 12,517,888 53,675,003 1,111,109,632
/clr C++20 41,550,336 198,202,365 3,107,594,240

I don't see any exports in msvcp, or symbols in msvcm, presumably because this class was trivial.
Same technique as <execution>; the affected separately compiled signatures are extern "C".

Also, avoid elaborated-type-specifiers.
P0433R2_deduction_guides uses usual_17_matrix.lst, which doesn't test /clr:pure.
…unction cannot be compiled as managed" instead of falling back to native codegen'.
…annot be compiled as managed" instead of falling back to native codegen'.
…ruct 'Incomplete'" despite std::variant's ADL defenses".
…th error LNK2020/LNK2001 instead of accepting or falling back to native codegen".
… "'__builtin_coro_done': identifier not found" instead of falling back to native codegen".
…alling ranges algorithms with PMD projections".
…tialization of 'constinit' entity with a non-constant expression"".
…ype or function cannot be used in an unmanaged function' instead of falling back to native codegen".
…_Same_sequence". ALSO guard _Same_sequence with `IDL != 0`.
…tc. concepts as being true for 'bad' iterators". ALSO silence C4793 for test code.
…nges concepts in P0896R4_ranges_iterator_machinery".
…ression did not evaluate to a constant' when a lambda tests a constexpr variable defined outside".
|  Mode  |  Std  | EXE (bytes) | OBJ (bytes) | Peak working set (bytes) |
|:------:|:-----:|------------:|------------:|------------------------: |
| Native | C++17 |     594,944 |   9,113,398 |              948,101,120 |
| Native | C++20 |   2,481,664 |  33,528,885 |            3,033,104,384 |
| /clr   | C++17 |  12,517,888 |  53,675,003 |            1,111,109,632 |
| /clr   | C++20 |  41,550,336 | 198,202,365 |            3,107,594,240 |
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Nov 7, 2022
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner November 7, 2022 03:45
Copy link
Member

@zacklj89 zacklj89 left a comment

Choose a reason for hiding this comment

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

Nice! I looked at all the changes, and nothing jumped out at me, but it might be a good idea to get a review from someone who understands /clr:pure nuances better.

@StephanTLavavej StephanTLavavej merged commit ea09254 into microsoft:main Nov 8, 2022
@StephanTLavavej StephanTLavavej deleted the clr20 branch November 8, 2022 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
3 participants