[libcxx] Only use copy_file_range on Linux or FreeBSD targets

[TartanLlama]

Prior to #169405, copy_file_range was used only on Linux targets with specific libc constraints, or any FreeBSD target. The PR simplified the ifdefs, resulting in the Linux requirement being dropped. This broke libc++ builds for wasm32-wasi targets. See LLVM issue #181543.

The WASI libc implementation is based on musl libc, we just don't have support for copy_file_range, so this is not declared in wasi-libc. Prior to the PR that changed the libc++ behaviour, we supplied HAS_MUSL_LIBC when building libc++. We have temporarily disabled this (WebAssembly/wasi-sdk#585), but it would be great if we could re-enable it to benefit from any other MUSL-targeted specializations.

This PR guards the GLIBC and musl libc checks against the __linux__ define, which will suffice to disable it for wasm32-wasi targets.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Sy Brand (TartanLlama)

Changes

Prior to #169405, copy_file_range was used only on Linux targets with specific libc constraints, or any FreeBSD target. The PR simplified the ifdefs, resulting in the Linux requirement being dropped. This broke libc++ builds for wasm32-wasi targets. See LLVM issue #181543.

The WASI libc implementation is based on musl libc, we just don't have support for copy_file_range, so this is not declared in wasi-libc. Prior to the PR that changed the libc++ behaviour, we supplied HAS_MUSL_LIBC when building libc++. We have temporarily disabled this (WebAssembly/wasi-sdk#585), but it would be great if we could re-enable it to benefit from any other MUSL-targeted specializations.

This PR guards the GLIBC and musl libc checks against the __linux__ define, which will suffice to disable it for wasm32-wasi targets.

---

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

1 Files Affected:

• (modified) libcxx/src/filesystem/operations.cpp (+1-1)

diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index 745db87ce3736..c1834a61b55de 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -41,7 +41,7 @@
 #include <time.h>
 
 // since Linux 4.5 and FreeBSD 13, but the Linux libc wrapper is only provided by glibc >= 2.27 and musl
-#if _LIBCPP_GLIBC_PREREQ(2, 27) || _LIBCPP_HAS_MUSL_LIBC || defined(__FreeBSD__)
+#if (defined(__linux__) && (_LIBCPP_GLIBC_PREREQ(2, 27) || _LIBCPP_HAS_MUSL_LIBC)) || defined(__FreeBSD__)
 #  define _LIBCPP_FILESYSTEM_USE_COPY_FILE_RANGE
 #endif
 

[stefson]

any chance this can still be merged into the 22.x branch?

musl was possibly refactored out of the list of if def., all this patch does is to revert back to the original logic.

[ldionne]

The underlying difficulty here is that we have no CI for wasi. As such, it's technically not a supported platform, and so this patch is technically introducing dead code. I assume you folks have a setup for testing libc++ on wasi? Would it be possible to add that to our pre-merge CI -- then that configuration would be properly supported.

[stefson]

@TartanLlama are you maybe able to help out with the proper informations for the last post? I'm not a dev myself, just your average linux user.

[alexcrichton]

I was cc'd here by @TartanLlama and I'm one of the maintainers of wasi-sdk which is one of the primary locations llvm and its tooling are all built for WASI targets. With respect to @ldionne your thoughts about CI -- personally I'm not familiar enough with LLVM and it's processes to know what it would entail to add WASI targets to CI. I'm also hesitant to try to decree that everyone else should care about the platform and ensure things work on it, so in that sense I'd at least personally be hesitant about adding it to LLVM's CI -- although this is definitely largely driven by my own ignorance of what this would mean and how to do it.

Personally though what we're currently doing I think is reasonable enough. We try to update wasi-sdk when an LLVM release branch is made and test out that everything works. There are occasional build failures and such which are not the hardest things in the world to track down, and to me it feels a reasonable "balance of power" that we're the ones tracking this down right now. The main drag on things at this point is that there are a number of PRs to libcxx and related projects which don't get any movement and we end up carrying a number of patches for in the wasi-sdk repository.

For example, of the current list of patches there's two PRs on LLVM that have been not seeing much activity:

• [libunwind] Exlcude wasm build from using __declspec export #168449
• [libc++abi] Define __cxa_thread_atexit on platforms that need it #186054

In updating to LLVM 23 I've also found locally that a minor issue was introduced in #194317 which we'll have to work around as well, which I'd ideally like to send a PR for to include upstream here but I'm hesitant to do so as I'm not confident that it will merge.

---

Putting all of this another way, I don't believe we currently have buy-in from preexisting LLVM maintainers to support WASI upstream officially in LLVM. Those of us working on wasi-sdk/WASI in general aren't preexisting LLVM maintainers so there's no know-how to do this. I realize that puts LLVM in a difficult position in the sense that there's desire to support a platform but no resources to add it to CI and such. We've got folks who are around and able to answer questions but that may not be sufficient for adding to CI (I'm not sure).

