[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 inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/exception:117
+
+class _LIBCPP_HIDE_FROM_ABI exception { // base of all library exceptions
 public:
----------------
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.


================
Comment at: libcxx/include/new:154
+#if defined(_LIBCPP_ABI_VCRUNTIME) && _HAS_EXCEPTIONS == 0
+class _LIBCPP_HIDE_FROM_ABI bad_alloc : public exception {
+public:
----------------
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?


================
Comment at: libcxx/include/typeinfo:377-379
+  static bad_cast __construct_from_string_literal(const char* const _Message) _NOEXCEPT {
+    return bad_cast(_Message);
+  }
----------------
What is this? I don't see it used anywhere, why is it necessary?


================
Comment at: libcxx/test/support/test.support/test_macros_header.no_exceptions.verify.cpp:12
 
 // REQUIRES: no-exceptions
 
----------------
If you didn't set the `no-exceptions` feature when `_HAS_EXCEPTIONS=0` (which is misleading IMO, see other comment), you wouldn't need to touch this test at all IIUC. You also would not need to introduce `TEST_COMPILING_WITH_NO_EXCEPTIONS`.


================
Comment at: libcxx/test/support/test_macros.h:184
+    (defined(_HAS_EXCEPTIONS) && _HAS_EXCEPTIONS == 0)
+#  define TEST_HAS_NO_EXCEPTIONS
+#endif
----------------
`TEST_HAS_NO_EXCEPTIONS` is used to represent `-fno-exceptions`. Let's not overload it to mean "either `-fno-exceptions` or no exception classes provided by VCRuntime". In fact, I don't understand why we even need to detect that no exception classes are provided by vcruntime, since we are defining them in their place. So from the test suite's perspective, my understanding is that we should not even need any special code to handle `_HAS_EXCEPTIONS=0`.


================
Comment at: libcxx/utils/ci/buildkite-pipeline.yml:568
 
+    - label: "Windows (_HAS_EXCEPTIONS=0)"
+      command: "bash libcxx/utils/ci/run-buildbot clang-cl-noexceptions"
----------------
mstorsjo wrote:
> Since earlier revisions of the patch, we now have both clang-cl and mingw CI configurations, so for clarity, please name this configuration `Clang-cl (_HAS_EXCEPTIONS=0)`, or as @ldionne requested earlier, `Clang-cl (no exceptions)`.
Actually, in light of the latest comments, I think this should either be `(no exception classes)`, `(no vcruntime exceptions)` or `(_HAS_EXCEPTIONS=0)`. `(no exceptions)` is misleading, since one would expect that it's basically `-fno-exceptions`. I think my vote would go for `(no vcruntime exceptions)`, and make the build job become `clang-cl-no-vcruntime-exceptions`.

Since this has apparently nothing to do with `-fno-exceptions`, let's make it clear.


================
Comment at: libcxx/utils/libcxx/test/features.py:34-40
+  # 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.
+  Feature(name='no-exceptions',                 when=lambda cfg: '_HAS_EXCEPTIONS' in compilerMacros(cfg) and compilerMacros(cfg)['_HAS_EXCEPTIONS'] == '0'),
----------------
mstorsjo wrote:
> ldionne wrote:
> > This feels like it shouldn't be necessary, but I'd like to understand.
> > 
> > Why would you run the test suite with `_HAS_EXCEPTIONS=0` without passing `enable_exceptions=False`?
> So this is the core issue to understand here: We don't intentionally build a setup of libc++ with exceptions disabled, we build a totally regular libc++ build. Then when this libc++ is used to build other projects, a small fraction of the translation units that include libc++ headers will be built with `_HAS_EXCEPTIONS=0`, which pulls out the rug under us, removing a bunch of the definitions we regularly rely on.
> 
> For testing purposes, we could build libc++ without exceptions support, but that wouldn't actually be testing the same scenario that we're going to hit in the wild. The same goes for unix platforms - you have libc++ built with exceptions enabled, but some amount of the code built using libc++ will be built with `-fno-exceptions`, and that should work even if the lib itself was built with exceptions enabled. Except `-fno-exceptions` has less brutal effect than `HAS_EXCEPTIONS=0`.
In that case, `no-exceptions` is not the right Lit feature name. It's already used for `-fno-exceptions` (i.e. no support for exceptions in the runtime), and this is something entirely different.

In fact, why do you even need to set a Lit feature here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103947