[libc++] Fix filesystem::remove_all() on FreeBSD

[markjdb]

remove_all_impl() opens the target path with O_NOFOLLOW, which fails if
the target is a symbolic link. On FreeBSD, rather than returning ELOOP,
openat() returns EMLINK. This is unlikely to change for compatibility
reasons, see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=214633 .

Thus, check for EMLINK as well.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Mark Johnston (markjdb)

Changes

See the commit message for an explanation.

I'm not sure if this bug is the reason for the XFAIL: LIBCXX-FREEBSD-FIXME annotation in test/std/input.output/filesystems/fs.op.funcs/fs.op.remove_all/remove_all.pass.cpp. @emaste do you have any idea?

---

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

1 Files Affected:

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

diff --git a/libcxx/src/filesystem/operations.cpp b/libcxx/src/filesystem/operations.cpp
index 6253d1551b062e..c39a3ee53db439 100644
--- a/libcxx/src/filesystem/operations.cpp
+++ b/libcxx/src/filesystem/operations.cpp
@@ -815,8 +815,10 @@ uintmax_t remove_all_impl(int parent_directory, const path& p, error_code& ec) {
 
   // If opening `p` failed because it wasn't a directory, remove it as
   // a normal file instead. Note that `openat()` can return either ENOTDIR
-  // or ELOOP depending on the exact reason of the failure.
-  if (ec == errc::not_a_directory || ec == errc::too_many_symbolic_link_levels) {
+  // or ELOOP depending on the exact reason of the failure. On FreeBSD it
+  // may return EMLINK instead of ELOOP, contradicting POSIX.
+  if (ec == errc::not_a_directory || ec == errc::too_many_symbolic_link_levels ||
+      ec == errc::too_many_links) {
     ec.clear();
     if (::unlinkat(parent_directory, p.c_str(), /* flags = */ 0) == -1) {
       ec = detail::capture_errno();

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[emaste]

The XFAIL was added in 7ec6c62 and just captured the current state of the tests in order to stand up the FreeBSD CI instance. It is presumably because of this issue, and assuming the test passes with this change the XFAIL should be removed.

[markjdb]

The XFAIL was added in 7ec6c62 and just captured the current state of the tests in order to stand up the FreeBSD CI instance. It is presumably because of this issue, and assuming the test passes with this change the XFAIL should be removed.

Do you happen to have a simple recipe to build and run these tests on FreeBSD? I haven't done that before.

[emaste]

Do you happen to have a simple recipe to build and run these tests on FreeBSD? I haven't done that before.

I haven't built upstream llvm in a while, but you should just be able to run e.g. bin/llvm-lit ../libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.remove_all

It looks a run with this change should have results here in a bit though:
https://buildkite.com/llvm-project/libcxx-ci/builds/33234#018d43a2-514b-40fd-86c4-6e743f60fe21

[ldionne]

This looks reasonable to me but you'll need to remove the XFAIL for the CI to pass. Please also ensure that all the other platforms are green before merging.

[markjdb]

This looks reasonable to me but you'll need to remove the XFAIL for the CI to pass. Please also ensure that all the other platforms are green before merging.

Thank you. I pushed a second commit which does that. I verified that the test now passes on FreeBSD.

[DimitryAndric]

LGTM

[markjdb]

It looks like one of the test runs was cancelled. Should I try to rerun them?

[DimitryAndric]

All the buildkite stuff at https://buildkite.com/llvm-project/libcxx-ci/builds/33247#_ says "cancelled after 5h" so I think it's probably some VM or container failing to start. No idea what is behind all this infrastructure. :))

[DimitryAndric]

Not sure about the CI failure, it just seems like a timeout for a VM to start up. Nothing to do with your commit, as usual.

[jwakely]

FWIW while handling EMLINK in libstdc++ I discovered that NetBSD sets its non-standard EFTYPE error for the "openat with O_NOFOLLOW meets a symlink" case.

DragonFly matches FreeBSD, so the current libc++ code should handle that already.

NetBSD 10.0 openat(2)

DragonFly 6.4 openat(2)