Does that help answer your question as to why this isn't currently on CI, and why it's probably not the easiest to add to CI? And/or do you have thoughts on how we could improve the situation here and/or engage better?

[SilensAngelusNex]

This issue is also breaking Libc++ builds for Trusty, since we also don't support copy_file_range. I've been guarding just the Musl check because that seems more with the spirit of 105900c, but both approaches are logically equivalent.

[nickdesaulniers]

(Trusty also uses musl as their C runtime). Is there libc++ premerge CI for musl support? Maybe it doesn't need to be wasi, and could just be musl on linux?

Edit: ah, right Trusty is not linux (and has no copy_file_range), which is why this patch works for them. @SilensAngelusNex 's patch: SilensAngelusNex@41e52a3.

Edit2: key takeaway, musl does not imply linux

[philnik777]

Generally we much prefer having CI than not having CI. This allows us to not break people and be confident in our changes. If anything platform-specific does break we expect that there is someone we can ask to help us, and that's basically it. Having back-and-forth patches to make a particular platform happy is much more hassle than simply running the CI and knowing that it works as expected.

I'd be very happy to have musl on linux as well as other platforms, since that is a configuration people clearly use, but we don't have any visibility in what we might break.

Adding CI is described at https://libcxx.llvm.org/AddingNewCIJobs.html#addingnewcijobs for buildkite, but you can also use GH Actions if that's easier for you.

Edit: If you'd like to talk about this there is also the libc++ monthly, which might get you faster results than just messages through GitHub.

[SilensAngelusNex]

What kind of CI check would you like to see that would catch issues like this?

I doubt you want a CI target that builds with Trusty's Musl libc; it'd probably end up breaking because of our changes. We could add CI for something like x86_64-unknown-none-musl, which would at least make this fallback not dead code, but if the buildkite agent is actually running Linux, its Musl would still provide copy_file_range and therefore have built despite this regression.

[philnik777]

What kind of CI check would you like to see that would catch issues like this?

I doubt you want a CI target that builds with Trusty's Musl libc; it'd probably end up breaking because of our changes.

If you use libc++ on Trusty (whatever that may be) I'd be very happy if there was a bot targeting Trusty. Looking at the link it may not be that different from running libc++ tests on Android, which we already do.

[ldionne]

As discussed in today's libc++ monthly, @SilensAngelusNex will work with @petrhosek to figure out how to bring up CI for Trusty. This can probably be done similarly to what's being done for the Android testers, which also run Qemu, etc. @pirama-arumuga-nainar , @Sharjeel-Khan and @nickdesaulniers are also listed as Android bot owners for libc++ and can probably provide additional support.

@TartanLlama We'd love to support WASI officially, but that requires pre-commit CI to be set up for it per our policy. Otherwise platform support gets completely out of hand. I see there are Docker images like ghcr.io/webassembly/wasi-sdk that contain the necessary setup to build WASI programs, perhaps it would be easy to add a pre-commit CI job that does that?

[ldionne]

In the meantime, requesting changes so it shows up properly in the queue.

[nickdesaulniers]

Checks like this are probably broken for bionic, too. I should probably should audit libc++ to see if there's other cases like this. cc @enh-google

[enh-google]

Checks like this are probably broken for bionic, too. I should probably should audit libc++ to see if there's other cases like this. cc @enh-google

