-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[libc] pipe(2) linux syscall wrapper and unittest #85514
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
Changes from all commits
6a75de8
a8afc5c
174676f
c6fab70
30d2385
53c43e3
6100033
7b87696
863c2ab
63d9955
4cf1101
95f743d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| //===-- 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. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| // References | ||
| // https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/watch_queue.h | ||
| // https://kernelnewbies.org/Linux_5.8#Core_.28various.29 | ||
| // https://docs.kernel.org/core-api/watch_queue.html | ||
|
|
||
| #ifndef LLVM_LIBC_MACROS_LINUX_WATCH_QUEUE_MACROS_H | ||
| #define LLVM_LIBC_MACROS_LINUX_WATCH_QUEUE_MACROS_H | ||
|
|
||
| #define O_NOTIFICATION_PIPE \ | ||
| O_EXCL /* Parameter to pipe2() selecting notification pipe */ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are weird conflict errors coming from including PS: Btw, this error happens even without
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a pretty endemic issue throughout our libc; most of the includes of 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| #endif // LLVM_LIBC_MACROS_LINUX_WATCH_QUEUE_MACROS_H | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| //===-- 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); | ||
| #else | ||
| #error "pipe2 not available." | ||
| #endif | ||
| if (ret < 0) { | ||
| libc_errno = -ret; | ||
| return -1; | ||
| } | ||
| return ret; | ||
| } | ||
|
|
||
| } // namespace LIBC_NAMESPACE |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| //===-- 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 | ||
|
|
||
| namespace LIBC_NAMESPACE { | ||
|
|
||
| int pipe2(int pipefd[2], int flags); | ||
|
|
||
| } // namespace LIBC_NAMESPACE | ||
|
|
||
| #endif // LLVM_LIBC_SRC_UNISTD_PIPE2_H |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| //===-- 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 "include/llvm-libc-macros/linux/fcntl-macros.h" | ||
| #include "include/llvm-libc-macros/linux/watch-queue-macros.h" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Sorry about the delay and the changing design, but this should be the final change needed before landing. |
||
| #include "src/errno/libc_errno.h" | ||
| #include "src/unistd/close.h" | ||
| #include "src/unistd/pipe2.h" | ||
| #include "src/unistd/read.h" | ||
| #include "src/unistd/write.h" | ||
| #include "test/UnitTest/ErrnoSetterMatcher.h" | ||
| #include "test/UnitTest/Test.h" | ||
|
|
||
| #include <fcntl.h> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please include include/llvm-libc-macros/linux/fcntl-macros.h instead.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need <fcntl.h> for this subtest in Pipe2CreationTest. |
||
|
|
||
| TEST(LlvmLibcPipe2Test, Pipe2CreationTest) { | ||
| int pipefd[2]; | ||
| int flags; | ||
|
|
||
| LIBC_NAMESPACE::libc_errno = 0; | ||
| using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds; | ||
|
|
||
| // Create pipe(2) with all valid flags set | ||
| #ifdef CONFIG_WATCH_QUEUE | ||
| flags = O_CLOEXEC | O_NONBLOCK | O_DIRECT | O_NOTIFICATION_PIPE; | ||
| #else | ||
| flags = O_CLOEXEC | O_NONBLOCK | O_DIRECT; | ||
| #endif | ||
| ASSERT_NE(LIBC_NAMESPACE::pipe2(pipefd, flags), -1); | ||
| ASSERT_ERRNO_SUCCESS(); | ||
|
|
||
| // Check if file descriptors are distinct and valid | ||
| ASSERT_GE(pipefd[0], 0); | ||
| ASSERT_GE(pipefd[1], 0); | ||
| ASSERT_NE(pipefd[0], pipefd[1]); | ||
|
|
||
| // Check file status flags associated with pipe file descriptors | ||
| ASSERT_TRUE((fcntl(pipefd[0], F_GETFL) & flags) != 0); | ||
| ASSERT_TRUE((fcntl(pipefd[1], F_GETFL) & flags) != 0); | ||
|
|
||
| // 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, 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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In terms of flags we define, it looks like we only define
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
| 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, Pipe2InvalidFlags) { | ||
| int invalidflags = 0xDEADBEEF; | ||
| int pipefd[2]; | ||
|
|
||
| using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails; | ||
| ASSERT_THAT(LIBC_NAMESPACE::pipe2(pipefd, invalidflags), Fails(EINVAL)); | ||
muffpy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| TEST(LlvmLibcPipe2Test, Pipe2InvalidPipeFD) { | ||
| using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails; | ||
| ASSERT_THAT(LIBC_NAMESPACE::pipe2(NULL, 0), Fails(EFAULT)); | ||
| } | ||
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-formatdoes 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.