This is an archive of the discontinued LLVM Phabricator instance.

Split exception.cpp and new.cpp implementation into different files for different runtimes
ClosedPublic

Authored by EricWF on Jan 16 2017, 3:25 PM.

Details

Summary

exception.cpp is a bloody mess. It's full of confusing #ifdef branches for each different ABI library we support, and it's getting unmaintainable.

This patch breaks down exception.cpp into multiple different header files, roughly one per implementation. Additionally it moves the definitions of exceptions in new.cpp into the correct implementation header..

Diff Detail

Event Timeline

EricWF created this revision.Jan 16 2017, 3:25 PM
EricWF updated this revision to Diff 84613.Jan 16 2017, 4:47 PM
EricWF retitled this revision from Split exception.cpp implementation into different files for different runtimes to Split exception.cpp and new.cpp implementation into different files for different runtimes.
EricWF edited the summary of this revision. (Show Details)
  • Cleanup new.cpp as well.
  • Implement typeinfo on MSVC.
smeenai edited edge metadata.Jan 17 2017, 12:14 AM

I really like the cleanup, but I'm not the biggest fan of taking more Microsoft dependencies than necessary. I don't see any way around operator new/operator delete (since replacement in COFF can only work with a static library with one function per object file, the way msvcrt.lib does it, and it doesn't seem to be worth the trouble to provide our own corresponding static library), but I think the rest are avoidable (though it's entirely possible I'm missing something, of course).

include/exception
85

What's the rationale for relying on Microsoft's exception implementation rather than libc++'s?

include/new
96

new.h will pull in new unless you define certain macros. Is that desirable?

139

Again, why defer these to Microsoft's STL? In particular, set_new_handler and get_new_handler seem to be part of msvcprt, which means we would take a runtime dependency on Microsoft's C++ library, which doesn't seem great.

These functions should map pretty well to _query_new_handler and _set_new_handler (apart from the different function pointer signature, which can be thunked around), right?

178

Might be helpful to have a comment explaining why we wanna defer these to msvcrt on Windows?

Also, VS 2015 doesn't seem to have the sized and aligned allocation and deallocation functions. I haven't checked 2017.

EricWF added inline comments.Jan 17 2017, 1:49 AM
include/exception
85

vcruntime_new.h brings in vcruntime_exception.h which defines all of the exception symbols as inline. We have no choice but to cede to them.

include/new
96

That's not how I read the new.h header. In MSVC 2015 new.h pulls in vcruntime_new.h but it also declares std::new_handler and std::set_new_handler. <new> actually avoid declaring certain things if new.h has already been included.

std::get_new_handler is the only function declared in <new> that is not declared in <new.h>, however using this function also requires linking to the MSVC C++ STL which we can't do. It's not a great situation to be in, but I don't see how to avoid it.

139

We have to assume these declarations/definitions have already been included via a user including new.h, so we can't redefine them. std::set_new_handler seem to actually be a part of the CRT startup files, so we can't avoid using it (AFAIK).

These functions should map pretty well to _query_new_handler and _set_new_handler

Those functions take/return entirely different function types. So IDK how to turn the function pointer returned from _query_new_handler into an entirely different function type and return it from get_new_handler, at least not in a meaningful way.

178

You're right that VS 2015 doesn't have aligned new/delete. However until we can correctly implement get_new_handler we won't be able to correctly implement the additional aligned new/delete overloads.

EricWF added inline comments.Jan 17 2017, 1:51 AM
include/new
178

I failed to mention that I do plan to re-enable the aligned allocation overloads in the future; But before I do that I need a definition of std::get_new_handler which IDK how to provide yet. I've emailed the MSVC team asking for advice about implementing this.

EricWF updated this revision to Diff 84770.Jan 17 2017, 4:19 PM
  • Allow standalone libc++ build on OS X to default to using libc++abi.
smeenai added inline comments.Jan 17 2017, 10:16 PM
include/exception
85

Hmm, perhaps we're looking at different versions of the vcruntime headers? My _VC_CRT_BUILD_VERSION is 24210 (in crtversion.h).

vcruntime_new.h doesn't pull in vcruntime_exception.h for me:

% type headers.cpp
#include <vcruntime_new.h>

% cl /nologo /c /showIncludes headers.cpp
headers.cpp
Note: including file: C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\vcruntime_new.h
Note: including file:  C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\vcruntime.h
Note: including file:   C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\sal.h
Note: including file:    C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\ConcurrencySal.h
Note: including file:   C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\INCLUDE\vadefs.h

Additionally, I'm not sure what we need vcruntime_new.h for. As far as I can see, it just provides the operator new/delete overload prototypes, which we can provide ourselves.

include/new
96

My new.h header (from the 10.0.14393.0 SDK, though 10.0.10586.0 has the same line) contains:

#if !defined _MSC_EXTENSIONS && !defined _CRTBLD && !defined _CORECRT_BUILD
    #include <new>
#endif

_MSC_EXTENSIONS should be getting defined by default, which is why we're saved from that include, but I'd be explicit about avoiding that include.

I think it would be ideal to define _CORECRT_BUILD so that we avoid any extraneous definitions from new.h and just get _query_new_handler and _set_new_handler (which we can then use to implement std::get_new_handler and std::set_new_handler). Alternatively, we could just forward declare those two functions ourselves, since I think that's all we care about from that header.

139

Hmm, it seems annoying to have to be resilient to users including <new.h>. I guess it's technically part of the UCRT and not the MSVCPRT, but considering it can pull in <new> or define some standard C++ functions, it doesn't seem unreasonable to ask users to not mix and match <new.h> with libc++ headers.

I can think of a bunch of fun non-portable ways to do it, but the only portable way that comes to mind is stowing away the parameter to std::set_new_handler and then returning that in std::get_new_handler. That's what Microsoft's STL appears to be doing as well.

@EricWF and I discussed this on IRC a bunch yesterday.

We were inadvertently statically linking the Microsoft C++ libraries into tests, which is why std::set_new_handler was working with this patch. With that incorrect link removed, we get an undefined reference to std::set_new_handler, as expected. libc++ will have to implement std::get_new_handler and std:set_new_handler itself.

vcruntime_new.h is a gigantic pain, since it defines the operator new/delete overloads itself and is just generally annoying. We can avoid pulling it in ourselves, but user code might pull it in, and we probably want to be resilient to that? (I don't know how common the usage of that header is and if it's worth trying to be interoperable or just failing miserably in its presence.)

compnerd edited edge metadata.Jan 18 2017, 6:54 PM

While I love this direction (the original version really was an unintelligible pile of code), I really think that this change may be taking on too much. Why not split it up first and do nothing else. We could do the MS ABI implementation in a subsequent change. This would improve the code and would not be gated on the MS ABI changes.

While I love this direction (the original version really was an unintelligible pile of code), I really think that this change may be taking on too much. Why not split it up first and do nothing else. We could do the MS ABI implementation in a subsequent change. This would improve the code and would not be gated on the MS ABI changes.

I agree this review is taking on too much, it started out much smaller and I tried to avoid expanding it, but in the end I had three options:

A) Regress and remove all support for MSVC, this would break the windows build. (at least in exception.cpp and new.cpp).
B) Implement incorrect versions of support/runtime/<header>_msvc.ipp based on w/e we currently have, just to keep Windows building.
C) Implement correct versions of support/runtime/<header>_msvc.ipp.