android added the wrapper in api level 34 (see https://apilevels.com/ if that means nothing to you). so for us the missing disjunct would be || (defined(__ANDROID_API__) && __ANDROID_API__ >= 34).

(note that -- if anyone's thinking "how about we just check for __linux__ and use syscall(3) so we don't need to worry about the libc?" -- android has a default-disallow seccomp filter that means that you'll be killed with SIGSYS if you try it on too old an api level. there's a whitelist though for "stuff we'll probably use in future, when enough devices have new enough kernels", and it looks like copy_file_range(2) was in there from api level 26, so that's [up to] 5 extra years of support that way [though you'd need the ENOSYS fallback that i assume you already have].)

[enh-google]

Checks like this are probably broken for bionic, too. I should probably should audit libc++ to see if there's other cases like this. cc @enh-google

android added the wrapper in api level 34 (see https://apilevels.com/ if that means nothing to you). so for us the missing disjunct would be || (defined(__ANDROID_API__) && __ANDROID_API__ >= 34).

(note that -- if anyone's thinking "how about we just check for __linux__ and use syscall(3) so we don't need to worry about the libc?" -- android has a default-disallow seccomp filter that means that you'll be killed with SIGSYS if you try it on too old an api level. there's a whitelist though for "stuff we'll probably use in future, when enough devices have new enough kernels", and it looks like copy_file_range(2) was in there from api level 26, so that's [up to] 5 extra years of support that way [though you'd need the ENOSYS fallback that i assume you already have].)

(but to be clear: from the history it looks like this pr is "no functional change" for android anyway, so seems fine.)

[SilensAngelusNex]

Restating something I mentioned on Discord here so it's easier to find:

What does/should _LIBCPP_HAS_MUSL_LIBC actually mean? Both Trusty and WASI are setting LIBCXX_HAS_MUSL_LIBC because our libc implementations are Musl-derived. AFAICT the actual, upstream Musl library only supports Linux. Trusty (and I assume WASI too) doesn't support all the APIs that upstream Musl does, some because our version is old and some because we don't/can't support the underlying syscalls.

Looking at the uses of _LIBCPP_HAS_MUSL_LIBC, it seems like it could mean either "I am using the actual Musl libc" or "My libc's headers are in the same locations as Musl's." Trusty and WASI both mean "has Musl libc" in the second sense, but #169405 took it to mean the first. Should Trusty and WASI really be passing some other (new) option, like _LIBCPP_HAS_MUSL_DERIVED_LIBC? Or maybe we need to add || defined(__wasi__) || defined(__TRUSTY__) to the _LIBCPP_HAS_MUSL_LIBC checks that determine where to look for headers?

[stefson]

maybe I can write a few words of clarification here: this became an issue for everyone who, while on linux, uses musl as their libc.so while also wanting to have WASI-SDK available for wasm-sandboxing parts of firefox

the proposed fix simply restores the old libcxx-21.x behavior of the code, and it behaves just like before, which means the compile of the libcxx inside of wasi-sdk is fixed.

[ldionne]

Looking at the uses of _LIBCPP_HAS_MUSL_LIBC, it seems like it could mean either "I am using the actual Musl libc" or "My libc's headers are in the same locations as Musl's."

The TLDR here is that any Musl support in the library is vestigial of an earlier port that ended up rotting. We haven't tried ripping it out because I expect this would have caused outcry (and I think this thread confirms it would have), but that port is neither supported nor designed.

If we are to support Trusty and WASI in libc++ (I think we should, and it seems like there is demand for it), I would simply treat them as separate platforms and conditionalize on __wasi__, __TRUSTY__, etc. It's not clear to me there is even a need for a _LIBCPP_HAS_MUSL_LIBC, we probably want to conditionalize on platforms instead of C libraries.

If that turns out to be wrong and we actually want to detect Musl specifically, then we probably should have _LIBCPP_LIBC_MUSL, _LIBCPP_LIBC_<whatever-the-name-of-the-c-library-on-wasi>, etc. We can also do "Libc inheritance" like having a macro that is true for libcs derived from one, but honestly this usually ends up more confusing than dealing with mutually exclusive choices. We have precedent for libc detection:

llvm-project/libcxx/CMakeLists.txt

Lines 821 to 830 in 5f3b907

	# Set C library in use
	if (RUNTIMES_USE_LIBC STREQUAL "picolibc")
	config_define(1 _LIBCPP_LIBC_PICOLIBC)
	# picolibc is derived from newlib and behaves the same in regards to libc++
	# so setting both here:
	# * _LIBCPP_LIBC_NEWLIB is used now
	# * _LIBCPP_LIBC_PICOLIBC can be used for further customizations later
	config_define(1 _LIBCPP_LIBC_NEWLIB)
	elseif (RUNTIMES_USE_LIBC STREQUAL "newlib")
	config_define(1 _LIBCPP_LIBC_NEWLIB)

)

[SilensAngelusNex]

It's not clear to me there is even a need for a _LIBCPP_HAS_MUSL_LIBC, we probably want to conditionalize on platforms instead of C libraries.

Is there a reason there aren't individual knobs for these features? If we could set LIBCXX_FILESYSTEM_USE_COPY_FILE_RANGE = OFF, we wouldn't need special casing for our platform. Looks like the full list to replace everything that _LIBCPP_HAS_MUSL_LIBC currently controls would be:

• LIBCXX_FILESYSTEM_USE_COPY_FILE_RANGE (code)
• LIBCXX_HAS_MUSL_ALLTYPES (code)
• LIBCXX_HAS_MUSL_LOCALE (code)
• LIBCXX_ELAST (code, set to 4095 for existing LIBCXX_HAS_MUSL_LIBC users)
• LIBCXX_PROVIDES_DEFAULT_RUNE_TABLE (code)

I'd be happy to make that PR if you think it's a good approach; tests could run on the existing CI instead of needing a new platform.

[ldionne]

It's not clear to me there is even a need for a _LIBCPP_HAS_MUSL_LIBC, we probably want to conditionalize on platforms instead of C libraries.

Is there a reason there aren't individual knobs for these features? If we could set LIBCXX_FILESYSTEM_USE_COPY_FILE_RANGE = OFF, we wouldn't need special casing for our platform. Looks like the full list to replace everything that _LIBCPP_HAS_MUSL_LIBC currently controls would be:

That would make implementation details of libc++ part of our "public API" and make it easier for people to create frankenstein configurations. I don't think having such fine-grained knobs publicly would improve your situation in the long term. We'd end up breaking them by mistake or having to give up on providing any form of stability on those knobs, which doesn't help you.

For musl, following the path taken by WASI in #215384 seems to be by far the best way forward.