[libc++] Fix std::make_exception_ptr interaction with ObjC #135386

itrofimow · Apr 11, 2025

Clang treats throwing/catching ObjC types differently from C++ types, and omitting the throw in std::make_exception_ptr breaks ObjC invariants about how types are thrown/caught.

Fixes #135089

llvmbot · Apr 14, 2025

@llvm/pr-subscribers-libcxx

Author: None (itrofimow)

Changes

Clang treats throwing/catching ObjC types differently from C++ types, and omitting the throw in std::make_exception_ptr breaks ObjC invariants about how types are thrown/caught

Full diff: https://github.com/llvm/llvm-project/pull/135386.diff

1 Files Affected:

(modified) libcxx/include/__exception/exception_ptr.h (+2-1)

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));

jyknight · Apr 16, 2025

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()).

jyknight · May 12, 2025

Are you still planning to update this patch?

itrofimow · May 13, 2025

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

jyknight · May 13, 2025

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.

…n is a pointer

itrofimow · May 15, 2025

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

philnik777 · May 17, 2025

libcxx/include/__exception/exception_ptr.h

-  using _Ep2 = __decay_t<_Ep>;
-
-  void* __ex = __cxxabiv1::__cxa_allocate_exception(sizeof(_Ep));
+#    if _LIBCPP_AVAILABILITY_HAS_INIT_PRIMARY_EXCEPTION && __cplusplus >= 201703L


This should really be _LIBCPP_STD_VER >= 17. Also, why did this change from C++11 to C++17?

This is about language-level features rather than libc++ features: c++11 or higher was required for the lambda to compile; c++17 or higher is now required for if constexpr

Both are available as extensions in older versions. No need to put this behind a guard.

jyknight

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

libcxx/include/__exception/exception_ptr.h

rsesek · Jun 5, 2025

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.

itrofimow · Jun 10, 2025

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

ldionne · Jul 2, 2025

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

ldionne · Jul 2, 2025

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

ldionne · Jul 2, 2025

libcxx/include/__exception/exception_ptr.h

