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

[Paul Kirth via Phabricator via libcxx-commits]

paulkirth added inline comments.


================
Comment at: libcxx/include/exception:141
+protected:
+    const char* _Ptr; 
+};
----------------
phosek wrote:
> I wonder if we should annotate this with `[[maybe_unused]]` to avoid a compiler warning.
That may be a good idea. Grep didn't reveal a macro that I saw, but I may have missed it. I do see that this file has a `_LIBCPP_DIAGNOISTIC_IGNORED(-Wunused-private-field)` maybe that is the correct approach here w/ a change to the warning?




================
Comment at: libcxx/include/exception:89
+// which we use in order to be ABI-compatible with other STLs on Windows.
+#if defined(_LIBCPP_ABI_VCRUNTIME) && _HAS_EXCEPTIONS == 1
 #include <vcruntime_exception.h>
----------------
mstorsjo wrote:
> Is this condition strictly necessary? Isn't it the case that if we include the header, under these circumstances, it doesn't provide the definitions we need? I.e. we could still keep including the header even if it doesn't give us anything of value.
> 
> (OTOH I guess the condition can be good for clarity?)
> 
> Are we sure that `vcruntime.h` which sets the default `_HAS_EXCEPTIONS == 1` has been included at this point? (I guess `cstddef` and `cstdlib` boil down to including that transitively?) Or would we need to make this into `#if defined(_LIBCPP_ABI_VCRUNTIME) && (!defined(_HAS_EXCEPTIONS) || _HAS_EXCEPTIONS != 0)`?
I was trying to be explicit, but maybe my logic was off here. I believe that `vcruntime.h` should always have been pulled in by this point, but I will try and double check that will always be the case. Regardless,  I think your suggestion of `#if defined(_LIBCPP_ABI_VCRUNTIME) && (!defined(_HAS_EXCEPTIONS) || _HAS_EXCEPTIONS != 0)` is the better approach to take here






================
Comment at: libcxx/include/exception:104
+// However, <vcruntime_exception.h> does not define std::exception and std::bad_exception 
+// under -fno-exceptions.
+//
----------------
mstorsjo wrote:
> Isn't the `_HAS_EXCEPTIONS` define strictly speaking independent of `-fno-exceptions`? As far as I can see, clang-cl doesn't predefine `_HAS_EXCEPTIONS`, and `vcruntime.h` defines `_HAS_EXCEPTIONS` to 1 unless it has been set by the user/caller.
> 
> I.e. I think the comment would be more correct by speaking of `_HAS_EXCEPTIONS` consistently instead of involving `-fno-exceptions`.
> 
> Secondly, I see that the structure of comments for the ifdef conditions here, is to have the comment preceding the condition. As there's more than one condition involved, the comment for the second condition `#elif defined(_LIBCPP_ABI_VCRUNTIME) && _HAS_EXCEPTIONS == 0` ends up in the ifdef block for the first condition, so at least to me, it'd be more readable if the comment explaining the condition would be after the condition, i.e. within the ifdef block it explains.
> Isn't the _HAS_EXCEPTIONS define strictly speaking independent of -fno-exceptions? As far as I can see, clang-cl doesn't predefine _HAS_EXCEPTIONS, and vcruntime.h defines _HAS_EXCEPTIONS to 1 unless it has been set by the user/caller.

My understanding here was that this was a problem because when `-fno-exceptions` is set, then MSVC sets `_HAS_EXCEPTIONS=0`, and thus does not provide the definitions for the Exception classes. But if it adds clarity, I'm happy to amend the wording here to focus on `_HAS_EXCEPTIONS`

Additionally, I'm happy to place the comments differently if you think it would improve readability, so thanks for the suggestion.


================
Comment at: libcxx/include/exception:129
+
+    _NODISCARD virtual const char* __CLR_OR_THIS_CALL what() const _NOEXCEPT { // return pointer to message string
+        return "unknown exception";
----------------
mstorsjo wrote:
> Should this maybe be using `_LIBCPP_NODISCARD` instead? And `__CLR_OR_THIS_CALL` could maybe be omitted in all cases where libc++ is used... I guess it's a tradeoff between exactly matching the reference to the letter (easing compat if taking this to further untested configurations) vs making the code match libc++ conventions (where attribute macros may be defined slightly differently than in upstream vcruntime).
Oh, thanks for catching the error with `_NODISCARD`, I will update the patch to reflect this.

w.r.t `__CLR_OR_THIS_CALL` I erred on the side of caution, since I wasn't 100% sure it would be ABI compatible w/o it, but I was also hesitant about using it. 

Given that you also think its a bit out of place, I plan to remove them. If we find out it does indeed cause a problem, we can always add them back in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103947