[libc] revert all process_mrelease changes #118650

SchrodingerZhu · 2024-12-04T14:48:11Z

Revert as its test is unstable. #118057

llvmbot · 2024-12-04T14:48:47Z

@llvm/pr-subscribers-libc

Author: Schrodinger ZHU Yifan (SchrodingerZhu)

Changes

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

11 Files Affected:

(modified) libc/config/linux/aarch64/entrypoints.txt (-1)
(modified) libc/config/linux/riscv/entrypoints.txt (-2)
(modified) libc/config/linux/x86_64/entrypoints.txt (-1)
(modified) libc/include/sys/syscall.h.def (-7)
(modified) libc/newhdrgen/yaml/sys/mman.yaml (-7)
(modified) libc/src/sys/mman/CMakeLists.txt (-6)
(modified) libc/src/sys/mman/linux/CMakeLists.txt (+1-11)
(removed) libc/src/sys/mman/linux/process_mrelease.cpp (-41)
(removed) libc/src/sys/mman/process_mrelease.h (-21)
(modified) libc/test/src/sys/mman/linux/CMakeLists.txt (-21)
(removed) libc/test/src/sys/mman/linux/process_mrelease_test.cpp (-72)

diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index aa0b8ba0490e98..effa5b12d87ac4 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -252,7 +252,6 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.sys.mman.munlockall
     libc.src.sys.mman.munmap
     libc.src.sys.mman.remap_file_pages
-    libc.src.sys.mman.process_mrelease
     libc.src.sys.mman.posix_madvise
     libc.src.sys.mman.shm_open
     libc.src.sys.mman.shm_unlink
diff --git a/libc/config/linux/riscv/entrypoints.txt b/libc/config/linux/riscv/entrypoints.txt
index fedc5002d8c246..5a48baf104159f 100644
--- a/libc/config/linux/riscv/entrypoints.txt
+++ b/libc/config/linux/riscv/entrypoints.txt
@@ -251,8 +251,6 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.sys.mman.munmap
     libc.src.sys.mman.remap_file_pages
     libc.src.sys.mman.posix_madvise
-    # TODO: disabled due to buildbot failure. further investigation needed.
-    # libc.src.sys.mman.process_mrelease
     libc.src.sys.mman.shm_open
     libc.src.sys.mman.shm_unlink
 
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 5e9cc71279ab16..1bedc25a9d0291 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -252,7 +252,6 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.sys.mman.munmap
     libc.src.sys.mman.remap_file_pages
     libc.src.sys.mman.posix_madvise
-    libc.src.sys.mman.process_mrelease
     libc.src.sys.mman.shm_open
     libc.src.sys.mman.shm_unlink
 
diff --git a/libc/include/sys/syscall.h.def b/libc/include/sys/syscall.h.def
index 11758ea8336ddf..03c19eb0885ed6 100644
--- a/libc/include/sys/syscall.h.def
+++ b/libc/include/sys/syscall.h.def
@@ -2349,12 +2349,5 @@
 #define SYS_writev __NR_writev
 #endif
 
-#ifdef __NR_process_mrelease
-#define SYS_process_mrelease __NR_process_mrelease
-#endif
-
-#ifdef __NR_pidfd_open
-#define SYS_pidfd_open __NR_pidfd_open
-#endif
 
 #endif // LLVM_LIBC_SYS_SYSCALL_H
diff --git a/libc/newhdrgen/yaml/sys/mman.yaml b/libc/newhdrgen/yaml/sys/mman.yaml
index dd53eb60d1ec57..962ca3591917f7 100644
--- a/libc/newhdrgen/yaml/sys/mman.yaml
+++ b/libc/newhdrgen/yaml/sys/mman.yaml
@@ -132,10 +132,3 @@ functions:
     return_type: int
     arguments:
       - type: const char *
-  - name: process_mrelease
-    standards:
-      - Linux
-    return_type: int
-    arguments:
-      - type: int
-      - type: unsigned int
diff --git a/libc/src/sys/mman/CMakeLists.txt b/libc/src/sys/mman/CMakeLists.txt
index 281efc0ffcdf20..4d4c2ad376050e 100644
--- a/libc/src/sys/mman/CMakeLists.txt
+++ b/libc/src/sys/mman/CMakeLists.txt
@@ -113,9 +113,3 @@ add_entrypoint_object(
   DEPENDS
     .${LIBC_TARGET_OS}.mremap
 )
