Skip to content

[libc++] Only include <setjmp.h> from the C library if it exists #81887

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Feb 15, 2024

In 2cea1ba, we removed the <setjmp.h> header provided by libc++. However, we did not conditionally include the underlying <setjmp.h> header only if the C library provides one, which we otherwise do consistently (see e.g. 647ddc0).

rdar://122978778

In 2cea1ba, we removed the <setjmp.h> header provided by libc++.
However, we did not conditionally include the underlying <setjmp.h>
header only if the C library provides one, which we otherwise do
consistently (see e.g. 647ddc0).

rdar://122978778
@ldionne ldionne added this to the LLVM 18.X Release milestone Feb 15, 2024
@ldionne ldionne requested a review from a team as a code owner February 15, 2024 17:50
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Feb 15, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 15, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

In 2cea1ba, we removed the <setjmp.h> header provided by libc++. However, we did not conditionally include the underlying <setjmp.h> header only if the C library provides one, which we otherwise do consistently (see e.g. 647ddc0).

rdar://122978778


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

1 Files Affected:

  • (modified) libcxx/include/csetjmp (+7-1)
diff --git a/libcxx/include/csetjmp b/libcxx/include/csetjmp
index d219c8e6cb2250..9012cad22ebe74 100644
--- a/libcxx/include/csetjmp
+++ b/libcxx/include/csetjmp
@@ -33,7 +33,13 @@ void longjmp(jmp_buf env, int val);
 #include <__assert> // all public C++ headers provide the assertion handler
 #include <__config>
 
-#include <setjmp.h>
+// <setjmp.h> is not provided by libc++
+#if __has_include(<setjmp.h>)
+#  include <setjmp.h>
+#  ifdef _LIBCPP_SETJMP_H
+#    error "If libc++ starts defining <setjmp.h>, the __has_include check should move to libc++'s <setjmp.h>"
+#  endif
+#endif
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header

@ldionne
Copy link
Member Author

ldionne commented Feb 15, 2024

This broke some code downstream and we think this could have a negative impact on freestanding users (who are most likely not to have a setjmp.h header around), so I'm planning to backport this to LLVM 18.

Copy link
Member

@mordante mordante left a comment

Choose a reason for hiding this comment

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

LGTM!

@tstellar
Copy link
Collaborator

Does this need to be backported to the release branch?

@ldionne ldionne merged commit d8278b6 into llvm:main Feb 16, 2024
@ldionne ldionne deleted the review/fix-csetjmp branch February 16, 2024 21:45
@ldionne
Copy link
Member Author

ldionne commented Feb 16, 2024

Does this need to be backported to the release branch?

Yes, sorry I am going OOO and I forgot to merge this one. Merged from my phone just now.

/cherry-pick d8278b6

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 16, 2024
…m#81887)

In 2cea1ba, we removed the <setjmp.h> header provided by libc++. However, we did not conditionally include the underlying <setjmp.h>
header only if the C library provides one, which we otherwise do consistently (see e.g. 647ddc0).

rdar://122978778
(cherry picked from commit d8278b6)
@llvmbot
Copy link
Member

llvmbot commented Feb 16, 2024

/pull-request #82045

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Feb 23, 2024
…m#81887)

In 2cea1ba, we removed the <setjmp.h> header provided by libc++. However, we did not conditionally include the underlying <setjmp.h>
header only if the C library provides one, which we otherwise do consistently (see e.g. 647ddc0).

rdar://122978778
(cherry picked from commit d8278b6)
@pointhex pointhex mentioned this pull request May 7, 2024
# for free to join this conversation on GitHub. Already have an account? # 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.

5 participants