Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[libc][c11] Add stdio.h's rename() function #85068

Merged
merged 8 commits into from
Mar 21, 2024
Merged

Conversation

aniplcc
Copy link
Contributor

@aniplcc aniplcc commented Mar 13, 2024

Adds stdio.h's rename() function as defined in n3096. Fixes #84980.

@llvmbot llvmbot added the libc label Mar 13, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 13, 2024

@llvm/pr-subscribers-libc

Author: aniplcc (aniplcc)

Changes

Adds stdio.h's rename() function as defined in n3096. Fixes #84980.


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

11 Files Affected:

  • (modified) libc/config/linux/aarch64/entrypoints.txt (+1)
  • (modified) libc/config/linux/riscv/entrypoints.txt (+1)
  • (modified) libc/config/linux/x86_64/entrypoints.txt (+1)
  • (modified) libc/spec/stdc.td (+5)
  • (modified) libc/src/stdio/CMakeLists.txt (+7)
  • (modified) libc/src/stdio/linux/CMakeLists.txt (+14)
  • (added) libc/src/stdio/linux/rename.cpp (+38)
  • (added) libc/src/stdio/rename.h (+21)
  • (modified) libc/test/src/stdio/CMakeLists.txt (+17)
  • (added) libc/test/src/stdio/rename_test.cpp (+50)
  • (modified) utils/bazel/llvm-project-overlay/libc/BUILD.bazel (+11)
diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index abd1f83794ed0b..5a4c9b850d46b2 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -165,6 +165,7 @@ set(TARGET_LIBC_ENTRYPOINTS
 
     # stdio.h entrypoints
     libc.src.stdio.remove
+    libc.src.stdio.rename
     libc.src.stdio.sprintf
     libc.src.stdio.snprintf
     libc.src.stdio.vsprintf
diff --git a/libc/config/linux/riscv/entrypoints.txt b/libc/config/linux/riscv/entrypoints.txt
index 006aa787ea6afd..35e8e01ab51b8c 100644
--- a/libc/config/linux/riscv/entrypoints.txt
+++ b/libc/config/linux/riscv/entrypoints.txt
@@ -166,6 +166,7 @@ set(TARGET_LIBC_ENTRYPOINTS
 
     # stdio.h entrypoints
     libc.src.stdio.remove
+    libc.src.stdio.rename
     libc.src.stdio.sprintf
     libc.src.stdio.snprintf
     libc.src.stdio.fprintf
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 4fb31c593b9dc7..9e647842e1560a 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -197,6 +197,7 @@ set(TARGET_LIBC_ENTRYPOINTS
 
     # stdio.h entrypoints
     libc.src.stdio.remove
+    libc.src.stdio.rename
     libc.src.stdio.sprintf
     libc.src.stdio.snprintf
     libc.src.stdio.fprintf
diff --git a/libc/spec/stdc.td b/libc/spec/stdc.td
index afe01b1bb68566..c52463f23c5069 100644
--- a/libc/spec/stdc.td
+++ b/libc/spec/stdc.td
@@ -695,6 +695,11 @@ def StdC : StandardSpec<"stdc"> {
               RetValSpec<IntType>,
               [ArgSpec<ConstCharPtr>]
           >,
+          FunctionSpec<
+              "rename",
+              RetValSpec<IntType>,
+              [ArgSpec<ConstCharPtr>, ArgSpec<ConstCharPtr>]
+          >,
           FunctionSpec<
               "setbuf",
               RetValSpec<VoidType>,
diff --git a/libc/src/stdio/CMakeLists.txt b/libc/src/stdio/CMakeLists.txt
index bb8e41606c5dfb..bc3e30239072f3 100644
--- a/libc/src/stdio/CMakeLists.txt
+++ b/libc/src/stdio/CMakeLists.txt
@@ -246,6 +246,13 @@ add_entrypoint_object(
     .${LIBC_TARGET_OS}.remove
 )
 
+add_entrypoint_object(
+  rename
+  ALIAS
+  DEPENDS
+    .${LIBC_TARGET_OS}.rename
+)
+
 # These entrypoints have multiple potential implementations.
 add_stdio_entrypoint_object(feof)
 add_stdio_entrypoint_object(feof_unlocked)
diff --git a/libc/src/stdio/linux/CMakeLists.txt b/libc/src/stdio/linux/CMakeLists.txt
index 3b5a9960dc125e..8579b86665b736 100644
--- a/libc/src/stdio/linux/CMakeLists.txt
+++ b/libc/src/stdio/linux/CMakeLists.txt
@@ -11,3 +11,17 @@ add_entrypoint_object(
     libc.src.__support.OSUtil.osutil
     libc.src.errno.errno
 )
+
+add_entrypoint_object(
+  rename
+  SRCS
+    rename.cpp
+  HDRS
+    ../rename.h
+  DEPENDS
+    libc.include.fcntl
+    libc.include.unistd
+    libc.include.sys_syscall
+    libc.src.__support.OSUtil.osutil
+    libc.src.errno.errno
+)
diff --git a/libc/src/stdio/linux/rename.cpp b/libc/src/stdio/linux/rename.cpp
new file mode 100644
index 00000000000000..d89f70af3cc593
--- /dev/null
+++ b/libc/src/stdio/linux/rename.cpp
@@ -0,0 +1,38 @@
+//===-- Linux implementation of rename ------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/stdio/rename.h"
+
+#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
+#include "src/__support/common.h"
+
+#include "src/errno/libc_errno.h"
+#include <fcntl.h>       // For AT_* macros.
+#include <sys/syscall.h> // For syscall numbers.
+
+namespace LIBC_NAMESPACE {
+
+LLVM_LIBC_FUNCTION(int, rename, (const char *oldpath, const char *newpath)) {
+#if defined(SYS_rename)
+  int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_rename, oldpath, newpath);
+#elif defined(SYS_renameat)
+  int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_renameat, AT_FDCWD, oldpath,
+                                              AT_FDCWD, newpath);
+#else
+  int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_renameat2, AT_FDCWD, oldpath,
+                                              AT_FDCWD, newpath, 0);
+#endif
+
+  if (ret >= 0)
+    return 0;
+  libc_errno = -ret;
+  return -1;
+}
+
+} // namespace LIBC_NAMESPACE
+
diff --git a/libc/src/stdio/rename.h b/libc/src/stdio/rename.h
new file mode 100644
index 00000000000000..86a2e9912dcf63
--- /dev/null
+++ b/libc/src/stdio/rename.h
@@ -0,0 +1,21 @@
+//===-- Implementation header of rename -------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDIO_RENAME_H
+#define LLVM_LIBC_SRC_STDIO_RENAME_H
+
+#include <stdio.h>
+
+namespace LIBC_NAMESPACE {
+
+int rename(const char *oldpath, const char *newpath);
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC_STDIO_RENAME_H
+
diff --git a/libc/test/src/stdio/CMakeLists.txt b/libc/test/src/stdio/CMakeLists.txt
index 6e1c86e070a823..3c457f671dc701 100644
--- a/libc/test/src/stdio/CMakeLists.txt
+++ b/libc/test/src/stdio/CMakeLists.txt
@@ -355,6 +355,23 @@ if(${LIBC_TARGET_OS} STREQUAL "linux")
   )
 endif()
 
+if(${LIBC_TARGET_OS} STREQUAL "linux")
+  add_libc_test(
+    rename_test
+    SUITE
+      libc_stdio_unittests
+    SRCS
+      rename_test.cpp
+    DEPENDS
+      libc.include.unistd
+      libc.src.errno.errno
+      libc.src.fcntl.open
+      libc.src.stdio.rename
+      libc.src.unistd.access
+      libc.src.unistd.close
+  )
+endif()
+
 add_libc_test(
   fgetc_test
   SUITE
diff --git a/libc/test/src/stdio/rename_test.cpp b/libc/test/src/stdio/rename_test.cpp
new file mode 100644
index 00000000000000..f3b15bc83772c5
--- /dev/null
+++ b/libc/test/src/stdio/rename_test.cpp
@@ -0,0 +1,50 @@
+//===-- Unittests for rename ----------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/fcntl/open.h"
+#include "src/stdio/rename.h"
+#include "src/unistd/access.h"
+#include "src/unistd/close.h"
+#include "test/UnitTest/ErrnoSetterMatcher.h"
+#include "test/UnitTest/Test.h"
+
+#include "src/errno/libc_errno.h"
+#include <unistd.h>
+
+TEST(LlvmLibcRenameTest, CreateAndRenameFile) {
+  // The test strategy is to create a file and rename it, and also verify that
+  // it was renamed.
+  LIBC_NAMESPACE::libc_errno = 0;
+  using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
+  using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+
+  constexpr const char *FILENAME0 = "rename.test.file0";
+  auto TEST_FILEPATH0 = libc_make_test_file_path(FILENAME0);
+
+  int fd = LIBC_NAMESPACE::open(TEST_FILEPATH0, O_WRONLY | O_CREAT, S_IRWXU);
+  ASSERT_ERRNO_SUCCESS();
+  ASSERT_GT(fd, 0);
+  ASSERT_THAT(LIBC_NAMESPACE::close(fd), Succeeds(0));
+  ASSERT_THAT(LIBC_NAMESPACE::access(TEST_FILEPATH0, F_OK), Succeeds(0));
+
+  constexpr const char *FILENAME1 = "rename.test.file1";
+  auto TEST_FILEPATH1 = libc_make_test_file_path(FILENAME1);
+  ASSERT_THAT(LIBC_NAMESPACE::rename(TEST_FILEPATH0, TEST_FILEPATH1), Succeeds(0));
+  ASSERT_THAT(LIBC_NAMESPACE::access(TEST_FILEPATH1, F_OK), Succeeds(0));
+  ASSERT_THAT(LIBC_NAMESPACE::access(TEST_FILEPATH0, F_OK), Fails(ENOENT));
+}
+
+TEST(LlvmLibcRenameTest, RenameNonExistent) {
+  using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
+
+  constexpr const char *FILENAME1 = "rename.test.file1";
+  auto TEST_FILEPATH1 = libc_make_test_file_path(FILENAME1);
+
+  ASSERT_THAT(LIBC_NAMESPACE::rename("non-existent", TEST_FILEPATH1), Fails(ENOENT));
+}
+
diff --git a/utils/bazel/llvm-project-overlay/libc/BUILD.bazel b/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
index 073353a89c8907..b91e3d24527102 100644
--- a/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
@@ -3230,6 +3230,17 @@ libc_function(
     ],
 )
 
+libc_function(
+    name = "rename",
+    srcs = ["src/stdio/linux/rename.cpp"],
+    hdrs = ["src/stdio/rename.h"],
+    deps = [
+        ":__support_common",
+        ":__support_osutil_syscall",
+        ":errno",
+    ],
+)
+
 ############################### sys/stat targets ###############################
 
 libc_function(

Copy link

github-actions bot commented Mar 13, 2024

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

@SchrodingerZhu
Copy link
Contributor

https://pubs.opengroup.org/onlinepubs/9699919799/ seems to include a renameat function.

@nickdesaulniers
Copy link
Member

https://pubs.opengroup.org/onlinepubs/9699919799/ seems to include a renameat function.

That can be fixed in a distinct PR. For now, #84980 is just concerned with minimally supporting libc++.

Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

Looks good so far! Thanks for the patch!

libc/src/stdio/linux/CMakeLists.txt Outdated Show resolved Hide resolved
libc/src/stdio/linux/rename.cpp Outdated Show resolved Hide resolved
libc/src/stdio/linux/rename.cpp Outdated Show resolved Hide resolved
libc/test/src/stdio/rename_test.cpp Outdated Show resolved Hide resolved
libc/src/stdio/rename.h Outdated Show resolved Hide resolved
Copy link
Contributor

@michaelrj-google michaelrj-google left a comment

Choose a reason for hiding this comment

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

LGTM

libc/test/src/stdio/CMakeLists.txt Outdated Show resolved Hide resolved
@aniplcc aniplcc requested a review from rupprecht as a code owner March 20, 2024 00:34
@llvmbot llvmbot added the bazel "Peripheral" support tier build system: utils/bazel label Mar 20, 2024
Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

libc/test/src/stdio/rename_test.cpp Outdated Show resolved Hide resolved
libc/src/stdio/linux/rename.cpp Outdated Show resolved Hide resolved
Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

looks great! Thanks for the PR. Do you have permissions to merge, or do you need one of us to do that for you?

@aniplcc
Copy link
Contributor Author

aniplcc commented Mar 21, 2024

Thanks! I don't have write access, so I don't think I can merge this PR myself? @nickdesaulniers

@nickdesaulniers nickdesaulniers merged commit c04807c into llvm:main Mar 21, 2024
5 checks passed
@nickdesaulniers
Copy link
Member

nickdesaulniers commented Mar 21, 2024

Looks like the aarch64 buildbot is failing with:

llvm-project/libc/src/stdio/linux/rename.cpp:18:47: error: use of undeclared identifier 'SYS_rename'
  int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_rename, oldpath, newpath);
                                              ^

Same for the riscv64 buildbot.

The the fullbuild buildbots are failing to build the test:

llvm-project/libc/test/src/stdio/rename_test.cpp:27:69: error: use of undeclared identifier 'S_IRWXU'
  int fd = LIBC_NAMESPACE::open(TEST_FILEPATH0, O_WRONLY | O_CREAT, S_IRWXU);
                                                                    ^
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-fullbuild-dbg/llvm-project/libc/test/src/stdio/rename_test.cpp:31:54: error: use of undeclared identifier 'F_OK'
  ASSERT_THAT(LIBC_NAMESPACE::access(TEST_FILEPATH0, F_OK), Succeeds(0));
                                                     ^
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-fullbuild-dbg/llvm-project/libc/test/src/stdio/rename_test.cpp:37:54: error: use of undeclared identifier 'F_OK'
  ASSERT_THAT(LIBC_NAMESPACE::access(TEST_FILEPATH1, F_OK), Succeeds(0));
                                                     ^
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian-fullbuild/libc-x86_64-debian-fullbuild-dbg/llvm-project/libc/test/src/stdio/rename_test.cpp:38:54: error: use of undeclared identifier 'F_OK'
  ASSERT_THAT(LIBC_NAMESPACE::access(TEST_FILEPATH0, F_OK), Fails(ENOENT));
                                                     ^

nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this pull request Mar 21, 2024
nickdesaulniers added a commit that referenced this pull request Mar 21, 2024
@nickdesaulniers
Copy link
Member

Looking into the riscv failure.

  1. our libc/config/linux/syscall_numbers.h.inc #include's asm/unistd.h.
  2. on the riscv64 build bot, this is /usr/include/riscv64-linux-gnu/asm/unistd.h which just #include's /usr/include/asm-generic/unistd.h.
  3. /usr/include/asm-generic/unistd.h does not define __NR_rename.

It does have definitions for:

  • __NR_renameat
  • __NR_renameat2

@nickdesaulniers
Copy link
Member

So the availability of SYS_rename or SYS_renameat seems like they might not exist (have been removed for newer architectures) while SYS_renameat2 might not exist (for ancient kernels, though I guess that's true for all syscalls).

Because we don't need to support such prehistoric kernels, I suspect this is a syscall that we'll need to implement in terms of SYS_renameat2.

nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this pull request Mar 21, 2024
SYS_rename may be unavailable on architectures such as aarch64 and riscv.
rename can be implemented in terms of SYS_rename, SYS_renameat, or
SYS_renameat2. I don't have a full picture of the history here, but it seems
that SYS_renameat might also be unavailable on some platforms.

`man 2 rename` mentions that SYS_renameat2 was added in Linux 3.15.  We don't
need to support such ancient kernel versions.

Link: llvm#84980
Link: llvm#85068
nickdesaulniers added a commit that referenced this pull request Mar 21, 2024
SYS_rename may be unavailable on architectures such as aarch64 and
riscv.
rename can be implemented in terms of SYS_rename, SYS_renameat, or
SYS_renameat2. I don't have a full picture of the history here, but it
seems
that SYS_renameat might also be unavailable on some platforms.

`man 2 rename` mentions that SYS_renameat2 was added in Linux 3.15. We
don't
need to support such ancient kernel versions prior.

Link: #84980
Link: #85068
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
Adds stdio.h's rename() function as defined in n3096. Fixes  llvm#84980.
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
SYS_rename may be unavailable on architectures such as aarch64 and
riscv.
rename can be implemented in terms of SYS_rename, SYS_renameat, or
SYS_renameat2. I don't have a full picture of the history here, but it
seems
that SYS_renameat might also be unavailable on some platforms.

`man 2 rename` mentions that SYS_renameat2 was added in Linux 3.15. We
don't
need to support such ancient kernel versions prior.

Link: llvm#84980
Link: llvm#85068
@aniplcc aniplcc deleted the test1 branch March 26, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel libc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[libc][c11] add support for stdio.h's rename()
5 participants