-
-add_entrypoint_object(
-  process_mrelease 
-  ALIAS 
-  DEPENDS
-    .${LIBC_TARGET_OS}.process_mrelease)
diff --git a/libc/src/sys/mman/linux/CMakeLists.txt b/libc/src/sys/mman/linux/CMakeLists.txt
index aa2ca4b160181a..89a0ad1527a065 100644
--- a/libc/src/sys/mman/linux/CMakeLists.txt
+++ b/libc/src/sys/mman/linux/CMakeLists.txt
@@ -36,6 +36,7 @@ add_entrypoint_object(
     libc.src.__support.OSUtil.osutil
     libc.src.errno.errno
 )
+
 add_entrypoint_object(
   munmap
   SRCS
@@ -213,14 +214,3 @@ add_entrypoint_object(
     libc.src.unistd.unlink
     .shm_common
 )
-
-add_entrypoint_object(
-  process_mrelease
-  SRCS
-    process_mrelease.cpp
-  HDRS
-    ../process_mrelease.h
-  DEPENDS
-    libc.include.sys_syscall
-    libc.src.__support.OSUtil.osutil
-    libc.src.errno.errno)
diff --git a/libc/src/sys/mman/linux/process_mrelease.cpp b/libc/src/sys/mman/linux/process_mrelease.cpp
deleted file mode 100644
index 7660f1e23ece2a..00000000000000
--- a/libc/src/sys/mman/linux/process_mrelease.cpp
+++ /dev/null
@@ -1,41 +0,0 @@
-//===---------- Linux implementation of the mrelease function -----------===//
-//
-// 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/sys/mman/process_mrelease.h"
-
-#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
-#include "src/__support/common.h"
-
-#include "src/__support/macros/config.h"
-#include "src/errno/libc_errno.h"
-#include <linux/param.h> // For EXEC_PAGESIZE.
-#include <sys/syscall.h> // For syscall numbers.
-
-namespace LIBC_NAMESPACE_DECL {
-
-LLVM_LIBC_FUNCTION(int, process_mrelease, (int pidfd, unsigned int flags)) {
-#ifdef SYS_process_mrelease
-  long ret =
-      LIBC_NAMESPACE::syscall_impl<int>(SYS_process_mrelease, pidfd, flags);
-
-  if (ret < 0) {
-    libc_errno = static_cast<int>(-ret);
-    return -1;
-  }
-
-  return 0;
-#else
-  // The system call is not available.
-  (void)pidfd;
-  (void)flags;
-  libc_errno = ENOSYS;
-  return -1;
-#endif
-}
-
-} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/sys/mman/process_mrelease.h b/libc/src/sys/mman/process_mrelease.h
deleted file mode 100644
index 6c800f2d0eab86..00000000000000
--- a/libc/src/sys/mman/process_mrelease.h
+++ /dev/null
@@ -1,21 +0,0 @@
-//===-- Implementation header for process_mrelease function -*- 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_SYS_MMAN_PROCESS_MRELEASE_H
-#define LLVM_LIBC_SRC_SYS_MMAN_PROCESS_MRELEASE_H
-
-#include "src/__support/macros/config.h"
-#include <sys/mman.h> // For size_t and off_t
-
-namespace LIBC_NAMESPACE_DECL {
-
-int process_mrelease(int pidfd, unsigned int flags);
-
-} // namespace LIBC_NAMESPACE_DECL
-
-#endif // LLVM_LIBC_SRC_SYS_MMAN_PROCESS_MRELEASE_H
diff --git a/libc/test/src/sys/mman/linux/CMakeLists.txt b/libc/test/src/sys/mman/linux/CMakeLists.txt
index 6362b46a6fe090..44ed11aadfe8b7 100644
--- a/libc/test/src/sys/mman/linux/CMakeLists.txt
+++ b/libc/test/src/sys/mman/linux/CMakeLists.txt
@@ -181,24 +181,3 @@ add_libc_unittest(
     libc.hdr.fcntl_macros
     libc.test.UnitTest.ErrnoSetterMatcher
 )
-
-add_libc_unittest(
-  process_mrelease_test
-  SUITE
-    libc_sys_mman_unittests
-  SRCS
-    process_mrelease_test.cpp
-  DEPENDS
-    libc.include.sys_mman
-    libc.include.sys_syscall
-    libc.src.errno.errno
-    libc.src.sys.mman.process_mrelease
-    libc.src.unistd.close
-    libc.src.signal.kill
-    libc.include.signal
-    libc.src.stdlib.exit
-    libc.src.signal.raise
-    libc.src.__support.OSUtil.osutil
-    libc.src.__support.threads.sleep
-    libc.test.UnitTest.ErrnoSetterMatcher
-)
diff --git a/libc/test/src/sys/mman/linux/process_mrelease_test.cpp b/libc/test/src/sys/mman/linux/process_mrelease_test.cpp
deleted file mode 100644
index 865056c05f8dbc..00000000000000
--- a/libc/test/src/sys/mman/linux/process_mrelease_test.cpp
+++ /dev/null
@@ -1,72 +0,0 @@
-//===-- Unittests for process_mrelease ------------------------------------===//
-//
-// 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/__support/OSUtil/syscall.h" // For internal syscall function.
-#include "src/errno/libc_errno.h"
-#include "src/signal/kill.h"
-#include "src/signal/raise.h"
-#include "src/stdlib/exit.h"
-#include "src/sys/mman/process_mrelease.h"
-#include "src/unistd/close.h"
-#include "src/unistd/fork.h"
-#include "test/UnitTest/ErrnoSetterMatcher.h"
-#include "test/UnitTest/LibcTest.h"
-
-#include <sys/syscall.h>
-#if defined(SYS_process_mrelease) && defined(SYS_pidfd_open)
-using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
-
-int pidfd_open(pid_t pid, unsigned int flags) {
-  return LIBC_NAMESPACE::syscall_impl(SYS_pidfd_open, pid, flags);
-}
-
-TEST(LlvmLibcProcessMReleaseTest, NoError) {
-  pid_t child_pid = fork();
-  EXPECT_GE(child_pid, 0);
-
-  if (child_pid == 0) {
-    // pause the child process
-    LIBC_NAMESPACE::raise(SIGSTOP);
-  } else {
-    // Parent process: wait a bit and then kill the child.
-    // Give child process some time to start.
-    int pidfd = pidfd_open(child_pid, 0);
-    EXPECT_GE(pidfd, 0);
-
-    // Send SIGKILL to child process
-    LIBC_NAMESPACE::kill(child_pid, SIGKILL);
-
-    EXPECT_THAT(LIBC_NAMESPACE::process_mrelease(pidfd, 0), Succeeds());
-
-    LIBC_NAMESPACE::close(pidfd);
-  }
-}
-
-TEST(LlvmLibcProcessMReleaseTest, ErrorNotKilled) {
-  pid_t child_pid = fork();
-  EXPECT_GE(child_pid, 0);
-
-  if (child_pid == 0) {
-    // pause the child process
-    LIBC_NAMESPACE::raise(SIGSTOP);
-  } else {
-    int pidfd = pidfd_open(child_pid, 0);
-    EXPECT_GE(pidfd, 0);
-
-    EXPECT_THAT(LIBC_NAMESPACE::process_mrelease(pidfd, 0), Fails(EINVAL));
-
-    LIBC_NAMESPACE::kill(child_pid, SIGKILL);
-
-    LIBC_NAMESPACE::close(pidfd);
-  }
-}
-
-TEST(LlvmLibcProcessMReleaseTest, ErrorNonExistingPidfd) {
-  EXPECT_THAT(LIBC_NAMESPACE::process_mrelease(-1, 0), Fails(EBADF));
-}
-#endif

SchrodingerZhu · 2024-12-04T14:52:18Z

@moar55 instead of stacking more fixes, let's just revert the changes. If you have time, could you resubmit this patch and removing all tests but the one testing the invalid argument? As @lntue said, we should not test syscall's correctness in libc's unittests.

moar55 · 2024-12-04T18:18:09Z

Sure :)
One question: are the #ifdef Syscall#-#endif clause necessary around the function implementation and tests?
Aren't we using the latest linux headers?

