@llvm/pr-subscribers-libcxx

Author: None (itrofimow)

diff --git a/libcxx/include/__exception/exception_ptr.h b/libcxx/include/__exception/exception_ptr.h
index dac5b00b57fe3..167aa4d9b367a 100644
--- a/libcxx/include/__exception/exception_ptr.h
+++ b/libcxx/include/__exception/exception_ptr.h
@@ -92,7 +92,8 @@ class _LIBCPP_EXPORTED_FROM_ABI exception_ptr {
 template <class _Ep>
 _LIBCPP_HIDE_FROM_ABI exception_ptr make_exception_ptr(_Ep __e) _NOEXCEPT {
 #  if _LIBCPP_HAS_EXCEPTIONS
-#    if _LIBCPP_AVAILABILITY_HAS_INIT_PRIMARY_EXCEPTION && __cplusplus >= 201103L
+  // Clang treats throwing ObjC types differently, and we have to preserve original throw-ing behavior
+#    if !defined(__OBJC__) && _LIBCPP_AVAILABILITY_HAS_INIT_PRIMARY_EXCEPTION && __cplusplus >= 201103L
   using _Ep2 = __decay_t<_Ep>;
 
   void* __ex = __cxxabiv1::__cxa_allocate_exception(sizeof(_Ep));

It's not great to condition only on __OBJC__ preprocessor define, since that'll create a potential ODR issue for std::make_exception_ptr on a C++ type, when you're mixing a TU built with C++ and another TU built with ObjC++ in the same program (which, everyone is).

If you switch to the throw impl only for ObjC object-pointers, that stops being a problem, as an ObjC object pointer type cannot exist in a non-ObjC++ TU.

As a refinement to my suggestion on the issue, I think std::is_pointer_v<_Ep> && std::is_convertible_v<_Ep, id> would be better (so as to not accidentally trigger on a C++ value with a user-defined conversion operator id()).

Are you still planning to update this patch?

Hi! Yes, sorry, just got back to office

I was thinking, wouldn't just std::is_pointer_v<_Ep> condition be better it terms of readability?
After all, this is just an optimization, and given that people very seldom create exception pointers from pointers, it shouldn't be that much of a performance hit, and the less odd conditions we have here the better

You mean without the OBJC ifdef? I think that would be OK. But IMO it's not actually an improvement, because the condition is no longer clearly connected with the actual reason for the existence of the alternate codepath.

I don't think CI failure has to do with these changes, @jyknight could you please have a look?

Change looks fine.

But is there any way to add a test-case for this? (It's not clear to me there is since it depends on the non-llvm-project objc runtime, but it'd be great if there was.)

There's some prior art in func.blocks.arc.pass.mm for depending on the Blocks runtime in a test. Not sure if it's also okay to depend on the host libobjc similarly.

I'll see what I can do about the test for this.
I don't think it's actually necessary (but would be nice to have) to depend on objc runtime: maybe just a objc-codegen test is somewhat enough

@itrofimow @jyknight Thanks for the PR. I updated the PR with a test and I refactored the code as a drive-by so it would be easier to understand -- after adding yet another if in there, the function was getting pretty hard to follow. Can you please have a look at the PR? I'd like to get it merged whenever possible.

We might be able to make it into LLVM 20.1.9 if we end up having such a release.

Gentle ping @itrofimow @jyknight WDYT about the current PR? It's passing CI and I'd like to get it merged whenever possible!

Hi @ldionne! Thank you for taking care of this PR, I've got completely consumed by work and absolutely failed to clean up my own mess in time.

Personally, I don't think narrowing the "could it be ObjC type" condition any further via compiler intrinsics is worth it: for me (as someone who just reads libcxx source code from time to time, not a maintainer) the mere presence of ObjC-related code/comments in libcxx seems confusing enough, and I'd say that std::make_exception_ptr<T*> in C++ is a use-case too niche to further complicate code here, especially via something directly ObjC-related.

Nice refactoring by you, and I'm happy to see CI finally working.

LGTM

Thanks for the reviews. I'll merge this once CI is green.

CI is red for reasons unrelated to this patch, landing.

/cherry-pick fcc09b6

/pull-request #147554