This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Support Microsoft ABI without vcruntime headers
ClosedPublic

Authored by smeenai on Oct 3 2017, 5:25 PM.

Details

Summary

The vcruntime headers are hairy and clash with both libc++ headers
themselves and other libraries. libc++ normally deals with the clashes
by deferring to the vcruntime headers and silencing its own definitions,
but for clients which don't want to depend on vcruntime headers, it's
desirable to support the opposite, i.e. have libc++ provide its own
definitions.

Diff Detail

Repository
rL LLVM

Event Timeline

smeenai created this revision.Oct 3 2017, 5:25 PM
smeenai updated this revision to Diff 117613.Oct 3 2017, 5:47 PM

Remove SAL annotations as well; they're unneeded and add a header dependency

compnerd edited edge metadata.Oct 3 2017, 5:49 PM

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

src/support/runtime/exception_msvc.ipp
31 ↗(On Diff #117607)

Really? clang-format removes the space there?

typedef void (__CRTDECL * terminate_handler)(void);

seems so much more readable.

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
src/support/runtime/exception_msvc.ipp
31 ↗(On Diff #117607)

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.

smeenai added inline comments.Oct 3 2017, 6:21 PM
src/support/runtime/exception_msvc.ipp
31 ↗(On Diff #117607)
smeenai accepted this revision.Oct 9 2017, 12:26 PM

I spoke to @EricWF on IRC last week and got his approval to commit this without review if no one had gotten to it in a couple of days, so I'm going ahead with that.

This revision is now accepted and ready to land.Oct 9 2017, 12:26 PM
This revision was automatically updated to reflect the committed changes.