SchrodingerZhu · 2024-12-04T18:21:54Z

Some buildbots are using old environment, so it is better to guard the tests.

moar55 · 2024-12-04T19:29:14Z

@moar55 instead of stacking more fixes, let's just revert the changes. If you have time, could you resubmit this patch and removing all tests but the one testing the invalid argument? As @lntue said, we should not test syscall's correctness in libc's unittests.

Is this the one you mean with the one testing invalid argument:

TEST(LlvmLibcProcessMReleaseTest, ErrorNonExistingPidfd) {
  EXPECT_THAT(LIBC_NAMESPACE::process_mrelease(-1, 0), Fails(EBADF));
}

?

nickdesaulniers · 2024-12-04T19:33:28Z

hmm...bots have been green now for a while...

Some buildbots are using old environment, so it is better to guard the tests.

I've been meaning to explicitly document that llvm-libc will only support Linux kernel versions listed on kernel.org. Part of that is ensuring our buildbots are actually running LTS kernels (which I haven't done yet)! If we can move CI to qemu, this makes things simpler as its easier to update qemu than a whole linux distro the buildbot is running.

nickdesaulniers

Another failure overnight: https://lab.llvm.org/buildbot/#/builders/171/builds/11699/steps/4/logs/stdio

…lvm#118049)" This reverts commit cf478bf.

