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

Wed Jun 9 14:09:30 PDT 2021

mstorsjo added a comment.

In D103947#2808941 <https://reviews.llvm.org/D103947#2808941>, @ldionne wrote:

> It's the bare minimum for a configuration to be considered as supported. We already have a Windows bot, but I'm actually not sure whether it's using MinGW or straight Windows. @mstorsjo will know. Maybe it's trivial to add another Windows configuration to test this?

It would be fairly straightforward/near trivial to add a CI configuration with this setup - but it's more about how much CI resources it's ok to spend on this; when I set up the first libcxx windows CI runner some months ago, I was given the go-ahead for one config (and I ended up adding two) but not for a large matrix of configurations - and if the number of configs are limited, there might be more useful other configurations to test instead of this one (like MinGW based configs).

(Regarding testing this; to test the actual real world setup, one would build libc++ normally but define `_HAS_EXCEPTIONS=0` while using/testing it. I have no idea how much of our tests it would break if one would run the tests that way - I guess it could work mostly, if hooked up to `TEST_HAS_NO_EXCEPTIONS`.)

Overall, I'm not entirely sure if this in particular is a very useful configuration after all - the define was added as some sort of warning workaround in D57119 <https://reviews.llvm.org/D57119> (but I'm not seeing the exact full picture there), and apparently hasn't bit other actual users of libc++ on Windows so far.

FWIW, the current CI configs use MSVC toolchains. (Both MSVC and MinGW produce binaries that run on "straight Windows", while one is a first party official SDK and the other is a third party SDK.)

Testing MinGW based toolchains in CI would be great, but that requires a decent amount of changes to the CI environment, that I'm not sure I'm comfortable requesting - quite yet at least. I do run a nightly build of libc++ in the configurations I care about, but that doesn't give instant feedback though.

In D103947#2807992 <https://reviews.llvm.org/D103947#2807992>, @ldionne wrote:

> This looks good to me, except for the principle that we should not attempt to half-support configurations that we don't commit to testing - instead we should clearly state that those are not supported and simplify the library to not even try to support them. I'm 100% fine with this patch and supportive of this if I can get an answer to the questions above.

Overall regarding this, I totally agree that codepaths that aren't easily testable are problematic to maintain - but I wouldn't want to go quite as far as suggesting to rip out any configuration not set up in CI either. (Asynchronous private buildbots and similar, as many downstreams use, do test a number of more configurations, but you don't know exactly what ends up tested there though...)

That said I'm not arguing hard for or against this particular change (it's an odd configuration, but the fix seems fairly straightforward) - WDYT @rnk regarding whether this configuration makes sense and whether we should spend CI resources on it?

================
Comment at: libcxx/include/__config:225
+#  define _LIBCPP_ABI_VCRUNTIME
+#  if defined(_HAS_EXCEPTIONS) && _HAS_EXCEPTIONS == 0
+#    define _LIBCPP_NO_EXCEPTIONS
----------------
phosek wrote:
> Microsoft STL seems to be using `_HAS_EXCEPTIONS` [unconditionally](https://github.com/microsoft/STL/blob/62137922ab168f8e23ec1a95c946821e24bde230/stl/inc/exception#L30) presumably because vcruntime always defines it? I wonder if we should do the same to avoid accidentally going in the `else` branch if someone sets `_LIBCPP_ABI_VCRUNTIME` even when vcruntime isn't being used, I'd rather get an error in that case.
I think the distinction is that `__config` here can be included before `vcruntime.h` (which sets the default) gets included, so we can't (easily) check what it sets it to, only check if the user has indicated a specific desire e.g. on the command line.

Also, checking an undefined preprocessor macro is normally not a hard error in most setups (it's a warning at most) so that doesn't guard much against shooting oneself in the foot anyway I think.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103947