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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I'm mostly happy with this, except for the fact that we seem to allow `_HAS_EXCEPTIONS=0` when the compiler is still passed `-fexceptions`. That seems either wrong or not worth supporting to me, but I'm all ears.



================
Comment at: libcxx/include/exception:118-134
+public:
+  exception() _NOEXCEPT : _Data() {}
+
+  explicit exception(char const* _Message) _NOEXCEPT : _Data() {
+    _Data._What = _Message;
+    _Data._DoFree = true;
+  }
----------------
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).


================
Comment at: libcxx/include/typeinfo:399
+
+class __non_rtti_object : public bad_typeid {
+public:
----------------
What is this class?


================
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.
----------------
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`?


================
Comment at: libcxx/utils/libcxx/test/features.py:42-47
+  # Normally, the feature 'no-exceptions' is set in params.py if
+  # running tests with enable_exceptions=false, but if we didn't set
+  # that and only defined _HAS_EXCEPTIONS=0, pick up on that and set the
+  # no-exceptions feature so certain tests are omitted.
+  # It would probably be fine to just run that test configuration with
+  # enable_exceptions=false too.
----------------
I think this is the cause of most of my confusion. Why would someone ever define `_HAS_EXCEPTIONS=0` manually instead of using `-fno-exceptions`? That seems like a small quirk that causes a lot of complexity in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103947