[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.

Sorry if some of my questions are naive, but I am not familiar with how things are layered when building on Windows. My understanding is that we're building on top of the C Standard Library part of the Visual C++ Runtime, and also using bits and pieces of their C++ STL (i.e. the `std::exception` class). Is that correct? What else do we use from their C++ support? Do we reuse other classes (e.g. more exception classes)?

> In this build configuration, one can still create instances of exception subclasses, and those objects will be ABI incompatible with the ones from when _HAS_EXCEPTIONS isn't defined to 0 - but that's IMO a pathological/self-imposed problem in that case.

To avoid this, would it be possible to provide an ABI-compatible `std::exception` class, and use that unconditionally when building on top of the VC runtime?



================
Comment at: libcxx/include/typeinfo:349
+
+#if !defined(_LIBCPP_ABI_VCRUNTIME_EXCEPTION)
+
----------------
Why are we not providing those when building on top of the vcruntime exceptions?


================
Comment at: libcxx/test/support/test_macros.h:255
 
-#if !TEST_HAS_FEATURE(cxx_exceptions) && !defined(__cpp_exceptions) \
-     && !defined(__EXCEPTIONS)
+#if (!TEST_HAS_FEATURE(cxx_exceptions) && !defined(__cpp_exceptions) \
+      && !defined(__EXCEPTIONS)) \
----------------
I don't understand why we're passing/checking `_HAS_EXCEPTIONS=0` around everywhere like that. Doesn't the compiler set `__has_feature(cxx_exceptions)` correctly when we use `-fno-exceptions` (or the MSVC equivalent)?


================
Comment at: libcxx/utils/ci/buildkite-pipeline.yml:377
 
+  - label: "Windows (_HAS_EXCEPTIONS=0)"
+    command: "bash libcxx/utils/ci/run-buildbot windows-noexceptions"
----------------



================
Comment at: libcxx/utils/ci/run-buildbot:562
+    # exceptions enabled.
+    EXTRA_TESTFLAGS="-D_HAS_EXCEPTIONS=0" generate-cmake-libcxx-win -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF
+    echo "+++ Running the libc++ tests"
----------------
Why don't we pass `-DLIBCXX_ENABLE_EXCEPTIONS=OFF` to the CMake instead?


================
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'),
----------------
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`?


================
Comment at: libcxx/utils/libcxx/test/params.py:40
+
+  # PR50676, affects MSVC mode without exceptions. This is only a temporary
+  # hack - ideally we'd at most add this flag in that particular configuration,
----------------
Hans said in https://bugs.llvm.org/show_bug.cgi?id=50676 that `-fno-delayed-template-parsing` fixed the issue in MSVC mode. Is there a reason why we're not compiling with that flag? Are we trying to make libc++ work while emulating the old and broken MSVC template parsing machinery (which has been fixed now IIUC)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103947