[libc] implement ioctl #85890

changkhothuychung · 2024-03-20T02:39:18Z

This PR is to work on the issue #85275

llvmbot · 2024-03-20T02:39:47Z

@llvm/pr-subscribers-libc

Author: Nhat Nguyen (changkhothuychung)

Changes

This PR is to work on the issue #85275

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

11 Files Affected:

(modified) libc/config/linux/aarch64/entrypoints.txt (+3)
(modified) libc/config/linux/riscv/entrypoints.txt (+3)
(modified) libc/config/linux/x86_64/entrypoints.txt (+3)
(modified) libc/spec/linux.td (+18)
(added) libc/src/sys/ioctl/CMakeLists.txt (+12)
(added) libc/src/sys/ioctl/ioctl.h (+20)
(added) libc/src/sys/ioctl/linux/CMakeLists.txt (+13)
(added) libc/src/sys/ioctl/linux/ioctl.cpp (+39)
(added) libc/test/src/sys/ioctl/CMakeLists.txt (+3)
(added) libc/test/src/sys/ioctl/linux/CMakeLists.txt (+14)
(added) libc/test/src/sys/ioctl/linux/ioctl_test.cpp (+29)

diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index 52e9e8036a3fae..4c8ba6b169d2de 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -130,6 +130,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 52ca12121812d1..64a2732dde5b7a 100644
--- a/libc/config/linux/riscv/entrypoints.txt
+++ b/libc/config/linux/riscv/entrypoints.txt
@@ -136,6 +136,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 ab105e1d634472..fbfb3683197600 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -136,6 +136,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/spec/linux.td b/libc/spec/linux.td
index ba5f99c12ecd11..1c6ada3ac35830 100644
--- a/libc/spec/linux.td
+++ b/libc/spec/linux.td
@@ -74,6 +74,24 @@ def Linux : StandardSpec<"Linux"> {
       []  // Functions
   >;
 
+  HeaderSpec SysMMan = 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/ioctl/CMakeLists.txt b/libc/src/sys/ioctl/CMakeLists.txt
new file mode 100644
index 00000000000000..4b50c278c7871a
--- /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 00000000000000..660505ace4d13e
--- /dev/null
+++ b/libc/src/sys/ioctl/ioctl.h
@@ -0,0 +1,20 @@
+//===-- 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
+
+#include <sys/mman.h> // For size_t and off_t
+
+namespace LIBC_NAMESPACE {
+
+int ioctl(int fd, unsigned long request, ...);
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC_SYS_MMAN_MMAP_H
diff --git a/libc/src/sys/ioctl/linux/CMakeLists.txt b/libc/src/sys/ioctl/linux/CMakeLists.txt
new file mode 100644
index 00000000000000..8a23505d4e9d19
--- /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 00000000000000..6df254e4fea8cf
--- /dev/null
+++ b/libc/src/sys/ioctl/linux/ioctl.cpp
@@ -0,0 +1,39 @@
+//===---------- Linux implementation of the POSIX madvise 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 ptrToMemory;
+  va_start(ptrToMemory, 1);
+  va_arg(ptrToMemory, void *) int ret =
+      LIBC_NAMESPACE::syscall_impl<int>(SYS_ioctl, fd, request, ptrToMemory);
+  va_end(ptrToMemory);
+
+  // 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/ioctl/CMakeLists.txt b/libc/test/src/sys/ioctl/CMakeLists.txt
new file mode 100644
index 00000000000000..b4bbe81c92ff2e
--- /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 00000000000000..d1defd3b031be6
--- /dev/null
+++ b/libc/test/src/sys/ioctl/linux/CMakeLists.txt
@@ -0,0 +1,14 @@
+add_custom_target(libc_sys_mman_unittests)
+
+add_libc_unittest(
+  ioctl_test
+  SUITE
+    libc_sys_mman_unittests
+  SRCS
+    ioctl_test.cpp
+  DEPENDS
+    libc.include.sys_ioctl
+    libc.src.errno.errno
+    libc.test.errno_setter_matcher
+)
+
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 00000000000000..d53510294ccce1
--- /dev/null
+++ b/libc/test/src/sys/ioctl/linux/ioctl_test.cpp
@@ -0,0 +1,29 @@
+//===-- 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 "src/unistd/sysconf.h"
+#include "test/UnitTest/ErrnoSetterMatcher.h"
+#include "test/UnitTest/LibcTest.h"
+#include "test/UnitTest/Test.h"
+
+#include <sys/mman.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, 10, NULL);
+  EXPECT_THAT(res, Fails(EBADF, -1));
+}
\ No newline at end of file

nickdesaulniers · 2024-03-22T22:56:43Z

Thanks for the patch! I'll probably get around to reviewing this next week!

SchrodingerZhu

Thanks for the patch!

libc/spec/linux.td

libc/src/sys/ioctl/ioctl.h

libc/src/sys/ioctl/linux/ioctl.cpp

libc/test/src/sys/ioctl/linux/CMakeLists.txt

libc/test/src/sys/ioctl/linux/ioctl_test.cpp

libc/src/sys/ioctl/CMakeLists.txt

changkhothuychung · 2024-03-24T19:26:11Z

@nickdesaulniers nick
I am not really familiar with how to write test case for this syscall, do you have any recommnedations which case should I cover for unit test?

github-actions · 2024-03-24T19:27:34Z

✅ With the latest revision this PR passed the Python code formatter.

github-actions · 2024-03-24T19:27:34Z

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

libc/src/sys/ioctl/linux/ioctl.cpp

nickdesaulniers · 2024-03-25T15:51:38Z

@nickdesaulniers nick I am not really familiar with how to write test case for this syscall, do you have any recommnedations which case should I cover for unit test?

I think what you have is fine for now.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/adding-syscalls.rst#n186 for linux suggests that a "selftest" should be written (in the Linux kernel sources) to test these interfaces. At some point, we'll look into getting those building against llvm-libc. But @kaladron suggests we avoid testing kernel functionality.

…m-project into ioctl-impl

libc/src/sys/ioctl/linux/ioctl.cpp

libc/test/src/sys/ioctl/linux/ioctl_test.cpp

nickdesaulniers

Thanks for the patch!

changkhothuychung · 2024-03-30T18:15:06Z

@SchrodingerZhu can you take a look when you have a chance and approve? thanks.

SchrodingerZhu

LTGM!

(and sorry for the delay)

nickdesaulniers · 2024-04-04T18:37:42Z

@changkhothuychung do you need us to land this for you?

changkhothuychung · 2024-04-04T22:05:28Z

@changkhothuychung do you need us to land this for you?

it looks like I cannot merge by myself, can you help me merge? thanks.

This reverts commit 289a2c3.

Reverts #85890 This fails in full build mode: https://lab.llvm.org/buildbot/#/builders/163/builds/54478/steps/4/logs/stdio

lntue · 2024-04-10T03:59:29Z

This PR was failed to build:
https://lab.llvm.org/buildbot/#/builders/78/builds/68827/steps/4/logs/stdio
https://lab.llvm.org/buildbot/#/builders/163/builds/54479/steps/4/logs/stdio

nickdesaulniers · 2024-04-10T19:45:09Z

Here are the changes I needed to make to make this compile and run successfully.

diff --git a/libc/src/sys/ioctl/linux/ioctl.cpp b/libc/src/sys/ioctl/linux/ioctl.cpp
index 6c8ff54dc2ae..f2354bcabb9c 100644
--- a/libc/src/sys/ioctl/linux/ioctl.cpp
+++ b/libc/src/sys/ioctl/linux/ioctl.cpp
@@ -20,10 +20,10 @@ namespace LIBC_NAMESPACE {
 // 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, 1);
-  va_arg(ptr_to_memory, void *) int ret =
-      LIBC_NAMESPACE::syscall_impl<int>(SYS_ioctl, fd, request, ptr_to_memory);
+  va_start(ptr_to_memory, request);
+  va_arg(ptr_to_memory, void *);
   va_end(ptr_to_memory);
+  int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_ioctl, fd, request, ptr_to_memory);
 
   // A negative return value indicates an error with the magnitude of the
   // value being the error code.
diff --git a/libc/test/src/sys/CMakeLists.txt b/libc/test/src/sys/CMakeLists.txt
index dc0aa8bf7b75..e80936514fc4 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,3 @@ 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/linux/CMakeLists.txt b/libc/test/src/sys/ioctl/linux/CMakeLists.txt
index 93e68975c4e1..aa1bffec16bd 100644
--- a/libc/test/src/sys/ioctl/linux/CMakeLists.txt
+++ b/libc/test/src/sys/ioctl/linux/CMakeLists.txt
@@ -7,8 +7,7 @@ add_libc_unittest(
   SRCS
     ioctl_test.cpp
   DEPENDS
-    libc.include.sys_ioctl
+    libc.src.sys.ioctl.ioctl
     libc.src.errno.errno
-    libc.test.errno_setter_matcher
 )
 
diff --git a/libc/test/src/sys/ioctl/linux/ioctl_test.cpp b/libc/test/src/sys/ioctl/linux/ioctl_test.cpp
index 3de3eff3e8d4..4fc76d1b3c18 100644
--- a/libc/test/src/sys/ioctl/linux/ioctl_test.cpp
+++ b/libc/test/src/sys/ioctl/linux/ioctl_test.cpp
@@ -22,6 +22,6 @@ using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
 TEST(LlvmLibcIoctlTest, InvalidFileDescriptor) {
   int fd = 10;
   unsigned long request = 10;
-  int res = LIBC_NAMESPACE::ioctl(fd, 10, NULL);
+  int res = LIBC_NAMESPACE::ioctl(fd, request, NULL);
   EXPECT_THAT(res, Fails(EBADF, -1));
 }

Please open a new PR with:

git revert 3c2feab7d152b7f161b4adaecef4ec81f038a23e. Use "Reland: [libc] implement ioctl ([libc] implement ioctl #85890)" as the commit one line description and when opening the PR.
add the changes I described above on top as a distinct commit, which will make it easier for reviewers to see what's new. Use git clang-format HEAD~ to reformat it (since I did not)

Push that for review and tag us.

nickdesaulniers · 2024-04-10T19:46:10Z

Also, it's still not clear to me how presubmit appears to have run successfully. That's...concerning...

changkhothuychung · 2024-04-12T04:39:30Z

@nickdesaulniers thanks so much for the diff file, I will do it as soon as I can.

initial commit

b12e5c2

llvmbot added the libc label Mar 20, 2024

nickdesaulniers self-requested a review March 22, 2024 22:54

SchrodingerZhu requested changes Mar 22, 2024

View reviewed changes

nickdesaulniers reviewed Mar 22, 2024

View reviewed changes

libc/src/sys/ioctl/CMakeLists.txt Show resolved Hide resolved

changkhothuychung added 2 commits March 24, 2024 15:18

address comments

c771c27

Merge branch 'main' into ioctl-impl

8babea1

SchrodingerZhu self-requested a review March 24, 2024 19:54

SchrodingerZhu reviewed Mar 24, 2024

View reviewed changes

libc/src/sys/ioctl/linux/ioctl.cpp Outdated Show resolved Hide resolved

changkhothuychung added 2 commits March 26, 2024 00:29

snake case

a9bdedd

Merge branch 'ioctl-impl' of https://github.com/changkhothuychung/llv…

a06169b

…m-project into ioctl-impl

nickdesaulniers reviewed Mar 26, 2024

View reviewed changes

libc/src/sys/ioctl/linux/ioctl.cpp Outdated Show resolved Hide resolved

nickdesaulniers reviewed Mar 26, 2024

View reviewed changes

libc/test/src/sys/ioctl/linux/ioctl_test.cpp Outdated Show resolved Hide resolved

libc/test/src/sys/ioctl/linux/ioctl_test.cpp Outdated Show resolved Hide resolved

libc/test/src/sys/ioctl/linux/ioctl_test.cpp Outdated Show resolved Hide resolved

changkhothuychung added 3 commits March 27, 2024 20:22

some changes

c540adf

remove includes

a73265f

code formatter

475af7d

changkhothuychung requested review from nickdesaulniers and SchrodingerZhu March 28, 2024 03:12

nickdesaulniers approved these changes Mar 28, 2024

View reviewed changes

SchrodingerZhu approved these changes Apr 4, 2024

View reviewed changes

lntue merged commit 289a2c3 into llvm:main Apr 10, 2024

lntue added a commit that referenced this pull request Apr 10, 2024

Revert "[libc] implement ioctl (#85890)"

434bbc9

This reverts commit 289a2c3.

lntue mentioned this pull request Apr 10, 2024

Revert "[libc] implement ioctl" #88226

Merged

lntue added a commit that referenced this pull request Apr 10, 2024

Revert "[libc] implement ioctl" (#88226)

3c2feab

Reverts #85890 This fails in full build mode: https://lab.llvm.org/buildbot/#/builders/163/builds/54478/steps/4/logs/stdio

[libc] implement ioctl #85890

[libc] implement ioctl #85890

Uh oh!

Conversation

changkhothuychung commented Mar 20, 2024

Uh oh!

llvmbot commented Mar 20, 2024

Uh oh!

nickdesaulniers commented Mar 22, 2024

Uh oh!

SchrodingerZhu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

changkhothuychung commented Mar 24, 2024

Uh oh!

github-actions bot commented Mar 24, 2024

Uh oh!

github-actions bot commented Mar 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nickdesaulniers commented Mar 25, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickdesaulniers left a comment

Choose a reason for hiding this comment

Uh oh!

changkhothuychung commented Mar 30, 2024

Uh oh!

SchrodingerZhu left a comment

Choose a reason for hiding this comment

Uh oh!

nickdesaulniers commented Apr 4, 2024

Uh oh!

changkhothuychung commented Apr 4, 2024

Uh oh!

lntue commented Apr 10, 2024

Uh oh!

nickdesaulniers commented Apr 10, 2024

Uh oh!

nickdesaulniers commented Apr 10, 2024

Uh oh!

changkhothuychung commented Apr 12, 2024

Uh oh!

Uh oh!

github-actions bot commented Mar 24, 2024 •

edited

Loading