+  //
+  // [1]:
+  // https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Exceptions/Articles/Exceptions64Bit.html
+  if constexpr (is_pointer<_Ep>::value) {


Here, if there was a way to query Clang whether _Ep is an Objective-C pointer type, that would match exactly the condition they use in

llvm-project/clang/lib/CodeGen/CGException.cpp

Line 456 in 5b384c3

if (ThrowType->isObjCObjectPointerType()) {

to decide whether to generate an Objective-C throw or a normal C++ throw. Do you think that is something we should be able to query directly from Clang?

I had suggested std::is_pointer_v<_Ep> && std::is_convertible_v<_Ep, id> earlier if we want to avoid C++ pointers.

The problem is that id is not a known identifier in C++, so we have to guard this with an #ifdef checking whether we're compiling in Objective-C mode, and I think we had ODR issues with doing that?

Naively adding the check right now, I get:

exception_ptr.h:141:71: error: use of undeclared identifier 'id'

Right; it needs an ifdef. That shouldn't cause issues -- I think -- because the template arg cannot be an ObjC type and thus convertible to id if it's not an objc file. The reason not to do it was just itrofimow's feeling it was too complicated. It may well be, I don't have strong feelings here. So probably best to just leave it the way you have it now.

libcxx/include/__exception/exception_ptr.h

ldionne · Jul 4, 2025

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

itrofimow · Jul 4, 2025

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

itrofimow added 2 commits April 11, 2025 18:23

fix std::make_exception_ptr interaction with ObjC

9f88c67

add a comment about __OBJC__ define

764caba

itrofimow changed the title ~~fix std::make_exception_ptr interaction with ObjC~~ [libc++] fix std::make_exception_ptr interaction with ObjC Apr 14, 2025

itrofimow marked this pull request as ready for review April 14, 2025 19:04

itrofimow requested a review from a team as a code owner April 14, 2025 19:04

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 14, 2025

itrofimow added 2 commits May 14, 2025 01:11

remove the __OBJC__ usage in favor of checking whether the type throw…

6e108eb

…n is a pointer

Merge branch 'main' into objc_make_exception_ptr_fix

96c9c38

Merge branch 'main' into objc_make_exception_ptr_fix

966c3c7

philnik777 reviewed May 17, 2025

View reviewed changes

Merge branch 'main' into objc_make_exception_ptr_fix

bf5d0fa

itrofimow requested a review from philnik777 May 20, 2025 10:26

itrofimow added 5 commits May 20, 2025 14:23

relax the c++-standard requirement

a1e8586

Merge branch 'main' into objc_make_exception_ptr_fix

20083c2

Merge branch 'main' into objc_make_exception_ptr_fix

b27dc70

Merge branch 'main' into objc_make_exception_ptr_fix

25bdfd9

Merge branch 'main' into objc_make_exception_ptr_fix

c6c111d

jyknight reviewed Jun 4, 2025

View reviewed changes

libcxx/include/__exception/exception_ptr.h Outdated Show resolved Hide resolved

fix the closing #else comment typo

ec0735c

itrofimow and others added 3 commits June 24, 2025 00:19

Merge branch 'main' into objc_make_exception_ptr_fix

60073e3

Merge branch 'main' into objc_make_exception_ptr_fix

f3e83c1

Add test

985f7a6

ldionne added 3 commits July 1, 2025 14:29

Move test to proper location

e647f47

Use is_pointer instead of is_pointer_v

ca8f571

Refactor the function to be easier to understand

ddbbe97

ldionne changed the title ~~[libc++] fix std::make_exception_ptr interaction with ObjC~~ [libc++] Fix std::make_exception_ptr interaction with ObjC Jul 2, 2025

Fix broken friend

3808d88

ldionne reviewed Jul 2, 2025

View reviewed changes

jyknight reviewed Jul 2, 2025

View reviewed changes

libcxx/include/__exception/exception_ptr.h Show resolved Hide resolved

ldionne added 3 commits July 2, 2025 09:35

Fix C++03

f587c71

More C++03

860bec2

Fix issue with -fno-exceptions

455abac

jyknight approved these changes Jul 6, 2025

View reviewed changes

Search code, repositories, users, issues, pull requests...

[libc++] Fix std::make_exception_ptr interaction with ObjC #135386

Are you sure you want to change the base?

[libc++] Fix std::make_exception_ptr interaction with ObjC #135386

Conversation

itrofimow commented Apr 11, 2025 • edited by jyknight Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 14, 2025

Uh oh!

jyknight commented Apr 16, 2025

Uh oh!

jyknight commented May 12, 2025

Uh oh!

itrofimow commented May 13, 2025

Uh oh!

jyknight commented May 13, 2025

Uh oh!

itrofimow commented May 15, 2025

Uh oh!

philnik777 May 17, 2025

Choose a reason for hiding this comment

Uh oh!

itrofimow May 17, 2025

Choose a reason for hiding this comment

Uh oh!

philnik777 May 20, 2025

Choose a reason for hiding this comment

Uh oh!

jyknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rsesek commented Jun 5, 2025

Uh oh!

itrofimow commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldionne commented Jul 2, 2025

Uh oh!

ldionne commented Jul 2, 2025

Uh oh!

ldionne Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

jyknight Jul 2, 2025

Choose a reason for hiding this comment

Uh oh!

ldionne Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyknight Jul 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ldionne commented Jul 4, 2025

Uh oh!

itrofimow commented Jul 4, 2025

Uh oh!

Uh oh!

itrofimow commented Apr 11, 2025 •

edited by jyknight

Loading

itrofimow commented Jun 10, 2025 •

edited

Loading

ldionne Jul 3, 2025 •

edited

Loading