[PATCH] D38522: [libc++] Support Microsoft ABI without vcruntime headers

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In https://reviews.llvm.org/D38522#887768, @compnerd wrote:

> Why do we expect the tests to fail without vcruntime headers?


msvcrt's implementations of the various new/delete operators call `__scrt_throw_std_bad_alloc` and `__scrt_throw_std_bad_array_new_length`, which are found in `throw_bad_alloc.obj` inside `msvcrt.lib`. That object file also contains the definitions of various exception methods (e.g. `std::exception::~exception`) which libc++ will also define for `_LIBCPP_NO_VCRUNTIME`, leading to symbol conflicts between libc++ and msvcrt. Consequently, `_LIBCPP_NO_VCRUNTIME` must also use libc++'s definitions of the new/delete operators rather than msvcrt's.

The failing tests check indirect replacement, e.g. replacing `operator new`, calling `operator new[]`, and expecting the replaced `operator new` to be called. That would have worked with msvcrt's new/delete operators, since they're statically linked into the executable and therefore their `operator new[]` can call the replaced `operator new`, but it won't work with libc++'s new/delete operators, since libc++'s `operator new[]` is bound to its own `operator new`. (It would work as expected if the user replaced `operator new[]` in addition to `operator new`.) That's unfortunate, but it's also a pretty rare scenario.

On the plus side, the following tests only pass with `_LIBCPP_NO_VCRUNTIME`. I believe the change in `src/new.cpp` to include `support/runtime/new_handler_fallback.ipp` should actually not be limited to `_LIBCPP_NO_VCRUNTIME`; I'll investigate that in a follow-up.

  std/language.support/support.dynamic/alloc.errors/set.new.handler/get_new_handler.pass.cpp
  std/language.support/support.dynamic/alloc.errors/set.new.handler/set_new_handler.pass.cpp
  std/language.support/support.dynamic/new.delete/new.delete.array/new_array.pass.cpp
  std/language.support/support.dynamic/new.delete/new.delete.array/new_array_nothrow.pass.cpp
  std/language.support/support.dynamic/new.delete/new.delete.single/new.pass.cpp
  std/language.support/support.dynamic/new.delete/new.delete.single/new_nothrow.pass.cpp



================
Comment at: src/support/runtime/exception_msvc.ipp:31
+extern "C" {
+typedef void(__CRTDECL* terminate_handler)();
+_ACRTIMP terminate_handler __cdecl set_terminate(
----------------
compnerd wrote:
> Really?  clang-format removes the space there?
> 
>     typedef void (__CRTDECL * terminate_handler)(void);
> 
> seems so much more readable.
IIRC it's an issue with having a macro in a function pointer (`__CRTDECL`) in this case. I'll adjust the spacing manually and file a clang-format bug.


https://reviews.llvm.org/D38522