-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Reland: [libc] implement ioctl #85890 #90317
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
base: main
Are you sure you want to change the base?
Conversation
This reverts commit 3c2feab.
@llvm/pr-subscribers-libc Author: Nhat Nguyen (changkhothuychung) ChangesFull diff: https://github.com/llvm/llvm-project/pull/90317.diff 13 Files Affected:
diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index 78da7f0b334b1fe..b5f8454081c8984 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -204,6 +204,9 @@ set(TARGET_LIBC_ENTRYPOINTS
#libc.src.stdio.scanf
#libc.src.stdio.fscanf
+ # sys/ioctl.h entrypoints
+ libc.src.sys.ioctl.ioctl
+
# sys/mman.h entrypoints
libc.src.sys.mman.madvise
libc.src.sys.mman.mmap
diff --git a/libc/config/linux/riscv/entrypoints.txt b/libc/config/linux/riscv/entrypoints.txt
index 5aae4e246cfb3c2..da9f0a9fb5b506d 100644
--- a/libc/config/linux/riscv/entrypoints.txt
+++ b/libc/config/linux/riscv/entrypoints.txt
@@ -209,6 +209,9 @@ set(TARGET_LIBC_ENTRYPOINTS
libc.src.stdio.scanf
libc.src.stdio.fscanf
+ # sys/ioctl.h entrypoints
+ libc.src.sys.ioctl.ioctl
+
# sys/mman.h entrypoints
libc.src.sys.mman.madvise
libc.src.sys.mman.mmap
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 5b428e51aee6206..923946a75a24f59 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -221,6 +221,9 @@ set(TARGET_LIBC_ENTRYPOINTS
# https://github.com/llvm/llvm-project/issues/80060
# libc.src.sys.epoll.epoll_pwait2
+ # sys/ioctl.h entrypoints
+ libc.src.sys.ioctl.ioctl
+
# sys/mman.h entrypoints
libc.src.sys.mman.madvise
libc.src.sys.mman.mmap
diff --git a/libc/spec/linux.td b/libc/spec/linux.td
index f91f55ddac78466..6e9b64f5a6b4aff 100644
--- a/libc/spec/linux.td
+++ b/libc/spec/linux.td
@@ -79,6 +79,24 @@ def Linux : StandardSpec<"Linux"> {
[] // Functions
>;
+ HeaderSpec SysIoctl = HeaderSpec<
+ "sys/ioctl.h",
+ [Macro<"MAP_ANONYMOUS">],
+ [], // Types
+ [], // Enumerations
+ [
+ FunctionSpec<
+ "ioctl",
+ RetValSpec<IntType>,
+ [
+ ArgSpec<IntType>,
+ ArgSpec<UnsignedLongType>,
+ ArgSpec<VoidPtr>,
+ ]
+ >,
+ ] // Functions
+ >;
+
HeaderSpec SysMMan = HeaderSpec<
"sys/mman.h",
[Macro<"MAP_ANONYMOUS">],
diff --git a/libc/src/sys/CMakeLists.txt b/libc/src/sys/CMakeLists.txt
index adc666b94202f7d..ac54df35284a7b8 100644
--- a/libc/src/sys/CMakeLists.txt
+++ b/libc/src/sys/CMakeLists.txt
@@ -1,5 +1,6 @@
add_subdirectory(auxv)
add_subdirectory(epoll)
+add_subdirectory(ioctl)
add_subdirectory(mman)
add_subdirectory(random)
add_subdirectory(resource)
diff --git a/libc/src/sys/ioctl/CMakeLists.txt b/libc/src/sys/ioctl/CMakeLists.txt
new file mode 100644
index 000000000000000..4b50c278c7871a8
--- /dev/null
+++ b/libc/src/sys/ioctl/CMakeLists.txt
@@ -0,0 +1,12 @@
+if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
+ add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
+endif()
+
+add_entrypoint_object(
+ ioctl
+ ALIAS
+ DEPENDS
+ .${LIBC_TARGET_OS}.ioctl
+)
+
+
diff --git a/libc/src/sys/ioctl/ioctl.h b/libc/src/sys/ioctl/ioctl.h
new file mode 100644
index 000000000000000..8365678276a4286
--- /dev/null
+++ b/libc/src/sys/ioctl/ioctl.h
@@ -0,0 +1,17 @@
+//===-- Implementation header for mmap 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_IOCTL_IOCTL_H
+#define LLVM_LIBC_SRC_SYS_IOCTL_IOCTL_H
+namespace LIBC_NAMESPACE {
+
+int ioctl(int fd, unsigned long request, ...);
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC_SYS_IOCTL_IOCTL_H
diff --git a/libc/src/sys/ioctl/linux/CMakeLists.txt b/libc/src/sys/ioctl/linux/CMakeLists.txt
new file mode 100644
index 000000000000000..8a23505d4e9d19e
--- /dev/null
+++ b/libc/src/sys/ioctl/linux/CMakeLists.txt
@@ -0,0 +1,13 @@
+add_entrypoint_object(
+ ioctl
+ SRCS
+ ioctl.cpp
+ HDRS
+ ../ioctl.h
+ DEPENDS
+ libc.include.sys_ioctl
+ libc.include.sys_syscall
+ libc.src.__support.OSUtil.osutil
+ libc.src.errno.errno
+)
+
diff --git a/libc/src/sys/ioctl/linux/ioctl.cpp b/libc/src/sys/ioctl/linux/ioctl.cpp
new file mode 100644
index 000000000000000..c743cf7a9dd5969
--- /dev/null
+++ b/libc/src/sys/ioctl/linux/ioctl.cpp
@@ -0,0 +1,39 @@
+//===---------- Linux implementation of the POSIX ioctl 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/ioctl/ioctl.h"
+
+#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
+#include "src/__support/common.h"
+#include "src/errno/libc_errno.h"
+#include <stdarg.h>
+#include <sys/syscall.h> // For syscall numbers.
+
+namespace LIBC_NAMESPACE {
+
+// This function is currently linux only. It has to be refactored suitably if
+// madvise is to be supported on non-linux operating systems also.
+LLVM_LIBC_FUNCTION(int, ioctl, (int fd, unsigned long request, ...)) {
+ va_list ptr_to_memory;
+ va_start(ptr_to_memory, request);
+ va_arg(ptr_to_memory, void *);
+ int ret =
+ LIBC_NAMESPACE::syscall_impl<int>(SYS_ioctl, fd, request, ptr_to_memory);
+ va_end(ptr_to_memory);
+
+ // 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
diff --git a/libc/test/src/sys/CMakeLists.txt b/libc/test/src/sys/CMakeLists.txt
index dc0aa8bf7b75dc1..b58644e60f57872 100644
--- a/libc/test/src/sys/CMakeLists.txt
+++ b/libc/test/src/sys/CMakeLists.txt
@@ -1,4 +1,8 @@
+add_subdirectory(auxv)
+add_subdirectory(epoll)
+add_subdirectory(ioctl)
add_subdirectory(mman)
+add_subdirectory(prctl)
add_subdirectory(random)
add_subdirectory(resource)
add_subdirectory(select)
@@ -8,6 +12,4 @@ add_subdirectory(stat)
add_subdirectory(statvfs)
add_subdirectory(utsname)
add_subdirectory(wait)
-add_subdirectory(prctl)
-add_subdirectory(auxv)
-add_subdirectory(epoll)
+
diff --git a/libc/test/src/sys/ioctl/CMakeLists.txt b/libc/test/src/sys/ioctl/CMakeLists.txt
new file mode 100644
index 000000000000000..b4bbe81c92ff2eb
--- /dev/null
+++ b/libc/test/src/sys/ioctl/CMakeLists.txt
@@ -0,0 +1,3 @@
+if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${LIBC_TARGET_OS})
+ add_subdirectory(${LIBC_TARGET_OS})
+endif()
diff --git a/libc/test/src/sys/ioctl/linux/CMakeLists.txt b/libc/test/src/sys/ioctl/linux/CMakeLists.txt
new file mode 100644
index 000000000000000..aa1bffec16bd672
--- /dev/null
+++ b/libc/test/src/sys/ioctl/linux/CMakeLists.txt
@@ -0,0 +1,13 @@
+add_custom_target(libc_sys_ioctl_unittests)
+
+add_libc_unittest(
+ ioctl_test
+ SUITE
+ libc_sys_ioctl_unittests
+ SRCS
+ ioctl_test.cpp
+ DEPENDS
+ libc.src.sys.ioctl.ioctl
+ libc.src.errno.errno
+)
+
diff --git a/libc/test/src/sys/ioctl/linux/ioctl_test.cpp b/libc/test/src/sys/ioctl/linux/ioctl_test.cpp
new file mode 100644
index 000000000000000..4fc76d1b3c1842c
--- /dev/null
+++ b/libc/test/src/sys/ioctl/linux/ioctl_test.cpp
@@ -0,0 +1,27 @@
+//===-- Unittests for ioctl -----------------------------------------------===//
+//
+// 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/sys/ioctl/ioctl.h"
+#include "test/UnitTest/ErrnoSetterMatcher.h"
+#include "test/UnitTest/LibcTest.h"
+#include "test/UnitTest/Test.h"
+
+#include <sys/syscall.h>
+#include <unistd.h>
+
+using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
+using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
+
+TEST(LlvmLibcIoctlTest, InvalidFileDescriptor) {
+ int fd = 10;
+ unsigned long request = 10;
+ int res = LIBC_NAMESPACE::ioctl(fd, request, NULL);
+ EXPECT_THAT(res, Fails(EBADF, -1));
+}
|
@nickdesaulniers nick |
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.
Nick's busy for a couple weeks so I'll be reviewing this patch.
libc/src/sys/ioctl/linux/ioctl.cpp
Outdated
namespace LIBC_NAMESPACE { | ||
|
||
// This function is currently linux only. It has to be refactored suitably if | ||
// madvise is to be supported on non-linux operating systems also. |
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.
nit: update comment to refer to the correct function instead of madvise
.
TEST(LlvmLibcIoctlTest, InvalidFileDescriptor) { | ||
int fd = 10; | ||
unsigned long request = 10; | ||
int res = LIBC_NAMESPACE::ioctl(fd, request, NULL); |
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 test that succeeds as well.
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.
@michaelrj-google do you have an idea what test can I use to always guarantee success ? From the other PR @nickdesaulniers nick
told me I dont need to test for this syscall because it is a linux syscall, and the current test file is fine. I tried a test that I expect to succeeds in the PR but that seems to fail in the CI.
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.
Perhaps FS_IOC_GETFLAGS
.
https://man7.org/linux/man-pages/man2/ioctl_iflags.2.html
That is used as the example for ioctl in Michael Kerrisk's The Linux Programming Interface page 308.
int fd = open("/dev/null", O_RDWR); | ||
int data; | ||
int res = LIBC_NAMESPACE::ioctl(fd, FIONREAD, &data); | ||
EXPECT_THAT(res, Succeeds()); |
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.
From the presubmit build failure:
ioctl_test.cpp:35: FAILURE
Failed to match res against Succeeds().
Expected return value to be equal to 0 but got -1.
Expected errno to be equal to "Success" but got "Inappropriate ioctl for device".
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 applied FS_IOC_GETFLAGS
but it seems the same error is still there
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.
it looks like the error In appropriate ioctl for device
is for the case the test wants to connect to terminal but there is no terminal on the CI.
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.
This is perhaps related to the usage of /dev/null
as the input file. Perhaps we need to create a new file and run the test on that.
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.
$ lsattr /dev/null
lsattr: Operation not supported While reading flags on /dev/null
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.
See how libc/test/src/stdio/ tests create temp files.
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.
looks like it works now
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.
make sure to close the file
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.
@nickdesaulniers nick
do u know why I am getting error undefined symbol to libc_namspace::open() and close()?
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.
Is that a linkage failure? Then that would be a missing dependency in cmake for the test.
Is this ready for re-review, or? |
Yes, this is ready for review now! Thanks. |
@nickdesaulniers Hi, any update on this? Also can you help me confirm with the full-build mode? My laptop (mac m1) has issues with full-build mode, thanks! |
LIBC_NAMESPACE::libc_errno = 0; | ||
constexpr const char *FILENAME = "ioctl.test"; | ||
auto TEST_FILE = libc_make_test_file_path(FILENAME); | ||
int fd = LIBC_NAMESPACE::open(TEST_FILE, O_WRONLY | O_CREAT, S_IRWXU); |
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.
/android0/llvm-project/libc/test/src/sys/ioctl/linux/ioctl_test.cpp:33:64: error: use of undeclared identifier 'S_IRWXU'
33 | int fd = LIBC_NAMESPACE::open(TEST_FILE, O_WRONLY | O_CREAT, S_IRWXU);
| ^
need to include sys/stat.h
, though perhaps we should have a proxy header (akin to libc/hdr/sys_epoll_macros.h) for that.
@changkhothuychung and @SchrodingerZhu, are you planning to finish this PR? |
Fix #85275