Reland: [libc] implement ioctl #85890 #90317

changkhothuychung · 2024-04-27T07:05:17Z

No description provided.

This reverts commit 3c2feab.

llvmbot · 2024-04-27T07:05:49Z

@llvm/pr-subscribers-libc

Author: Nhat Nguyen (changkhothuychung)

Changes

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

13 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)
(modified) libc/src/sys/CMakeLists.txt (+1)
(added) libc/src/sys/ioctl/CMakeLists.txt (+12)
(added) libc/src/sys/ioctl/ioctl.h (+17)
(added) libc/src/sys/ioctl/linux/CMakeLists.txt (+13)
(added) libc/src/sys/ioctl/linux/ioctl.cpp (+39)
(modified) libc/test/src/sys/CMakeLists.txt (+5-3)
(added) libc/test/src/sys/ioctl/CMakeLists.txt (+3)
(added) libc/test/src/sys/ioctl/linux/CMakeLists.txt (+13)
(added) libc/test/src/sys/ioctl/linux/ioctl_test.cpp (+27)

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));
+}

changkhothuychung · 2024-04-27T15:19:31Z

@nickdesaulniers nick
can u take a look when you have time? thanks.

michaelrj-google

Nick's busy for a couple weeks so I'll be reviewing this patch.

michaelrj-google · 2024-05-06T22:36:59Z

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

+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.


nit: update comment to refer to the correct function instead of madvise.

michaelrj-google · 2024-05-06T22:37:41Z

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

+TEST(LlvmLibcIoctlTest, InvalidFileDescriptor) {
+  int fd = 10;
+  unsigned long request = 10;
+  int res = LIBC_NAMESPACE::ioctl(fd, request, NULL);


please add a test that succeeds as well.

@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.

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.

nickdesaulniers · 2024-05-20T18:38:10Z

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

+  int fd = open("/dev/null", O_RDWR);
+  int data;
+  int res = LIBC_NAMESPACE::ioctl(fd, FIONREAD, &data);
+  EXPECT_THAT(res, Succeeds());


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".

I applied FS_IOC_GETFLAGS but it seems the same error is still there

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.

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.

$ lsattr /dev/null lsattr: Operation not supported While reading flags on /dev/null

See how libc/test/src/stdio/ tests create temp files.

looks like it works now

make sure to close the file

@nickdesaulniers nick
do u know why I am getting error undefined symbol to libc_namspace::open() and close()?

Is that a linkage failure? Then that would be a missing dependency in cmake for the test.

…m-project into ioctl_impl

nickdesaulniers · 2024-06-18T22:43:53Z

Is this ready for re-review, or?

changkhothuychung · 2024-06-23T19:21:58Z

Is this ready for re-review, or?

Yes, this is ready for review now! Thanks.

changkhothuychung added 2 commits April 27, 2024 01:33

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

05a886d

This reverts commit 3c2feab.

reland ioctl

e1a93de

llvmbot added the libc label Apr 27, 2024

lntue requested review from SchrodingerZhu and michaelrj-google May 6, 2024 13:22

michaelrj-google reviewed May 6, 2024

View reviewed changes

changkhothuychung added 5 commits May 11, 2024 10:32

add success test

236873d

add lib

4f1c749

syntax fix

1578645

try a new device

e66e13b

missing header

de63af0

nickdesaulniers reviewed May 20, 2024

View reviewed changes

changkhothuychung added 4 commits May 21, 2024 19:23

update test file

c02aab6

header update

8cccec9

missing header

5100e86

chang file

84147f8

changkhothuychung requested a review from nickdesaulniers May 23, 2024 04:10

changkhothuychung added 9 commits June 2, 2024 15:22

close the file

49f745a

some fix

805266f

fix

40b9b29

more fix

d6ce110

close file

7d8d9d7

use internal function

4e07b1c

Merge branch 'main' into ioctl_impl

7b76aeb

add to lib

b784f43

Merge branch 'ioctl_impl' of https://github.com/changkhothuychung/llv…

0bb8dd7

…m-project into ioctl_impl

fix test

fd3bad5

changkhothuychung added 4 commits June 23, 2024 14:39

fix test

0b8f0db

fix text

e5e65f8

remove headers

7b55dee

fix header

82e9533

lntue mentioned this pull request Jul 3, 2024

[libc] Add basic ioctl function #97509

Closed

jhuber6 changed the title ~~Reland: [libc] implement ioctl ([libc] implement ioctl #85890)~~ Reland: [libc] implement ioctl #85890 Jul 3, 2024

izaakschroeder mentioned this pull request Jul 3, 2024

Tracking issue to bootstrap clang with clang's own libc on aarch64 #97191

Open

39 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reland: [libc] implement ioctl #85890 #90317

Reland: [libc] implement ioctl #85890 #90317

changkhothuychung commented Apr 27, 2024

llvmbot commented Apr 27, 2024

changkhothuychung commented Apr 27, 2024

michaelrj-google left a comment

michaelrj-google May 6, 2024

michaelrj-google May 6, 2024

changkhothuychung May 12, 2024

nickdesaulniers May 20, 2024

nickdesaulniers May 20, 2024 •

edited

Loading

changkhothuychung May 22, 2024

changkhothuychung May 22, 2024

nickdesaulniers May 22, 2024

nickdesaulniers May 22, 2024

nickdesaulniers May 22, 2024

changkhothuychung May 23, 2024

nickdesaulniers May 23, 2024

changkhothuychung Jun 2, 2024

nickdesaulniers Jun 18, 2024

nickdesaulniers commented Jun 18, 2024

changkhothuychung commented Jun 23, 2024

Reland: [libc] implement ioctl #85890 #90317

Are you sure you want to change the base?

Reland: [libc] implement ioctl #85890 #90317

Conversation

changkhothuychung commented Apr 27, 2024

llvmbot commented Apr 27, 2024

changkhothuychung commented Apr 27, 2024

michaelrj-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickdesaulniers May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickdesaulniers commented Jun 18, 2024

changkhothuychung commented Jun 23, 2024

nickdesaulniers May 20, 2024 •

edited

Loading