[libcxx-commits] [PATCH] D103947: [libcxx] Fix using the vcruntime ABI with _HAS_EXCEPTIONS=0 defined

[Paul Kirth via Phabricator via libcxx-commits]

paulkirth marked an inline comment as done.
paulkirth added inline comments.


================
Comment at: libcxx/include/exception:117
+
+class _LIBCPP_HIDE_FROM_ABI exception { // base of all library exceptions
 public:
----------------
ldionne wrote:
> No classes should ever be marked with `_LIBCPP_HIDE_FROM_ABI`, it's meant for functions exclusively. Otherwise, we'll give improper visibility/linkage to the RTTI of the class, and you won't be able to catch it across shared library (and even TU) boundary.
> 
> You should mark the individual member functions you want to hide from the ABI instead. Sorry, that's not obvious at all from the macro name.
That makes so much more sense. Thanks for the tip. I think there is very little left to hide anymore, since a lot of the initial changes I made to try and be compatible w/ vcruntime are no longer necessary.


================
Comment at: libcxx/include/new:154
+#if defined(_LIBCPP_ABI_VCRUNTIME) && _HAS_EXCEPTIONS == 0
+class _LIBCPP_HIDE_FROM_ABI bad_alloc : public exception {
+public:
----------------
ldionne wrote:
> I am confused -- why do we have a different definition for those than above? In other words, why don't we simply change the `#if !defined(_LIBCPP_ABI_VCRUNTIME)` above to `#if !defined(_LIBCPP_ABI_VCRUNTIME) || (defined(_HAS_EXCEPTIONS) && _HAS_EXCEPTIONS == 0)`?
> 
> Are we trying to match the vcruntime layout here too?
I tried to reuse those definitions, and ran into some other issues related to the weirdness that _HAS_EXCEPTIONS == 0 presents.

Since the vcruntime provides the full defs for these classes, and libcxx is compiled w/o _HAS_EXCEPTIONS == 0, we need to provide the implementation somewhere because it won't be availible in the library. I don't know of a good way to fallback unless I provide an implementation in the header, but providing a separate implementation from the class def was more challenging that I thought due to the visibility annotations used for those classes.

My first attempt was to only provide the missing function implementations, which were just empty and were along the lines of:
```
#if !defined(_LIBCPP_ABI_VCRUNTIME) || _HAS_EXCEPTIONS == 0
//.. existing defs
#endif
#if defined(_LIBCPP_ABI_VCRUNTIME) || _HAS_EXCEPTIONS == 0
bad_alloc::bad_alloc() _NOEXCEPT {}
// more implementations ...
#endif
```

That seemed like a clean solution until the build complained about redefining the functions w/o `dllimport`, and I don't see a good way to provide out of line definitions for the classes w/in the header file. If there's an option I overlooked, I'm happy to go back to this approach, but didn't find a suitable method to make that work. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103947/new/

https://reviews.llvm.org/D103947