-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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] pipe(2) linux syscall wrapper and unittest #85514
Conversation
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be 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 If you have received no comments on your PR for a week, you can request a review 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. |
@llvm/pr-subscribers-libc Author: None (muffpy) ChangesFull diff: https://github.com/llvm/llvm-project/pull/85514.diff 8 Files Affected:
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 4fb31c593b9dc7..702894b8a21d26 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -297,6 +297,7 @@ set(TARGET_LIBC_ENTRYPOINTS
libc.src.unistd.link
libc.src.unistd.linkat
libc.src.unistd.lseek
+ libc.src.unistd.pipe2
libc.src.unistd.pread
libc.src.unistd.pwrite
libc.src.unistd.read
diff --git a/libc/spec/linux.td b/libc/spec/linux.td
index f91f55ddac7846..80d3e860ed6c46 100644
--- a/libc/spec/linux.td
+++ b/libc/spec/linux.td
@@ -3,6 +3,8 @@ def StructEpollEventPtr : PtrType<StructEpollEvent>;
def StructEpollData : NamedType<"struct epoll_data">;
+def Pipe2fdArrayT : NamedType<"__pipe2fd_array_t">;
+
def Linux : StandardSpec<"Linux"> {
HeaderSpec Errno = HeaderSpec<
"errno.h",
@@ -264,6 +266,20 @@ def Linux : StandardSpec<"Linux"> {
]
>;
+ HeaderSpec UniStd = HeaderSpec<
+ "unistd.h",
+ [], // Macros
+ [Pipe2fdArrayT], // Types
+ [], // Enumerations
+ [
+ FunctionSpec<
+ "pipe2",
+ RetValSpec<IntType>,
+ [ArgSpec<Pipe2fdArrayT>, ArgSpec<IntType>]
+ >,
+ ]
+ >;
+
let Headers = [
Errno,
SysEpoll,
@@ -272,5 +288,6 @@ def Linux : StandardSpec<"Linux"> {
SysRandom,
SysTime,
Signal,
+ UniStd
];
}
diff --git a/libc/src/unistd/CMakeLists.txt b/libc/src/unistd/CMakeLists.txt
index e22b0e1872caa1..c63f170df29b96 100644
--- a/libc/src/unistd/CMakeLists.txt
+++ b/libc/src/unistd/CMakeLists.txt
@@ -149,6 +149,13 @@ add_entrypoint_object(
.${LIBC_TARGET_OS}.lseek
)
+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 df85d44e9e9edc..3496d32a18f2e4 100644
--- a/libc/src/unistd/linux/CMakeLists.txt
+++ b/libc/src/unistd/linux/CMakeLists.txt
@@ -273,6 +273,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..9f2e5390104703
--- /dev/null
+++ b/libc/src/unistd/linux/pipe2.cpp
@@ -0,0 +1,35 @@
+//===-- Linux implementation of 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/unistd/pipe2.h"
+
+#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
+#include "src/__support/common.h"
+
+#include "src/errno/libc_errno.h"
+#include <sys/syscall.h> // For syscall numbers.
+
+namespace LIBC_NAMESPACE {
+
+LLVM_LIBC_FUNCTION(int, pipe2, (int pipefd[2], int flags)) {
+ int ret;
+#ifdef SYS_pipe2
+ ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_pipe2, pipefd, flags);
+#elif defined (SYS_pipe)
+ ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_pipe, pipefd);
+#else
+#error "pipe and pipe2 not available."
+#endif
+ if (ret == -1) {
+ libc_errno = -ret;
+ return -1;
+ }
+ return ret;
+}
+
+} // namespace LIBC_NAMESPACE
diff --git a/libc/src/unistd/pipe2.h b/libc/src/unistd/pipe2.h
new file mode 100644
index 00000000000000..3cf5d81f1f280e
--- /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 <unistd.h>
+
+namespace LIBC_NAMESPACE {
+
+int pipe2(int pipefd[2], int flags);
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC_UNISTD_PIPE2_H
diff --git a/libc/test/src/unistd/CMakeLists.txt b/libc/test/src/unistd/CMakeLists.txt
index 3a7fe6f45c0911..76212a38467586 100644
--- a/libc/test/src/unistd/CMakeLists.txt
+++ b/libc/test/src/unistd/CMakeLists.txt
@@ -444,3 +444,22 @@ add_libc_test(
libc.src.stdio.fopencookie
libc.src.stdio.fflush
)
+
+add_libc_unittest(
+ pipe2_test
+ SUITE
+ libc_unistd_unittests
+ SRCS
+ pipe2_test.cpp
+ DEPENDS
+ libc.include.errno
+ libc.include.unistd
+ libc.src.errno.errno
+ libc.src.fcntl.open
+ libc.src.unistd.close
+ libc.src.unistd.pipe2
+ libc.src.unistd.read
+ libc.src.unistd.unlink
+ libc.src.unistd.write
+ libc.test.UnitTest.ErrnoSetterMatcher
+)
\ No newline at end of file
diff --git a/libc/test/src/unistd/pipe2_test.cpp b/libc/test/src/unistd/pipe2_test.cpp
new file mode 100644
index 00000000000000..8c91078df466c3
--- /dev/null
+++ b/libc/test/src/unistd/pipe2_test.cpp
@@ -0,0 +1,64 @@
+//===-- 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/fcntl/open.h"
+#include "src/unistd/close.h"
+#include "src/unistd/pipe2.h"
+#include "src/unistd/read.h"
+#include "src/unistd/unlink.h"
+#include "src/unistd/write.h"
+#include "test/UnitTest/ErrnoSetterMatcher.h"
+#include "test/UnitTest/Test.h"
+
+#include <sys/stat.h>
+
+TEST(LlvmLibcPipe2Test, ReadAndWriteViaPipe2) {
+ int pipefd[2];
+ int flags;
+
+ LIBC_NAMESPACE::libc_errno = 0;
+ using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+
+ // Create pipe(2) with flags set to 0
+ flags = 0;
+ ASSERT_NE(LIBC_NAMESPACE::pipe2(pipefd, flags), -1);
+ ASSERT_ERRNO_SUCCESS();
+
+ // Write something via the pipe and read from other end
+ constexpr char MESSAGE[] = "Hello from the write end!";
+ constexpr size_t MESSAGE_SIZE = sizeof(MESSAGE);
+ char buf[MESSAGE_SIZE];
+ ASSERT_EQ(ssize_t(MESSAGE_SIZE),
+ LIBC_NAMESPACE::write(pipefd[1], MESSAGE, MESSAGE_SIZE));
+ ASSERT_EQ(ssize_t(MESSAGE_SIZE),
+ LIBC_NAMESPACE::read(pipefd[0], buf, MESSAGE_SIZE));
+ ASSERT_STREQ(buf, MESSAGE);
+
+ // Close the pipe file descriptors
+ ASSERT_NE(LIBC_NAMESPACE::close(pipefd[0]), -1);
+ ASSERT_ERRNO_SUCCESS();
+ ASSERT_NE(LIBC_NAMESPACE::close(pipefd[1]), -1);
+ ASSERT_ERRNO_SUCCESS();
+}
+
+TEST(LlvmLibcPipe2Test, Pipe2BadFlags) {
+ int badflags = -1;
+ int pipefd[2];
+
+ using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
+ ASSERT_THAT(LIBC_NAMESPACE::pipe2(pipefd, badflag), Fails(EBADF));
+}
+
+TEST(LlvmLibcPipe2Test, Pipe2BadFD) {
+ int flags = 0;
+ int badpipefd[1];
+
+ using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
+ ASSERT_THAT(LIBC_NAMESPACE::pipe2(badpipefd, flags), Fails(EBADF));
+}
|
You can test this locally with the following command:git-clang-format --diff 61fadd0b09fb012b628b050725d348ad2164f328 95f743d8dd9d3c30dab305957bcd2f3fed1ed2e4 -- libc/include/llvm-libc-macros/linux/watch-queue-macros.h libc/src/unistd/linux/pipe2.cpp libc/src/unistd/pipe2.h libc/test/src/unistd/pipe2_test.cpp libc/include/llvm-libc-macros/linux/fcntl-macros.h View the diff from clang-format here.diff --git a/libc/include/llvm-libc-macros/linux/watch-queue-macros.h b/libc/include/llvm-libc-macros/linux/watch-queue-macros.h
index d79f13354c..55410f7613 100644
--- a/libc/include/llvm-libc-macros/linux/watch-queue-macros.h
+++ b/libc/include/llvm-libc-macros/linux/watch-queue-macros.h
@@ -1,4 +1,5 @@
-//===-- Definition of macros from watch-queue.h ---------------------------------===//
+//===-- Definition of macros from watch-queue.h
+//---------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
|
@nickdesaulniers please ignore my old pull request (85402) and review this patch instead 🙂 |
Note: this may have a merge conflict with #84587 (it doesn't currently, just something to keep an eye on). |
There was a problem hiding this 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!
#include "test/UnitTest/ErrnoSetterMatcher.h" | ||
#include "test/UnitTest/Test.h" | ||
|
||
#include <fcntl.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include include/llvm-libc-macros/linux/fcntl-macros.h instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and make sure to remove <fcntl.h>. If you get a compile time failure as a result, let me know what it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need <fcntl.h> for this subtest in Pipe2CreationTest.
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds; | ||
|
||
// Create pipe(2) with flags set to 0 | ||
flags = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of flags we define, it looks like we only define O_CLOEXEC
and O_NONBLOCK
. You should add definitions of O_DIRECT
and O_NOTIFICATION_PIPE
to include/llvm-libc-macros/linux/fcntl-macros.h.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. And created a new uapi header for O_NOTIFICATION_PIPE. May I ask if we are currently not categorizing headers into things like asm-generic and uapi?
Thank you for the suggestions! May I ask your help in fixing my local dev setup because right now, the following command is not creating and running the tests necessary for pipe2:
In fact, when I look inside llvm-project/build/projects/libc/test/src/unistd after running the above But, my patch seems to generate and run the necessary tests for pipe2 in buildkite so it's defo a problem with my local setup. |
#include "test/UnitTest/ErrnoSetterMatcher.h" | ||
#include "test/UnitTest/Test.h" | ||
|
||
#include <fcntl.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and make sure to remove <fcntl.h>. If you get a compile time failure as a result, let me know what it is.
libc/spec/linux.td
Outdated
FunctionSpec< | ||
"pipe2", | ||
RetValSpec<IntType>, | ||
[ArgSpec<IntPtr>, ArgSpec<IntType>] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment // TODO: use int array for the first ArgSpec when possible
.
#define LLVM_LIBC_MACROS_LINUX_WATCH_QUEUE_MACROS_H | ||
|
||
#define O_NOTIFICATION_PIPE \ | ||
O_EXCL /* Parameter to pipe2() selecting notification pipe */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is not something we should be providing; it's ok to #include <linux/watch_queue.h>
for the definition of O_NOTIFICATION_PIPE
.
I think that include should go in libc/include/llvm-libc-macros/unistd-macros.h.
libc/include/llvm-libc-macros/unistd-macros.h should then be included in libc/test/src/unistd/pipe2_test.cpp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are weird conflict errors coming from including libc/include/llvm-libc-macros/unistd-macros.h
in the pipe2_test.cpp file because I have included src/unistd/read.h
too which contains #include <unistd.h>
. And <unistd.h>
has some beef with libc/.../unistd-macros.h
.
[27/29] Building CXX object projects/libc/test/src/unistd/CMa...ibc.test.src.unistd.pipe2_test.__build__.dir/pipe2_test.cpp.o
FAILED: projects/libc/test/src/unistd/CMakeFiles/libc.test.src.unistd.pipe2_test.__build__.dir/pipe2_test.cpp.o
/usr/bin/clang++ -DLIBC_NAMESPACE=__llvm_libc_19_0_0_git -I/home/llvm-project/build/projects/libc/test/src/unistd -I/home/llvm-project/libc/test/src/unistd -I/home/llvm-project/libc -isystem /home/llvm-project/build/projects/libc/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -fpie -mcpu=native -fno-exceptions -fno-rtti -std=c++17 -MD -MT projects/libc/test/src/unistd/CMakeFiles/libc.test.src.unistd.pipe2_test.__build__.dir/pipe2_test.cpp.o -MF projects/libc/test/src/unistd/CMakeFiles/libc.test.src.unistd.pipe2_test.__build__.dir/pipe2_test.cpp.o.d -o projects/libc/test/src/unistd/CMakeFiles/libc.test.src.unistd.pipe2_test.__build__.dir/pipe2_test.cpp.o -c /home/llvm-project/libc/test/src/unistd/pipe2_test.cpp
In file included from /home/llvm-project/libc/test/src/unistd/pipe2_test.cpp:15:
In file included from /home/llvm-project/libc/src/unistd/read.h:12:
In file included from /usr/include/unistd.h:630:
/usr/include/aarch64-linux-gnu/bits/confname.h:133:5: error: expected identifier
_SC_PAGESIZE,
^
/home/llvm-project/libc/include/llvm-libc-macros/linux/unistd-macros.h:21:22: note: expanded from macro '_SC_PAGESIZE'
#define _SC_PAGESIZE 1
^
In file included from /home/llvm-project/libc/test/src/unistd/pipe2_test.cpp:15:
In file included from /home/llvm-project/libc/src/unistd/read.h:12:
/usr/include/unistd.h:1091:35: error: expected ')'
extern long int syscall (long int __sysno, ...) __THROW;
^
/usr/include/unistd.h:1091:17: note: to match this '('
extern long int syscall (long int __sysno, ...) __THROW;
^
/home/llvm-project/libc/include/llvm-libc-macros/linux/unistd-macros.h:31:22: note: expanded from macro 'syscall'
#define syscall(...) __syscall_helper(__VA_ARGS__, 0, 1, 2, 3, 4, 5, 6)
^
/home/llvm-project/libc/include/llvm-libc-macros/linux/unistd-macros.h:29:29: note: expanded from macro '__syscall_helper'
__llvm_libc_syscall((long)(sysno), (long)(arg1), (long)(arg2), (long)(arg3), \
^
In file included from /home/llvm-project/libc/test/src/unistd/pipe2_test.cpp:15:
In file included from /home/llvm-project/libc/src/unistd/read.h:12:
/usr/include/unistd.h:1091:17: error: expected expression
extern long int syscall (long int __sysno, ...) __THROW;
^
/home/llvm-project/libc/include/llvm-libc-macros/linux/unistd-macros.h:31:22: note: expanded from macro 'syscall'
#define syscall(...) __syscall_helper(__VA_ARGS__, 0, 1, 2, 3, 4, 5, 6)
^
/home/llvm-project/libc/include/llvm-libc-macros/linux/unistd-macros.h:29:36: note: expanded from macro '__syscall_helper'
__llvm_libc_syscall((long)(sysno), (long)(arg1), (long)(arg2), (long)(arg3), \
^
In file included from /home/llvm-project/libc/test/src/unistd/pipe2_test.cpp:15:
In file included from /home/llvm-project/libc/src/unistd/read.h:12:
/usr/include/unistd.h:1091:44: error: expected expression
extern long int syscall (long int __sysno, ...) __THROW;
^
/usr/include/unistd.h:1091:48: error: expected ';' after top level declarator
extern long int syscall (long int __sysno, ...) __THROW;
^
5 errors generated.
ninja: build stopped: subcommand failed.
PS: Btw, this error happens even without #include <linux/watch_queue.h>
in libc/include/llvm-libc-macros/unistd-macros.h
but that should be clear from the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And <unistd.h>
Yeah, that's a pretty endemic issue throughout our libc; most of the includes of <unistd.h>
currently look incorrect to me.
Let me see if I can clean that up real quick, then you can rebase on my change. I'll try to get that posted real quick, let's see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind waiting for #85971 to land, then are you comfortable rebasing your changes on top of that? I'd wait until it lands; it's possible to rebase your branch on my branch BEFORE my branch lands, but IME if my branch gets updated before landing, it causes merge conflicts getting yours landed that are a waste of your time to resolve.
@@ -1,5 +1,4 @@ | |||
//===-- Definition of macros from watch-queue.h | |||
//---------------------------------===// | |||
//===-- Definition of macros from watch-queue.h ---------------------------------===// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git clang-format
does not like this. Please ignore for now as this file may not even be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That header is way too long, shorten it to 80 characters and it will stop doing this.
It's not hermetic to include the system libc's headers and is one of the reasons why our linter bot has been very red for a long time. This is starting to be an issue for new contributors as well, since we're now getting conflicts between the parts of our codebase that are hermetic, vs parts that aren't. I need to think about a rule of thumb or policy that can be publicly documented so that we don't backslide. I should probably clean up test/ as well, but let's clean up just src/ to unblock fellow contributors. Link: llvm#85514 (comment)
It's not hermetic to include the system libc's headers and is one of the reasons why our linter bot has been very red for a long time. This is starting to be an issue for new contributors as well, since we're now getting conflicts between the parts of our codebase that are hermetic, vs parts that aren't. I need to think about a rule of thumb or policy that can be publicly documented so that we don't backslide. I should probably clean up test/ as well, but let's clean up just src/ to unblock fellow contributors. Link: llvm#85514 (comment)
//===----------------------------------------------------------------------===// | ||
|
||
#include "include/llvm-libc-macros/linux/fcntl-macros.h" | ||
#include "include/llvm-libc-macros/linux/watch-queue-macros.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way we interact with public headers changed since this patch was written. The short version is we're now using proxy headers in /hdr/
instead of including the types from include
directly. You can see an example of this in the patch that adds pipe
: https://github.com/llvm/llvm-project/pull/84587/files#diff-0d107f2b3067e045736b456ccc6d3523aedade072afc031562b9103cb1972805
Sorry about the delay and the changing design, but this should be the final change needed before landing.
Hi @muffpy , are you still working on this PR, or should we close it? |
Is it possible to finish this PR? |
Please confirm if you plan to finish this PR, or not. |
closing for now, but happy to reopen+reassign #85289. |
No description provided.