[libc] [unistd] implement pipe2 syscall wrapper #114474

duncpro · 2024-10-31T22:45:00Z

github-actions · 2024-10-31T22:45:21Z

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 · 2024-10-31T22:45:50Z

@llvm/pr-subscribers-libc

Author: Duncan (duncpro)

Changes

Closes #85289

Full diff: https://github.com/llvm/llvm-project/pull/114474.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/newhdrgen/yaml/unistd.yaml (+7)
(modified) libc/spec/linux.td (+18)
(modified) libc/src/unistd/CMakeLists.txt (+7)
(modified) libc/src/unistd/linux/CMakeLists.txt (+13)
(added) libc/src/unistd/linux/pipe2.cpp (+30)
(added) libc/src/unistd/pipe2.h (+20)
(modified) libc/test/src/unistd/CMakeLists.txt (+14)
(added) libc/test/src/unistd/pipe2_test.cpp (+30)

diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index b3f94a581c8ad9..69275f39355592 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -330,6 +330,7 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.unistd.lseek
     libc.src.unistd.pathconf
     libc.src.unistd.pipe
+    libc.src.unistd.pipe2
     libc.src.unistd.pread
     libc.src.unistd.pwrite
     libc.src.unistd.read
diff --git a/libc/config/linux/riscv/entrypoints.txt b/libc/config/linux/riscv/entrypoints.txt
index 5c09edf7cfb266..6e098abe630827 100644
--- a/libc/config/linux/riscv/entrypoints.txt
+++ b/libc/config/linux/riscv/entrypoints.txt
@@ -329,6 +329,7 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.unistd.lseek
     libc.src.unistd.pathconf
     libc.src.unistd.pipe
+    libc.src.unistd.pipe2
     libc.src.unistd.pread
     libc.src.unistd.pwrite
     libc.src.unistd.read
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index a2fb97d04584d5..a20dd79c64b70a 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -329,6 +329,7 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.unistd.lseek
     libc.src.unistd.pathconf
     libc.src.unistd.pipe
+    libc.src.unistd.pipe2
     libc.src.unistd.pread
     libc.src.unistd.pwrite
     libc.src.unistd.read
diff --git a/libc/newhdrgen/yaml/unistd.yaml b/libc/newhdrgen/yaml/unistd.yaml
index 4d12abe96a2ec4..c6441c04ce3a3d 100644
--- a/libc/newhdrgen/yaml/unistd.yaml
+++ b/libc/newhdrgen/yaml/unistd.yaml
@@ -204,6 +204,13 @@ functions:
     return_type: int
     arguments:
       - type: int *
+  - name: pipe2
+    standards:
+      - Linux
+    return_type: int
+    arguments:
+      - type: int *
+      - type: int
   - name: pread
     standards:
       - POSIX
diff --git a/libc/spec/linux.td b/libc/spec/linux.td
index 4aaf18b0803f3d..36281603ec0402 100644
--- a/libc/spec/linux.td
+++ b/libc/spec/linux.td
@@ -287,6 +287,23 @@ def Linux : StandardSpec<"Linux"> {
       ]
   >;
 
+
+  HeaderSpec UniStd = HeaderSpec<
+    "unistd.h",
+    [], // Macros
+    [],
+    [], // Enumerations
+    [
+        FunctionSpec<
+          "pipe",
+          RetValSpec<IntType>,
+          [ArgSpec<IntPtr>] //TODO: make this int[2]
+        >,
+    ],
+    []
+  >;
+  
+
   let Headers = [
     Errno,
     SysEpoll,
@@ -295,5 +312,6 @@ def Linux : StandardSpec<"Linux"> {
     SysRandom,
     SysTime,
     Signal,
+    UniStd,
   ];
 }
diff --git a/libc/src/unistd/CMakeLists.txt b/libc/src/unistd/CMakeLists.txt
index 64e07725010b31..1a0b2e3293d03c 100644
--- a/libc/src/unistd/CMakeLists.txt
+++ b/libc/src/unistd/CMakeLists.txt
@@ -182,6 +182,13 @@ add_entrypoint_object(
     .${LIBC_TARGET_OS}.pipe
 )
 
