[libc++] Avoid including <features.h> on arbitrary platforms #125587
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesThis reverts commit 5f2389d. That commit started checking whether <features.h> was a valid include, however codebases are free to have such a header on their search path, which breaks compilation. LLVM libc should instead provide a more standard way of getting configuration macros like LLVM_LIBC. Full diff: https://github.com/llvm/llvm-project/pull/125587.diff 2 Files Affected:
diff --git a/libcxx/include/__configuration/platform.h b/libcxx/include/__configuration/platform.h
index 2a92ce209b91f8b..27f68d04e8a8d9c 100644
--- a/libcxx/include/__configuration/platform.h
+++ b/libcxx/include/__configuration/platform.h
@@ -30,18 +30,15 @@
// ... add new file formats here ...
#endif
-// To detect which libc we're using
-#if __has_include(<features.h>)
-# include <features.h>
-#endif
-
+// Need to detect which libc we're using if we're on Linux.
#if defined(__linux__)
+# include <features.h>
# if defined(__GLIBC_PREREQ)
# define _LIBCPP_GLIBC_PREREQ(a, b) __GLIBC_PREREQ(a, b)
# else
# define _LIBCPP_GLIBC_PREREQ(a, b) 0
# endif // defined(__GLIBC_PREREQ)
-#endif
+#endif // defined(__linux__)
#ifndef __BYTE_ORDER__
# error \
diff --git a/libcxx/include/__random/binomial_distribution.h b/libcxx/include/__random/binomial_distribution.h
index 9538c15e2dc97b5..47790b674db5882 100644
--- a/libcxx/include/__random/binomial_distribution.h
+++ b/libcxx/include/__random/binomial_distribution.h
@@ -97,8 +97,7 @@ class _LIBCPP_TEMPLATE_VIS binomial_distribution {
}
};
-// The LLVM C library provides this with conflicting `noexcept` attributes.
-#if !defined(_LIBCPP_MSVCRT_LIKE) && !defined(__LLVM_LIBC__)
+#ifndef _LIBCPP_MSVCRT_LIKE
extern "C" double lgamma_r(double, int*);
#endif
|
|
Would it make sense to partially revert? IIUC this still works when targeting linux with the LLVM libc, so we could just revert the inclusion of |
|
Might need to check for |
|
My strong preference would be to include a standard header instead, but that would require LLVM libc to define its detection macro when such header is included. I'd be okay with a partial revert only, but @jhuber6 would have to tell me what the check should be instead. |
I think we already have a handful of Though that might break during offloading usage on Windows if the user has
But |
This partially reverts commit 5f2389d. That commit started checking whether <features.h> was a valid include unconditionally, however codebases are free to have such a header on their search path, which breaks compilation. LLVM libc should instead provide a more standard way of getting configuration macros like __LLVM_LIBC__. After this patch, we only include <features.h> when we're on Linux or when we're compiling for GPUs.
ldionne
changed the title
|
This is going to break other platforms supported by LLVM libc, most notably baremetal (for which we don't really have a great detection mechanism since there's no OS). An alternative to using an LLVM libc provided header would be to store the value of |
At first glance, I'm really not a huge fan of hardcoding the libc we're building on top of in I'm willing to work on that, but it's not something we'd be able to cherry-pick back to LLVM 20 given its complexity and the time it'll take to put something together. In the meantime, I think it's pretty urgent to restore to a behavior that works on mainstream platforms. If you have specific concerns about this patch breaking some new setups since the last release, perhaps we can carve them out using additional |
|
I'm personally not concerned with the GPU stuff being broken on Clang-20 since the support is still experimental and I doubt anyone's depending on it. |
|
@petrhosek Gentle ping. If you have specific concerns about landing this partial revert, please let me know ASAP so we can work on addressing them. Landing this patch addresses a regression that was introduced in LLVM 20 so I'd like to do this sooner rather than later. |
I believe this change should be fine, but I'll test it locally against our embedded projects just to be sure that nothing breaks. |
I tested this locally and it looks like this change breaks the libc++ on baremetal. Specifically this condition no longer evaluates as true: llvm-project/libcxx/src/chrono.cpp Lines 34 to 36 in d7fd2a2 We'll need some alternative. We could move the |
|
Now that #126877 landed, this should be safe to land. |
|
/cherry-pick cffc1ac |
|
/pull-request #127310 |
|
Looks like this breaks cuda tests on Mac, at least in the gn build: http://45.33.8.238/macm1/100921/step_6.txt I think the test isn't sufficiently hermetic and the test is to blame, but nevertheless check-clang is currently broken due to this PR. |
Ack, just saw this. I think it's easy to add another layer of guard with Edit: #127691 |
…5587) This partially reverts commit 5f2389d. That commit started checking whether <features.h> was a valid include unconditionally, however codebases are free to have such a header on their search path, which breaks compilation. LLVM libc now provides a more standard way of getting configuration macros like __LLVM_LIBC__. After this patch, we only include <features.h> when we're on Linux or when we're compiling for GPUs. (cherry picked from commit cffc1ac)
…5587) This partially reverts commit 5f2389d. That commit started checking whether <features.h> was a valid include unconditionally, however codebases are free to have such a header on their search path, which breaks compilation. LLVM libc now provides a more standard way of getting configuration macros like __LLVM_LIBC__. After this patch, we only include <features.h> when we're on Linux or when we're compiling for GPUs.
This partially reverts commit 5f2389d. That commit started checking whether
<features.h> was a valid include unconditionally, however codebases are free
to have such a header on their search path, which breaks compilation. LLVM
libc should instead provide a more standard way of getting configuration
macros like LLVM_LIBC.
After this patch, we only include <features.h> when we're on Linux or
when we're compiling for GPUs.