Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++] Only forward-declare ABI-functions in exception_ptr.h if they are meant to be used #84707

Merged

Conversation

itrofimow
Copy link
Contributor

@itrofimow itrofimow commented Mar 11, 2024

This patch fixes the unconditional forward-declarations of ABI-functions in exception_ptr.h, and makes it dependent on the availability macro, as it should've been from the beginning.
The declarations being unconditional break the build with libcxxrt before 045c52ce821388f4ae4d119fe4fb75f1eb547b85, now they are opt-out.

  • libcxx/include/__exception/exception_ptr.h
    Only forward-declare ABI-functions if they are meant to be used.

@itrofimow itrofimow requested a review from a team as a code owner March 11, 2024 02:12
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Mar 11, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 11, 2024

@llvm/pr-subscribers-libcxx

Author: None (itrofimow)

Changes

This patch fixes the unconditional fodward-declarations of ABI-functions in exception_ptr.h, and makes it dependendent on the availability macro, as it should've been from the beginning.
The declarations being unconditional break the build with libcxxrt before 045c52ce821388f4ae4d119fe4fb75f1eb547b85, now the are opt-out.

  • libcxx/include/__exception/exception_ptr.h
    Only forward-declare ABI-functions if they are meant to be used.

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

1 Files Affected:

  • (modified) libcxx/include/__exception/exception_ptr.h (+6-2)
diff --git a/libcxx/include/__exception/exception_ptr.h b/libcxx/include/__exception/exception_ptr.h
index 53e2f718bc1b35..c9027de9238cdd 100644
--- a/libcxx/include/__exception/exception_ptr.h
+++ b/libcxx/include/__exception/exception_ptr.h
@@ -26,6 +26,8 @@
 
 #ifndef _LIBCPP_ABI_MICROSOFT
 
+#  if _LIBCPP_AVAILABILITY_HAS_INIT_PRIMARY_EXCEPTION
+
 namespace __cxxabiv1 {
 
 extern "C" {
@@ -37,14 +39,16 @@ _LIBCPP_OVERRIDABLE_FUNC_VIS __cxa_exception* __cxa_init_primary_exception(
     void*,
     std::type_info*,
     void(
-#  if defined(_WIN32)
+#    if defined(_WIN32)
         __thiscall
-#  endif
+#    endif
             *)(void*)) throw();
 }
 
 } // namespace __cxxabiv1
 
+#  endif
+
 #endif
 
 namespace std { // purposefully not using versioning namespace

@itrofimow
Copy link
Contributor Author

itrofimow commented Mar 11, 2024

This

I hope having a way to remove the conflicting forward-declaration would suffice, but if this continues to cause problem I'd be ok with the thing being reverted from libcxx, and I'd shamefully pretend it never happened.

Could you @ldionne please have a look?

@itrofimow itrofimow force-pushed the exception_ptr_conditional_cxa_fwd_declaration branch from 8251e2b to 9daa1b3 Compare March 11, 2024 02:23
This patch fixes the unconditional fodward-declarations of ABI-functions in
exception_ptr.h, and makes it dependent on the availability macro, as it
should've been from the beginning.
The declarations being unconditional break the build with libcxxrt
before 045c52ce821388f4ae4d119fe4fb75f1eb547b85, now they are opt-out.

* libcxx/include/__exception/exception_ptr.h
  Only forward-declare ABI-functions if they are meant to be used.
@itrofimow itrofimow force-pushed the exception_ptr_conditional_cxa_fwd_declaration branch from 9daa1b3 to 7174c99 Compare March 11, 2024 02:59
@itrofimow
Copy link
Contributor Author

The build failed for generic-optimized-speed, libcxx-runners-8-set with

Unexpectedly Passed Tests (1):
  llvm-libc++-shared.cfg.in :: std/experimental/simd/simd.class/simd_ctor_conversion.pass.cpp

doesn't seem related to me.

@itrofimow itrofimow changed the title [libc++] NFC: only forward-declare ABI-functions in exception_ptr.h if the are meant to be used [libc++] NFC: only forward-declare ABI-functions in exception_ptr.h if they are meant to be used Mar 11, 2024
__thiscall
# endif
# endif
*)(void*)) throw();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing, no change requested: Should we replace these throw()'s with _NOEXCEPT? (perhaps in another PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do it in this PR, I can't see a way for a potential need to revert just throw() -> _NOEXCEPT.
Should I?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep the patch as-is just to separate concerns.

@ldionne ldionne changed the title [libc++] NFC: only forward-declare ABI-functions in exception_ptr.h if they are meant to be used [libc++] Only forward-declare ABI-functions in exception_ptr.h if they are meant to be used Mar 11, 2024
@ldionne ldionne added this to the LLVM 18.X Release milestone Mar 11, 2024
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM. The CI failure in simd is not your patch.

__thiscall
# endif
# endif
*)(void*)) throw();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep the patch as-is just to separate concerns.

@ldionne ldionne merged commit 63af858 into llvm:main Mar 11, 2024
50 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
Development

Successfully merging this pull request may close these issues.

4 participants