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

[Petr Hosek via Phabricator via libcxx-commits]

phosek added inline comments.


================
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:
> > paulkirth wrote:
> > > 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.
> > > 
> > > 
> > > 
> > I understand. What you want to test is the combination of:
> > 
> > - libc++ built with support for exceptions
> > - user code built without support for exceptions
> > 
> > I'm OK with that -- I think that is useful and important to test. However, for me, that equates to the combination of `LIBCXX_ENABLE_EXCEPTIONS=ON` when building the library, and `--param enable_exceptions=False` when running the test suite. For me, the crazy part that we should not support is the fact that `_HAS_EXCEPTIONS=0` is not the same as `-fno-exceptions` -- there's no place for that distinction in the test suite, IMO. It **is** okay if users build some TUs with exceptions enabled and others disabled, as long as they use `-fno-exceptions` and `-fexceptions` to do so.
> > 
> > So I might be thick, but I still think we should be using `shared-clangcl.cfg.in` with `enable_exceptions=False`, and building the library with `LIBCXX_ENABLE_EXCEPTIONS=ON`. Please let me know if you think I'm misunderstanding something and this is somehow not reasonable. With my understanding, the only users we'd be "not supporting" with that path forward are those that set `_HAS_EXCEPTIONS=0` instead of passing `-fno-exceptions` in the TUs that shouldn't have exceptions, and I'm fine with that.
> I get that you want that - however the exact situation where this was encountered in the wild, which prompted this whole patch, is that existing user code does exactly that - build with `_HAS_EXCEPTIONS=0`, without `-fno-exceptions` (or the clang-cl equivalent).
> 
> Just building with `-fno-exceptions` alone does not trigger the whole thing that we try to fix here. Building with `-fno-exceptions` still keeps the vcruntime exception base classes visible in headers, but just disables exception handling codegen.
`/EH` and `_HAS_EXCEPTIONS` are technically orthogonal. `/EH` is the equivalent of `-f[no]-exceptions`. `_HAS_EXCEPTIONS` controls the use of exceptions in Microsoft STL including vcruntime which we use as a C++ ABI library on Windows. I don't know if there's a direct analogue on UNIX platforms, but on Windows we need to support both because they are being used in practice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103947