Skip to content

[libc] Add Linux mman extension remap_file_pages. #110307

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

Conversation

AlyElashram
Copy link
Contributor

@AlyElashram AlyElashram commented Sep 27, 2024

Fixes #110122

  • Create remap_file_pages.h/.cpp wrapper for the linux sys call.
  • Add UnitTests for remap_file_pages
  • Add function to libc/spec/linux.td
  • Add Function spec to mman.yaml

- Add UnitTest for remap_file_pages
- Add function to libc/spec/linux.td
- Add Function spec to mman.yaml
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added the libc label Sep 27, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 27, 2024

@llvm/pr-subscribers-libc

Author: Aly ElAshram (AlyElashram)

Changes
  • Create remap_file_pages.h/.cpp wrapper for the linux sys call.
  • Add UnitTests for remap_file_pages
  • Add function to libc/spec/linux.td
  • Add Function spec to mman.yaml

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

8 Files Affected:

  • (modified) libc/newhdrgen/yaml/sys/mman.yaml (+10)
  • (modified) libc/spec/linux.td (+11)
  • (modified) libc/src/sys/mman/CMakeLists.txt (+7)
  • (modified) libc/src/sys/mman/linux/CMakeLists.txt (+13)
  • (added) libc/src/sys/mman/linux/remap_file_pages.cpp (+44)
  • (added) libc/src/sys/mman/remap_file_pages.h (+21)
  • (modified) libc/test/src/sys/mman/linux/CMakeLists.txt (+17)
  • (added) libc/test/src/sys/mman/linux/remap_file_pages_test.cpp (+74)
diff --git a/libc/newhdrgen/yaml/sys/mman.yaml b/libc/newhdrgen/yaml/sys/mman.yaml
index 272e7e2af16aaf..2f5f199bddec83 100644
--- a/libc/newhdrgen/yaml/sys/mman.yaml
+++ b/libc/newhdrgen/yaml/sys/mman.yaml
@@ -98,6 +98,16 @@ functions:
       - type: void *
       - type: size_t
       - type: int
+  - name: remap_file_pages
+    standards:
+      - LINUX
+    return_type: int
+    arguments:
+      - type: void *
+      - type: size_t
+      - type: int
+      - type: size_t
+      - type: int
   - name: shm_open
     standards:
       - POSIX
diff --git a/libc/spec/linux.td b/libc/spec/linux.td
index 395c2a6fe853a7..4aaf18b0803f3d 100644
--- a/libc/spec/linux.td
+++ b/libc/spec/linux.td
@@ -103,6 +103,17 @@ def Linux : StandardSpec<"Linux"> {
             ArgSpec<UnsignedIntType>,
           ]
         >,
+         FunctionSpec<
+          "remap_file_pages",
+          RetValSpec<IntType>,
+          [
+            ArgSpec<VoidPtr>,
+            ArgSpec<SizeTType>,
+            ArgSpec<IntType>,
+            ArgSpec<SizeTType>,
+            ArgSpec<IntType>,
+          ]
+        >,
       ]  // Functions
   >;
 
diff --git a/libc/src/sys/mman/CMakeLists.txt b/libc/src/sys/mman/CMakeLists.txt
index 9c74202a09f035..4ea43e14be0297 100644
--- a/libc/src/sys/mman/CMakeLists.txt
+++ b/libc/src/sys/mman/CMakeLists.txt
@@ -86,6 +86,13 @@ add_entrypoint_object(
     .${LIBC_TARGET_OS}.msync
 )
 