+add_entrypoint_object(
+  pipe2
+  ALIAS
+  DEPENDS
+    .${LIBC_TARGET_OS}.pipe2
+)
+
 add_entrypoint_object(
   pread
   ALIAS
diff --git a/libc/src/unistd/linux/CMakeLists.txt b/libc/src/unistd/linux/CMakeLists.txt
index 472438ca72e49e..33fca036291dfb 100644
--- a/libc/src/unistd/linux/CMakeLists.txt
+++ b/libc/src/unistd/linux/CMakeLists.txt
@@ -330,6 +330,19 @@ add_entrypoint_object(
     libc.src.errno.errno
 )
 
+add_entrypoint_object(
+  pipe2
+  SRCS
+    pipe2.cpp
+  HDRS
+    ../pipe2.h
+  DEPENDS
+    libc.include.unistd
+    libc.include.sys_syscall
+    libc.src.__support.OSUtil.osutil
+    libc.src.errno.errno
+)
+
 add_entrypoint_object(
   pread
   SRCS
diff --git a/libc/src/unistd/linux/pipe2.cpp b/libc/src/unistd/linux/pipe2.cpp
new file mode 100644
index 00000000000000..3820b0491cf279
--- /dev/null
+++ b/libc/src/unistd/linux/pipe2.cpp
@@ -0,0 +1,30 @@
+//===-- Linux implementation of pipe --------------------------------------===//
+//
+// 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/unistd/pipe2.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 {
+
+LLVM_LIBC_FUNCTION(int, pipe2, (int pipefd[2], int flags)) {
+  int ret = LIBC_NAMESPACE::syscall_impl<int>(
+      SYS_pipe2, reinterpret_cast<long>(pipefd), flags);
+
+  if (ret < 0) {
+    libc_errno = -ret;
+    return -1;
+  }
+  return ret;
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/unistd/pipe2.h b/libc/src/unistd/pipe2.h
new file mode 100644
index 00000000000000..7118f4aaf0171c
--- /dev/null
+++ b/libc/src/unistd/pipe2.h
@@ -0,0 +1,20 @@
+//===-- Implementation header for pipe2 -------------------------*- 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_UNISTD_PIPE2_H
+#define LLVM_LIBC_SRC_UNISTD_PIPE2_H
+
+#include "src/__support/macros/config.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+int pipe2(int pipefd[2], int flags);
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_UNISTD_PIPE2_H
diff --git a/libc/test/src/unistd/CMakeLists.txt b/libc/test/src/unistd/CMakeLists.txt
index ce936cebad4260..e036e09cde702e 100644
--- a/libc/test/src/unistd/CMakeLists.txt
+++ b/libc/test/src/unistd/CMakeLists.txt
@@ -217,6 +217,20 @@ add_libc_unittest(
     libc.test.UnitTest.ErrnoSetterMatcher
 )
 
+add_libc_unittest(
+  pipe2_test
+  SUITE
+    libc_unistd_unittests
+  SRCS
+    pipe2_test.cpp
+  DEPENDS
+    libc.include.unistd
+    libc.src.errno.errno
+    libc.src.unistd.close
+    libc.src.unistd.pipe2
+    libc.test.UnitTest.ErrnoSetterMatcher
+)
+
 add_libc_unittest(
   rmdir_test
   SUITE
diff --git a/libc/test/src/unistd/pipe2_test.cpp b/libc/test/src/unistd/pipe2_test.cpp
new file mode 100644
index 00000000000000..110fbb8ed05d5d
--- /dev/null
+++ b/libc/test/src/unistd/pipe2_test.cpp
@@ -0,0 +1,30 @@
+//===-- Unittests for pipe2 -----------------------------------------------===//
+//
+// 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/unistd/close.h"
+#include "src/unistd/pipe2.h"
+
+#include "test/UnitTest/ErrnoSetterMatcher.h"
+#include "test/UnitTest/Test.h"
+
+using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
+
+TEST(LlvmLibcPipe2Test, SmokeTest) {
+  int pipefd[2];
+  ASSERT_THAT(LIBC_NAMESPACE::pipe2(pipefd, 0), Succeeds());
+  ASSERT_THAT(LIBC_NAMESPACE::close(pipefd[0]), Succeeds());
+  ASSERT_THAT(LIBC_NAMESPACE::close(pipefd[1]), Succeeds());
+}
+
+TEST(LlvmLibcPipe2ErrTest, SmokeTest) {
+  int pipefd[2];
+  ASSERT_THAT(LIBC_NAMESPACE::pipe2(pipefd, -1), Fails(EINVAL));
+  ASSERT_THAT(LIBC_NAMESPACE::pipe2(nullptr, 0), Fails(EFAULT));
+}
+
+// TODO: Functionality tests

nickdesaulniers · 2024-10-31T22:52:00Z

libc/test/src/unistd/pipe2_test.cpp

+  ASSERT_THAT(LIBC_NAMESPACE::pipe2(nullptr, 0), Fails(EFAULT));
+}
+
+// TODO: Functionality tests


The tests aren't as thorough as https://github.com/llvm/llvm-project/pull/85514/files, but I think that's ok for syscall wrappers.

syscall wrapper tests just need to exercise the libc side, so for most syscalls we just need a success and an error. It's also good to check that the values it returns are valid, since it's easy to forget to unpoison memory for sanitizers.

The tests at https://github.com/llvm/llvm-project/pull/85514/files seem to be testing the kernel itself. For instance they verify that it's possible to read/write to the file descriptors, and that the descriptors are unique, etc. AFAIK this is outside the scope of libc. We assume the kernel is correct.

As far as the tests in the new PR go,

We've verified that when return code is -1, libc_errno is populated.

We've verified that pipefd holds valid file descriptors when the return code is 0. (If they were invalid close would not be successful.)

Is there anything I'm missing here?

Also, the comment // TODO: Functionality tests is copied from pipe_test.cpp. Can it be removed here?

Can it be removed here?

Sure. The linux kernel contains kselftests to exercise numerous things, including the syscall interfaces. Ideally, the functional test suite we'd use is to build the Linux kernel's kselftests against llvm-libc, then run the result.

duncpro · 2024-10-31T23:02:38Z

Just noticed spec/linux.td should declare pipe2 not pipe. I need to fix before we can merge.

libc/spec/linux.td

michaelrj-google

overall LGTM, with some minor tweaks. Thanks for the patch!

michaelrj-google · 2024-10-31T23:13:42Z

libc/test/src/unistd/pipe2_test.cpp

+  ASSERT_THAT(LIBC_NAMESPACE::pipe2(nullptr, 0), Fails(EFAULT));
+}
+
+// TODO: Functionality tests


syscall wrapper tests just need to exercise the libc side, so for most syscalls we just need a success and an error. It's also good to check that the values it returns are valid, since it's easy to forget to unpoison memory for sanitizers.

libc/src/unistd/linux/pipe2.cpp

nickdesaulniers · 2024-11-01T15:29:07Z

libc/test/src/unistd/pipe2_test.cpp

+  ASSERT_THAT(LIBC_NAMESPACE::pipe2(nullptr, 0), Fails(EFAULT));
+}
+
+// TODO: Functionality tests


Can it be removed here?

Sure. The linux kernel contains kselftests to exercise numerous things, including the syscall interfaces. Ideally, the functional test suite we'd use is to build the Linux kernel's kselftests against llvm-libc, then run the result.

nickdesaulniers · 2024-11-01T17:32:19Z

Do you need us to merge this for you? If not, please wait until the presubmits complete and are green before merging.

duncpro · 2024-11-01T17:34:18Z

I don't think I have write access

nickdesaulniers · 2024-11-01T17:35:50Z

ok, will merge once green. ping this thread if it's not merged by monday afternoon

michaelrj-google

Noticed one small issue in the oldhdrgen files.

libc/spec/linux.td

duncpro · 2024-11-05T22:53:47Z

@nickdesaulniers Will you merge this?

michaelrj-google

LGTM with one minor nit

libc/src/unistd/linux/pipe2.cpp

Co-Authored-By: Michael Jones <[email protected]>

michaelrj-google

LGTM, thanks for the patch! I'll let Nick merge this.

github-actions · 2024-11-06T16:35:22Z

@duncpro 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!

llvmbot added the libc label Oct 31, 2024

nickdesaulniers self-requested a review October 31, 2024 22:47

nickdesaulniers reviewed Oct 31, 2024

View reviewed changes

libc/spec/linux.td Outdated Show resolved Hide resolved

michaelrj-google reviewed Oct 31, 2024

View reviewed changes

nickdesaulniers approved these changes Nov 1, 2024

View reviewed changes

michaelrj-google reviewed Nov 1, 2024

View reviewed changes

libc/spec/linux.td Outdated Show resolved Hide resolved

michaelrj-google approved these changes Nov 5, 2024

View reviewed changes

libc/src/unistd/linux/pipe2.cpp Outdated Show resolved Hide resolved

[libc] [unistd] implement pipe2 syscall wrapper

2aeb511

Co-Authored-By: Michael Jones <[email protected]>

michaelrj-google approved these changes Nov 5, 2024

View reviewed changes

nickdesaulniers merged commit 6219c80 into llvm:main Nov 6, 2024
6 checks passed

[libc] [unistd] implement pipe2 syscall wrapper #114474

[libc] [unistd] implement pipe2 syscall wrapper #114474

Uh oh!

Conversation

duncpro commented Oct 31, 2024

Uh oh!

github-actions bot commented Oct 31, 2024

Uh oh!

llvmbot commented Oct 31, 2024

Uh oh!

nickdesaulniers Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

michaelrj-google Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

duncpro Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickdesaulniers Nov 1, 2024

Choose a reason for hiding this comment

Uh oh!

duncpro commented Oct 31, 2024

Uh oh!

Uh oh!

michaelrj-google left a comment

Choose a reason for hiding this comment

Uh oh!

michaelrj-google Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickdesaulniers Nov 1, 2024

Choose a reason for hiding this comment

Uh oh!

nickdesaulniers commented Nov 1, 2024

Uh oh!

duncpro commented Nov 1, 2024

Uh oh!

nickdesaulniers commented Nov 1, 2024

Uh oh!

michaelrj-google left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

duncpro commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelrj-google left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelrj-google left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Nov 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

duncpro Oct 31, 2024 •

edited

Loading

duncpro commented Nov 5, 2024 •

edited

Loading