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

[Paul Kirth via Phabricator via libcxx-commits]

paulkirth added a comment.

Thanks for the feedback, and glad most of it seems to be in order.



================
Comment at: libcxx/include/exception:118-134
+public:
+  exception() _NOEXCEPT : _Data() {}
+
+  explicit exception(char const* _Message) _NOEXCEPT : _Data() {
+    _Data._What = _Message;
+    _Data._DoFree = true;
+  }
----------------
ldionne wrote:
> I think we want to mark all of those as `_LIBCPP_HIDE_FROM_ABI` (and similar for other exception classes defined in this patch).
> 
> And then, we can get rid of the weird `exception(char const* _Message, int)` constructor in favour of something more elegant, like passing a boolean directly or using a named tag like `__dont_free_tag{}` (because we don't care about matching the exact ABI of the constructor defined by VCRuntime, which I assume is why you're using the funky `int` overload here).
Adding the `_LIBCPP_HIDE_FROM_ABI` is a good suggestion. Thank you.

Re removing the odd constructor, I think I had some trouble getting that to work IIRC, but let me give it another shot. 


================
Comment at: libcxx/include/typeinfo:399
+
+class __non_rtti_object : public bad_typeid {
+public:
----------------
ldionne wrote:
> What is this class?
oh, you're right. I think this is unnecessary. 


================
Comment at: libcxx/test/configs/llvm-libc++-shared-no-exception-clangcl.cfg.in:1
+# This testing configuration handles running the test suite against LLVM's libc++
+# using a DLL, with Clang-cl on Windows.
----------------
mstorsjo wrote:
> ldionne wrote:
> > Why is this not handled by passing `-fno-exceptions` on the command-line when running the test suite? In other words, why don't we use the existing `shared-clangcl.cfg.in` configuration and add `enable_exceptions=False`?
> I think the main point is that this isn’t a configuration that someone intentionally chooses, as a project wide configuration. For various historic reasons, some projects seem to be building smaller subsets of their code (e.g. a subset of their source files) with this define (`_HAS_EXCEPTIONS=0`) set. They don’t build with `-fno-exceptions` (or the MSVC/clang-cl equivalents). I.e. the end user code is built with exception handling codegen enabled, but vcruntime’s classes hidden.
> 
> (Also, building with `-fno-exceptions` doesn’t define `_HAS_EXCEPTIONS=0`. They’re totally independent toggles.)
> 
> If building with `LIBCXX_ENABLE_EXCEPTIONS=OFF`, you’d have the libc++ built with vcruntime exception classes hidden - but that’s not the situation that end users use. Testing that way would probably cover many aspects of the code, but it wouldn’t test what end users actually end up using.
> 
> End users have a libc++ built entirely normally, but some fraction of end users code is built with vcruntime exception classes hidden.
> 
> (The fact that the library is built differently than end user code is the same situation as `-fexception` vs `-fno-exceptions` on unix. The library will be built with exceptions enabled, while some end user object files may be built with `-fno-exceptions`.)
Here we're testing the case when the user hasn't passed `-fno-exceptions` but the has disabled exceptions through `_HAS_EXCEPTIONS=0`. From reading Martin's comments about this in earlier revisions it  seems to be an important case to handle on Windows.

I'm happy to include the other approach, but I'm not sure we should forgo this configuration, unless libc++ wants to document it as unsupported.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103947