+add_entrypoint_object(
+  remap_file_pages
+  ALIAS
+  DEPENDS
+  .${LIBC_TARGET_OS}.remap_file_pages
+)
+
 add_entrypoint_object(
   shm_open
   ALIAS
diff --git a/libc/src/sys/mman/linux/CMakeLists.txt b/libc/src/sys/mman/linux/CMakeLists.txt
index 00f4f0e64ec06b..f673d47ba96aef 100644
--- a/libc/src/sys/mman/linux/CMakeLists.txt
+++ b/libc/src/sys/mman/linux/CMakeLists.txt
@@ -153,6 +153,19 @@ add_entrypoint_object(
     libc.src.errno.errno
 )
 
+add_entrypoint_object(
+  remap_file_pages
+  SRCS
+    remap_file_pages.cpp.cpp
+  HDRS
+    ../remap_file_pages.h.h
+  DEPENDS
+    libc.include.sys_mman
+    libc.include.sys_syscall
+    libc.src.__support.OSUtil.osutil
+    libc.src.errno.errno
+)
+
 add_header_library(
   shm_common
   HDRS
diff --git a/libc/src/sys/mman/linux/remap_file_pages.cpp b/libc/src/sys/mman/linux/remap_file_pages.cpp
new file mode 100644
index 00000000000000..074d24d7c9b435
--- /dev/null
+++ b/libc/src/sys/mman/linux/remap_file_pages.cpp
@@ -0,0 +1,44 @@
+//===----- Linux implementation of the POSIX remap_file_pages 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/remap_file_pages.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 <sys/syscall.h>          // For syscall numbers.
+
+namespace LIBC_NAMESPACE_DECL {
+
+// This function is currently linux only. It has to be refactored suitably if
+// remap_file_pages is to be supported on non-linux operating systems also.
+LLVM_LIBC_FUNCTION(int, remap_file_pages,
+                   (void *addr, size_t size, int prot, size_t pgoff, int flags))
+                    {
+#ifdef SYS_remap_file_pages
+  long syscall_number = SYS_remap_file_pages;
+#else
+#error "remap_file_pages syscall is not available."
+#endif
+
+
+  int ret = LIBC_NAMESPACE::syscall_impl<int>(
+  syscall_number, reinterpret_cast<long>(addr), size, prot, pgoff, flags);
+
+  // A negative return value indicates an error with the magnitude of the
+  // value being the error code.
+  if (ret < 0) {
+    libc_errno = ret;
+    return -1;
+  }
+
+  return 0;
+}
+
+} // namespace LIBC_NAMESPACE_DECL
\ No newline at end of file
diff --git a/libc/src/sys/mman/remap_file_pages.h b/libc/src/sys/mman/remap_file_pages.h
new file mode 100644
index 00000000000000..cfb540fd904140
--- /dev/null
+++ b/libc/src/sys/mman/remap_file_pages.h
@@ -0,0 +1,21 @@
+//===-- Implementation header for remap_file_pages 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_REMAP_FILE_PAGES_H
+#define LLVM_LIBC_SRC_SYS_MMAN_REMAP_FILE_PAGES_H
+
+#include "src/__support/macros/config.h"
+#include <sys/mman.h>
+
+namespace LIBC_NAMESPACE_DECL {
+
+int remap_file_pages(void *addr, size_t size, int prot, size_t pgoff, int flags);
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif //LLVM_LIBC_SRC_SYS_MMAN_REMAP_FILE_PAGES_H
diff --git a/libc/test/src/sys/mman/linux/CMakeLists.txt b/libc/test/src/sys/mman/linux/CMakeLists.txt
index b63c76f4306fac..3718f732b425f6 100644
--- a/libc/test/src/sys/mman/linux/CMakeLists.txt
+++ b/libc/test/src/sys/mman/linux/CMakeLists.txt
@@ -128,6 +128,23 @@ add_libc_unittest(
     libc.test.UnitTest.ErrnoSetterMatcher
 )
 
+add_libc_unittest(
+  remap_file_pages_test
+  SUITE
+    libc_sys_mman_unittests
+  SRCS
+    remap_file_pages_test.cpp
+  DEPENDS
+    libc.include.sys_mman
+    libc.src.unistd.sysconf
+    libc.test.UnitTest.Test
+    libc.test.UnitTest.ErrnoSetterMatcher
+    libc.src.sys.mman.remap_file_pages
+    libc.src.errno.errno
+    libc.src.sys.mman.mmap
+    libc.src.sys.mman.munmap
+)
+
 add_libc_unittest(
   shm_test
   SUITE
diff --git a/libc/test/src/sys/mman/linux/remap_file_pages_test.cpp b/libc/test/src/sys/mman/linux/remap_file_pages_test.cpp
new file mode 100644
index 00000000000000..270ae4b6701e97
--- /dev/null
+++ b/libc/test/src/sys/mman/linux/remap_file_pages_test.cpp
@@ -0,0 +1,74 @@
+//===-- Unittests for remap_file_pages ------------------------------------===//
+//
+// 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/errno/libc_errno.h"
+#include "src/sys/mman/mmap.h"
+#include "src/sys/mman/munmap.h"
+#include "src/sys/mman/remap_file_pages.h"
+#include "test/UnitTest/ErrnoSetterMatcher.h"
+#include "test/UnitTest/Test.h"
+#include "src/unistd/sysconf.h"
+
+#include <sys/mman.h>
+
+
+using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
+using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+
+TEST(LlvmLibcRemapFilePagesTest, NoError) {
+  size_t page_size = sysconf(_SC_PAGE_SIZE);
+  ASSERT_GT(page_size, size_t(0));
+
+  // First, allocate some memory using mmap
+  size_t alloc_size = 2 * page_size;
+  LIBC_NAMESPACE::libc_errno = 0;
+  void *addr = LIBC_NAMESPACE::mmap(nullptr, alloc_size, PROT_READ | PROT_WRITE,
+                                    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  ASSERT_ERRNO_SUCCESS();
+  EXPECT_NE(addr, MAP_FAILED);
+
+  // Reset error number for the new function
+  LIBC_NAMESPACE::libc_errno = 0;
+
+  // Now try to remap the pages
+  EXPECT_THAT(LIBC_NAMESPACE::remap_file_pages(addr, page_size, PROT_READ, page_size, 0),
+              Succeeds());
+
+  // Reset error number for the new function
+  LIBC_NAMESPACE::libc_errno = 0;
+
+  // Clean up
+  EXPECT_THAT(LIBC_NAMESPACE::munmap(addr, alloc_size), Succeeds());
+}
+
+TEST(LlvmLibcRemapFilePagesTest, ErrorInvalidFlags) {
+  size_t page_size = sysconf(_SC_PAGESIZE);
+  ASSERT_GT(page_size, size_t(0));
+
+  void *addr = LIBC_NAMESPACE::mmap(nullptr, page_size, PROT_READ | PROT_WRITE,
+                                    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  ASSERT_NE(addr, MAP_FAILED);
+
+  // Try to remap pages with an invalid flag MAP_PRIVATE
+  EXPECT_THAT(LIBC_NAMESPACE::remap_file_pages(addr, page_size, PROT_READ, 0, MAP_PRIVATE),
+              Fails(EINVAL));
+
+  // Clean up
+  EXPECT_THAT(LIBC_NAMESPACE::munmap(addr, page_size), Succeeds());
+}
+
+TEST(LlvmLibcRemapFilePagesTest, ErrorInvalidAddress) {
+  size_t page_size = sysconf(_SC_PAGESIZE);
+  ASSERT_GT(page_size, size_t(0));
+
+  // Use an address that we haven't mapped
+  void *invalid_addr = reinterpret_cast<void*>(0x12345000);
+
+  EXPECT_THAT(LIBC_NAMESPACE::remap_file_pages(invalid_addr, page_size, PROT_READ, 0, 0),
+              Fails(EINVAL));
+}
\ No newline at end of file

@AlyElashram
Copy link
Contributor Author

@SchrodingerZhu I'm not really certain of the testing macros used , I kind of "monkey see monkey do"-ed them from other test files, so please let me know if something is off.

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.

This looks like a good start, I left some comments for minor things to fix.

return 0;
}

} // namespace LIBC_NAMESPACE_DECL
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: needs ending newline


EXPECT_THAT(LIBC_NAMESPACE::remap_file_pages(invalid_addr, page_size, PROT_READ, 0, 0),
Fails(EINVAL));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: needs ending newline

@@ -0,0 +1,44 @@
//===----- Linux implementation of the POSIX remap_file_pages function ----===//
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since this function isn't actually from POSIX, you can remove that from the title here.

Comment on lines 19 to 20
// This function is currently linux only. It has to be refactored suitably if
// remap_file_pages is to be supported on non-linux operating systems also.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since this is a linux specific function you don't need this comment.

(void *addr, size_t size, int prot, size_t pgoff, int flags))
{
#ifdef SYS_remap_file_pages
long syscall_number = SYS_remap_file_pages;
Copy link
Contributor

Choose a reason for hiding this comment

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

there's only one possible syscall here, so you can move the syscall into this ifdef like this (though you may need to reformat the code):

Suggested change
long syscall_number = SYS_remap_file_pages;
int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_remap_file_pages, reinterpret_cast<long>(addr), size, prot, pgoff, flags);

#define LLVM_LIBC_SRC_SYS_MMAN_REMAP_FILE_PAGES_H

#include "src/__support/macros/config.h"
#include <sys/mman.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need sys/mman.h here, since this function doesn't take any types that it defines. Instead this should just be stddef.h

Suggested change
#include <sys/mman.h>
#include <stddef.h>

//
//===----------------------------------------------------------------------===//

#include <src/sys/mman/remap_file_pages.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

this should use "quote marks" instead of <angle brackets>

Suggested change
#include <src/sys/mman/remap_file_pages.h>
#include "src/sys/mman/remap_file_pages.h"

@@ -98,6 +98,16 @@ functions:
- type: void *
- type: size_t
- type: int
- name: remap_file_pages
standards:
- LINUX
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: in other places it's capitalized as Linux

Suggested change
- LINUX
- Linux

Copy link

github-actions bot commented Sep 27, 2024

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

…d UnitTest for remap_file_pages - Add function to libc/spec/linux.td - Add Function spec to mman.yaml
@AlyElashram
Copy link
Contributor Author

AlyElashram commented Sep 27, 2024

I have one concern , I downloaded the logs for the previous build and didn't find the new remap_file_pages_test test run after searching for it. Shouldn't this be a concern? @michaelrj-google
I've attached the logs if you would like to take a look yourself.
github-pull-requests_build_104817_linux-linux-x64.log

@michaelrj-google
Copy link
Contributor

Ah, you need to add this function to entrypoints.txt which is the list of functions we provide for each target. If you want to see which functions are being skipped, add -DLIBC_CMAKE_VERBOSE_LOGGING=ON to your cmake command.

…d UnitTest for remap_file_pages - Add function to libc/spec/linux.td - Add Function spec to mman.yaml
Comment on lines 159 to 161
remap_file_pages.cpp.cpp
HDRS
../remap_file_pages.h.h
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is going to cause an error most definitely.

Suggested change
remap_file_pages.cpp.cpp
HDRS
../remap_file_pages.h.h
remap_file_pages.cpp
HDRS
../remap_file_pages.h

@AlyElashram AlyElashram marked this pull request as draft September 30, 2024 22:30
@AlyElashram
Copy link
Contributor Author

Converting this to a draft till I figure out why the tests are missing 👀

@lntue lntue changed the title Add Linux sys call wrapper in LibC [ Issue #110122 ] [libc] Add Linux mman extension remap_file_pages. Oct 1, 2024
Comment on lines 28 to 35
int fd = open("/dev/zero", O_RDWR);
ASSERT_GT(fd, 0);

// First, allocate some memory using mmap
size_t alloc_size = 2 * page_size;
LIBC_NAMESPACE::libc_errno = 0;
void *addr = LIBC_NAMESPACE::mmap(nullptr, alloc_size, PROT_READ | PROT_WRITE,
MAP_SHARED, fd, 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this makes much more sense when using remap_file_pages , instead of using MAP_PRIVATE and not having a file backed mapping. The use case of remap_file_pages should remap a file backed memory allocation if i understand correctly.

@michaelrj-google @SchrodingerZhu please correct me if I'm wrong here.

@AlyElashram AlyElashram marked this pull request as ready for review October 1, 2024 20:41
ASSERT_GT(page_size, size_t(0));

// Create a file-backed mapping
int fd = open("/dev/zero", O_RDWR);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're opening a file in a test you should use a file in the same directory as the test, similar to what's done in access_test

also small nit: this should use LIBC_NAMESPACE::open.

Comment on lines 39 to 40
// Reset error number for the new function
LIBC_NAMESPACE::libc_errno = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need to reset errno here since you asserted that it succeeded above.

Comment on lines 58 to 59
int fd = open("/dev/zero", O_RDWR);
ASSERT_GT(fd, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

same file path stuff as above, though make sure to use a different name since tests may be run in parallel.

@AlyElashram
Copy link
Contributor Author

@michaelrj-google @SchrodingerZhu Pinging on this as I think it's good to go. Awaiting approval to rebase 👀

@SchrodingerZhu
Copy link
Contributor

SchrodingerZhu commented Oct 13, 2024 via email

SRCS
remap_file_pages_test.cpp
DEPENDS
libc.include.sys_mman
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
libc.include.sys_mman
libc.include.sys_mman
libc.include.sys_stat

@SchrodingerZhu
Copy link
Contributor

This PR is of a good shape to go. Just some nit on header inclusion.

@AlyElashram
Copy link
Contributor Author

Should I rebase and push --force-with-lease after getting 2 approvals ? or should I do it now @SchrodingerZhu ?

@SchrodingerZhu SchrodingerZhu self-requested a review October 14, 2024 17:13
@SchrodingerZhu
Copy link
Contributor

Should I rebase and push --force-with-lease after getting 2 approvals ? or should I do it now @SchrodingerZhu ?

I guess no more operation on your side is needed for now. let's wait to see if the CI is happy.

@SchrodingerZhu SchrodingerZhu merged commit 76173b1 into llvm:main Oct 14, 2024
7 checks passed
Copy link

@AlyElashram Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
Fixes llvm#110122
- Create remap_file_pages.h/.cpp wrapper for the linux sys call.
- Add UnitTests for remap_file_pages
- Add function to libc/spec/linux.td
- Add Function spec to mman.yaml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[libc] add linux mman extension remap_file_pages
4 participants