[libcxx] Fix build for glibc < 2.27 #121893
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PR llvm#109211 introduced a build break on systems with glibc < 2.27, since copy_file_range was only introduced after that version. A version check is added to prevent this breakage.
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Yi Kong (kongy) ChangesPR #109211 introduced a build break on systems with glibc < 2.27, since copy_file_range was only introduced after that version. A version check is added to prevent this breakage. Full diff: https://github.com/llvm/llvm-project/pull/121893.diff 1 Files Affected:
diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index bd37c5af86f6c3..e68f89ab1e6114 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -39,8 +39,8 @@
#include <fcntl.h> /* values for fchmodat */
#include <time.h>
-// since Linux 4.5 and FreeBSD 13, but the Linux libc wrapper is only provided by glibc and musl
-#if (defined(__linux__) && (defined(__GLIBC__) || _LIBCPP_HAS_MUSL_LIBC)) || defined(__FreeBSD__)
+// since Linux 4.5 and FreeBSD 13, but the Linux libc wrapper is only provided by glibc >= 2.27 and musl
+#if (defined(__linux__) && (_LIBCPP_GLIBC_PREREQ(2, 27) || _LIBCPP_HAS_MUSL_LIBC)) || defined(__FreeBSD__)
# define _LIBCPP_FILESYSTEM_USE_COPY_FILE_RANGE
#endif
#if __has_include(<sys/sendfile.h>)
|
philnik777
reviewed
Jan 7, 2025
|
For reference, RHEL 8 uses glibc 2.28, and Ubuntu 20.04 uses glibc 2.31 |
|
Android is still using glibc 2.17 on the host, but we're planning to move to musl. There are still some issues preventing us from doing so. We are not planning to upgrade glibc before the migration. Since this requires minimal effort to support (just a version macro check), I hope we can continue supporting older versions until it becomes genuinely difficult to maintain. |
We already only claim support for glibc 2.24 and later and the Android bots are just happy with the patch. Also, A libc from 2012 seems a bit unreasonable to support IMO. Having support for half a dozen different libc implementations is already a strain and we don't need even more of that by supporting versions that are more than a decade old. I'm also very sceptical of "just a version macro check". These accumulate, and while one of them isn't much of a problem, all of them together are a burden to support. |
If Android is using glibc 2.17, why don't our CI bots fail after applying #109211? CC @Jannik2099 |
from what I understand, Google is using glibc 2.17 on some build hosts, meaning the host is some GNU-esque userland that builds an Android environment. Unrelated to Android itself, which always uses Bionic, which is what CI tests. |
ldionne
changed the title
ldionne
changed the title
|
I'm a bit confused. @kongy Mentioned Glibc 2.27 in their original comment, but then 2.17 in a subsequent comment. The patch itself checks for 2.27. @kongy Can you confirm that you meant 2.27 in #121893 (comment)? Assuming that's correct, then I think we should actually support this since we do claim to support glibc >= 2.24, and 2.27 is certainly more recent than 2.24. We'd then add this and remove it once we bump the glibc requirement. |
|
FWIW, the original change also breaks building chromium against a debian bullseye sysroot (which you can get from here: https://commondatastorage.googleapis.com/chrome-linux-sysroot/dec7a3a0fc5b83b909cba1b6d119077e0429a138eadef6bf5a0f2e03b1904631). https://issues.chromium.org/issues/388064491 has some details. |
bullseye is glibc 2.31, and as already noticed in the link, the symbol is present in your libc. I think this is due to your linker scripts / wrapper and needs to be fixed on the chromium end? On that note a heads up, you'll possibly want to add |
To clarify, |
ldionne
approved these changes
Jan 7, 2025
There was a problem hiding this comment.
In that case, this patch LGTM since we support glibc 2.24 and this is truly required for glibc 2.24 through 2.26.
@philnik777 We can and should clean this up as soon as we drop support for older glibcs.
no, see AIX CI this'll require a bit of ugly |
ldionne
reviewed
Jan 7, 2025
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
ldionne
approved these changes
Jan 7, 2025
|
Can be merged once the CI is green. |
vitalybuka
reviewed
Jan 7, 2025
vitalybuka
approved these changes
Jan 7, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/7146 Here is the relevant piece of the build log for the reference |
|
Looks like this healed our bots too, so I suppose the problem was with android sysroots, not with linux sysroots. Sorry for the noise! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR #109211 introduced a build break on systems with glibc < 2.27, since copy_file_range was only introduced after that version. A version check is added to prevent this breakage.