I choose (C) since I didn't want to regress Windows, or spend time implementing incorrect <header>_msvc.ipp versions.
However I'm willing to try and shrink this down if you think that would be better.

While I love this direction (the original version really was an unintelligible pile of code), I really think that this change may be taking on too much. Why not split it up first and do nothing else. We could do the MS ABI implementation in a subsequent change. This would improve the code and would not be gated on the MS ABI changes.

I agree this review is taking on too much, it started out much smaller and I tried to avoid expanding it, but in the end I had three options:

A) Regress and remove all support for MSVC, this would break the windows build. (at least in exception.cpp and new.cpp).
B) Implement incorrect versions of support/runtime/<header>_msvc.ipp based on w/e we currently have, just to keep Windows building.
C) Implement correct versions of support/runtime/<header>_msvc.ipp.

I choose (C) since I didn't want to regress Windows, or spend time implementing incorrect <header>_msvc.ipp versions.
However I'm willing to try and shrink this down if you think that would be better.

That's dumb, and I'm dumb. I should be perfectly capable of breaking this down into smaller pieces

EricWF updated this revision to Diff 87944.Feb 9 2017, 8:10 PM

Merge with upstream.

EricWF accepted this revision.Feb 9 2017, 8:27 PM

Accepting before committing.

I wasn't able to split out the MSVC only changes in a fruitful way, so I'm going to commit this patch as-is.

This revision is now accepted and ready to land.Feb 9 2017, 8:27 PM
EricWF closed this revision.Feb 9 2017, 8:37 PM

Committed in r294707.

Still not a fan of the amount of vcruntime dependencies this is taking on, but I guess that can be followed up on.

Was the get_new_handler/set_new_handler issue resolved? (The issue being that those functions are in msvcprt, which our tests were silently linking against statically).

EricWF added a comment.Feb 9 2017, 9:26 PM

Still not a fan of the amount of vcruntime dependencies this is taking on, but I guess that can be followed up on.

Neither am I. However this gets the debug build working, and fixes RTTI, which in turn gets a ton of tests passing.
Now we have regression tests for when we start removing vcruntime dependencies.

Was the get_new_handler/set_new_handler issue resolved? (The issue being that those functions are in msvcprt, which our tests were silently linking against statically).

Not yet. I'll address that in another patch, and then also enable the aligned new/delete overloads.