This reverts commit 700d9ac.

…ailable (llvm#117951)" This reverts commit 819b155.

…117851)" This reverts commit d2b482b.

moar55 · 2024-12-06T09:57:30Z

@moar55 instead of stacking more fixes, let's just revert the changes. If you have time, could you resubmit this patch and removing all tests but the one testing the invalid argument? As @lntue said, we should not test syscall's correctness in libc's unittests.

Is this the one you mean with the one testing invalid argument:
TEST(LlvmLibcProcessMReleaseTest, ErrorNonExistingPidfd) {
  EXPECT_THAT(LIBC_NAMESPACE::process_mrelease(-1, 0), Fails(EBADF));
}
?

@SchrodingerZhu can you respond to this question so I can create a new PR?

SchrodingerZhu · 2024-12-06T16:37:30Z

Yes, it is. Sorry for my late response.

llvmbot added the libc label Dec 4, 2024

SchrodingerZhu requested a review from lntue December 4, 2024 14:52

lntue requested a review from nickdesaulniers December 4, 2024 18:28

nickdesaulniers approved these changes Dec 5, 2024

View reviewed changes

SchrodingerZhu added 4 commits December 5, 2024 11:57

Revert "[libc] always clean up child process to avoid hanging ninja (l…

6cf357e

…lvm#118049)" This reverts commit cf478bf.

Revert "[libc] disable process_mrelease for riscv (llvm#117956)"

758d8b9

This reverts commit 700d9ac.

Revert "[libc] skip test and return ENOSYS when processm_release unav…

8962412

…ailable (llvm#117951)" This reverts commit 819b155.

Revert "[libc] (reland llvm#117503) Implement process_mrelease (llvm#…

eff7282

…117851)" This reverts commit d2b482b.

SchrodingerZhu force-pushed the revert/mrelease branch from cc299c2 to eff7282 Compare December 5, 2024 16:58

SchrodingerZhu merged commit 245f26a into llvm:main Dec 5, 2024
4 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libc] revert all process_mrelease changes #118650

[libc] revert all process_mrelease changes #118650

Uh oh!

SchrodingerZhu commented Dec 4, 2024 •

edited

Loading

Uh oh!

llvmbot commented Dec 4, 2024

Uh oh!

SchrodingerZhu commented Dec 4, 2024

Uh oh!

moar55 commented Dec 4, 2024 •

edited

Loading

Uh oh!

SchrodingerZhu commented Dec 4, 2024

Uh oh!

moar55 commented Dec 4, 2024

Uh oh!

nickdesaulniers commented Dec 4, 2024

Uh oh!

nickdesaulniers left a comment

Uh oh!

Uh oh!

moar55 commented Dec 6, 2024

Uh oh!

SchrodingerZhu commented Dec 6, 2024

Uh oh!

Uh oh!

[libc] revert all process_mrelease changes #118650

[libc] revert all process_mrelease changes #118650

Uh oh!

Conversation

SchrodingerZhu commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 4, 2024

Uh oh!

SchrodingerZhu commented Dec 4, 2024

Uh oh!

moar55 commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SchrodingerZhu commented Dec 4, 2024

Uh oh!

moar55 commented Dec 4, 2024

Uh oh!

nickdesaulniers commented Dec 4, 2024

Uh oh!

nickdesaulniers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

moar55 commented Dec 6, 2024

Uh oh!

SchrodingerZhu commented Dec 6, 2024

Uh oh!

Uh oh!

SchrodingerZhu commented Dec 4, 2024 •

edited

Loading

moar55 commented Dec 4, 2024 •

edited

Loading