[libc] Add the implementation of the fdopen function #94186

simonzgx · 2024-06-03T05:36:56Z

Fixes #93711 .
This patch implements the fdopen function. Given that fdopen
internally calls fcntl, the implementation of fcntl has been
moved to the __support/OSUtil, where it serves as an internal public
function.

llvmbot · 2024-06-03T05:37:27Z

@llvm/pr-subscribers-libc

Author: Xu Zhang (simonzgx)

Changes

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

9 Files Affected:

(modified) libc/config/linux/aarch64/entrypoints.txt (+1)
(modified) libc/config/linux/riscv/entrypoints.txt (+1)
(modified) libc/config/linux/x86_64/entrypoints.txt (+1)
(modified) libc/spec/posix.td (+5)
(modified) libc/src/stdio/CMakeLists.txt (+7)
(added) libc/src/stdio/fdopen.h (+20)
(modified) libc/src/stdio/linux/CMakeLists.txt (+14)
(added) libc/src/stdio/linux/fdopen.cpp (+79)
(added) libc/test/src/stdio/fdopen_test.cpp ()

diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index ca0418c3618ae..81d14271547dc 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -201,6 +201,7 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.stdio.snprintf
     libc.src.stdio.vsprintf
     libc.src.stdio.vsnprintf
+    libc.src.stdio.fdopen
     #libc.src.stdio.sscanf
     #libc.src.stdio.scanf
     #libc.src.stdio.fscanf
diff --git a/libc/config/linux/riscv/entrypoints.txt b/libc/config/linux/riscv/entrypoints.txt
index 479af40b5b26b..fc5300f9be2d0 100644
--- a/libc/config/linux/riscv/entrypoints.txt
+++ b/libc/config/linux/riscv/entrypoints.txt
@@ -209,6 +209,7 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.stdio.sscanf
     libc.src.stdio.scanf
     libc.src.stdio.fscanf
+    libc.src.stdio.fdopen
 
     # sys/mman.h entrypoints
     libc.src.sys.mman.madvise
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 699bb9dcdf3c9..082dfe7ac5f73 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -214,6 +214,7 @@ set(TARGET_LIBC_ENTRYPOINTS
     libc.src.stdio.scanf
     libc.src.stdio.fscanf
     libc.src.stdio.fileno
+    libc.src.stdio.fdopen
 
     # sys/epoll.h entrypoints
     libc.src.sys.epoll.epoll_create
diff --git a/libc/spec/posix.td b/libc/spec/posix.td
index e16353b8142de..78d9d727ac7b4 100644
--- a/libc/spec/posix.td
+++ b/libc/spec/posix.td
@@ -1293,6 +1293,11 @@ def POSIX : StandardSpec<"POSIX"> {
             RetValSpec<IntType>,
             [ArgSpec<FILEPtr>]
           >,
+          FunctionSpec<
+            "fdopen",
+            RetValSpec<FILEPtr>,
+            [ArgSpec<IntType>, ArgSpec<ConstCharPtr>]
+          >,
       ]
   >;
 
diff --git a/libc/src/stdio/CMakeLists.txt b/libc/src/stdio/CMakeLists.txt
index 1056f38fc7513..deb7e5ad942bf 100644
--- a/libc/src/stdio/CMakeLists.txt
+++ b/libc/src/stdio/CMakeLists.txt
@@ -263,6 +263,13 @@ add_entrypoint_object(
     .${LIBC_TARGET_OS}.rename
 )
 
+add_entrypoint_object(
+  fdopen
+  ALIAS
+  DEPENDS
+    .${LIBC_TARGET_OS}.fdopen
+)
+
 # These entrypoints have multiple potential implementations.
 add_stdio_entrypoint_object(feof)
 add_stdio_entrypoint_object(feof_unlocked)
diff --git a/libc/src/stdio/fdopen.h b/libc/src/stdio/fdopen.h
new file mode 100644
index 0000000000000..158a133e7e131
--- /dev/null
+++ b/libc/src/stdio/fdopen.h
@@ -0,0 +1,20 @@
+//===-- Implementation header of open ---------------------------*- 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_STDIO_FDOPEN_H
+#define LLVM_LIBC_SRC_STDIO_FDOPEN_H
+
+#include <stdio.h>
+
+namespace LIBC_NAMESPACE {
+
+FILE *fdopen(int fd, const char *mode);
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC_STDIO_FDOPEN_H
diff --git a/libc/src/stdio/linux/CMakeLists.txt b/libc/src/stdio/linux/CMakeLists.txt
index a08ff0ba4832f..133740fcb41f4 100644
--- a/libc/src/stdio/linux/CMakeLists.txt
+++ b/libc/src/stdio/linux/CMakeLists.txt
@@ -24,3 +24,17 @@ add_entrypoint_object(
     libc.src.__support.OSUtil.osutil
     libc.src.errno.errno
 )
+
+add_entrypoint_object(
+  fdopen
+  SRCS
+    fdopen.cpp
+  HDRS
+    ../fdopen.h
+  DEPENDS
+    libc.include.fcntl
+    libc.include.stdio
+    libc.src.errno.errno
+    libc.src.fcntl.fcntl
+    libc.src.stdio.fseek
+)
\ No newline at end of file
diff --git a/libc/src/stdio/linux/fdopen.cpp b/libc/src/stdio/linux/fdopen.cpp
new file mode 100644
index 0000000000000..91b27a568b0e1
--- /dev/null
+++ b/libc/src/stdio/linux/fdopen.cpp
@@ -0,0 +1,79 @@
+//===-- Implementation of fprintf -------------------------------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/stdio/fdopen.h"
+
+#include "include/llvm-libc-macros/generic-error-number-macros.h"
+#include "include/llvm-libc-macros/linux/fcntl-macros.h"
+#include "src/__support/File/linux/file.h"
+#include "src/errno/libc_errno.h"
+#include "src/fcntl/fcntl.h"
+#include "src/stdio/fseek.h"
+
+#include <stdio.h>
+
+namespace LIBC_NAMESPACE {
+
+LLVM_LIBC_FUNCTION(::FILE *, fdopen, (int fd, const char *mode)) {
+  auto modeflags = LIBC_NAMESPACE::File::mode_flags(mode);
+  if (modeflags == 0) {
+    libc_errno = EINVAL;
+    return nullptr;
+  }
+
+  int fd_flags = LIBC_NAMESPACE::fcntl(fd, F_GETFL);
+  if (fd_flags == -1) {
+    libc_errno = EBADF;
+    return nullptr;
+  }
+
+  using OpenMode = File::OpenMode;
+  if (((fd_flags & O_ACCMODE) == O_RDONLY &&
+       !(modeflags & static_cast<ModeFlags>(OpenMode::READ))) ||
+      ((fd_flags & O_ACCMODE) == O_WRONLY &&
+       !(modeflags & static_cast<ModeFlags>(OpenMode::WRITE)))) {
+    libc_errno = EINVAL;
+    return nullptr;
+  }
+
+  bool do_seek = false;
+  if ((modeflags & static_cast<File::ModeFlags>(File::OpenMode::APPEND)) &&
+      !(fd_flags & O_APPEND)) {
+    do_seek = true;
+    if (LIBC_NAMESPACE::fcntl(fd, F_SETFL, fd_flags | O_APPEND) == -1) {
+      libc_errno = EBADF;
+      return nullptr;
+    }
+  }
+
+  uint8_t *buffer;
+  {
+    AllocChecker ac;
+    buffer = new (ac) uint8_t[File::DEFAULT_BUFFER_SIZE];
+    if (!ac) {
+      libc_errno = ENOMEM;
+      return nullptr;
+    }
+  }
+  AllocChecker ac;
+  auto *linux_file = new (ac)
+      LinuxFile(fd, buffer, File::DEFAULT_BUFFER_SIZE, _IOFBF, true, modeflags);
+  if (!ac) {
+    libc_errno = ENOMEM;
+    return nullptr;
+  }
+  auto *file = reinterpret_cast<::FILE *>(linux_file);
+  if (do_seek && LIBC_NAMESPACE::fseek(file, 0, SEEK_END) != 0) {
+    free(linux_file);
+    return nullptr;
+  }
+
+  return file;
+}
+
+} // namespace LIBC_NAMESPACE
diff --git a/libc/test/src/stdio/fdopen_test.cpp b/libc/test/src/stdio/fdopen_test.cpp
new file mode 100644
index 0000000000000..e69de29bb2d1d

github-actions · 2024-06-03T17:57:52Z

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

simonzgx · 2024-06-05T07:12:44Z

This PR is ready to be reviewed, please hava a look.
cc @michaelrj-google @nickdesaulniers

michaelrj-google

This is a good start, but needs some tweaks before landing

libc/src/stdio/linux/fdopen.cpp

libc/test/src/stdio/fdopen_test.cpp

lntue · 2024-06-06T01:34:51Z

libc/src/stdio/fdopen.h

+#ifndef LLVM_LIBC_SRC_STDIO_FDOPEN_H
+#define LLVM_LIBC_SRC_STDIO_FDOPEN_H
+
+#include <stdio.h>


@michaelrj-google : shall we create an issue to add proxy header for FILE?

michaelrj-google

The code looks good, just a few nits and organizational comments left

libc/src/__support/OSUtil/fcntl.h

libc/src/stdio/linux/CMakeLists.txt

libc/src/stdio/linux/fdopen.cpp

libc/test/src/stdio/fdopen_test.cpp

michaelrj-google

almost done, just one last change and I think this will be good to land

michaelrj-google · 2024-06-07T20:54:30Z

libc/src/__support/File/file.h

 ErrorOr<File *> openfile(const char *path, const char *mode);
+// Create a File object and associate it with a fd.
+ErrorOr<File *> create_file_from_fd(int fd, const char *mode);


this shouldn't be in the top level file.h since it's linux specific. It should be in the linux specific file.h

If so, I hava to include linux/file.h in fdopen.cpp, is that OK?

yes that's fine. Given that this is a linux specific implementation of fdopen I'm okay with it directly including the linux specific file.

simonzgx · 2024-06-13T11:23:41Z

ping

lntue · 2024-06-13T14:47:50Z

libc/src/__support/OSUtil/linux/fcntl.cpp

+    if (retVal == -EINVAL)
+      return LIBC_NAMESPACE::syscall_impl<int>(SYS_fcntl, fd, cmd,
+                                               reinterpret_cast<void *>(arg));
+    if (static_cast<unsigned long>(retVal) <= -4096UL)


-4096UL while left hand side is unsigned?

Michael seems to have asked the same question, see #89507

I'm confused by this logic. If the return result is negative but malformed, should we return errors instead?

also we should use MAX_ERRNO instead of hard-coded value here.

I'm confused by this logic. If the return result is negative but malformed, should we return errors instead?

Frankly, I'm also puzzled by this code. I found the same code in GLIBC, yet I'm also uncertain as to why the GLIBC's authors chose this way. Perhaps we could set it aside for now, and I'll open a new issue to address it after I delve into it.

Since you're only moving the function to different location, it's fine for me to leave it as-is. But please create an issue for further investigation / fixing and add a TODO here. Thanks,

Since you're only moving the function to different location, it's fine for me to leave it as-is. But please create an issue for further investigation / fixing and add a TODO here. Thanks,

Please help assign #95570 to me.

We really should not be copying code from ANY libc which we do not understand.

michaelrj-google

LGTM

simonzgx · 2024-06-14T17:24:11Z

LGTM

@michaelrj-google Could you help merge this patch? Thanks.

michaelrj-google · 2024-06-14T23:21:14Z

Merged. Thanks for the patch!

lntue · 2024-06-15T03:32:09Z

libc/src/__support/OSUtil/linux/CMakeLists.txt

@@ -8,11 +8,17 @@ add_object_library(
  linux_util
  SRCS
    exit.cpp
+    fcntl.cpp


fcntl should be a separate target.

nickdesaulniers · 2024-06-18T15:13:37Z

libc/src/__support/OSUtil/linux/fcntl.cpp

+    flk->l_start = flk64.l_start;
+    flk->l_len = flk64.l_len;


Thank you for this patch. Note though that it did break the 32b ARM build bot:
https://lab.llvm.org/buildbot/#/builders/182/builds/54

/llvm/libc_worker/worker/libc-arm32-debian/libc-arm32-debian-dbg/llvm-project/libc/src/__support/OSUtil/linux/fcntl.cpp:63:26: error: implicit conversion loses integer precision: '__off64_t' (aka 'long long') to '__off_t' (aka 'long') [-Werror,-Wshorten-64-to-32] flk->l_start = flk64.l_start; ~ ~~~~~~^~~~~~~ /llvm/libc_worker/worker/libc-arm32-debian/libc-arm32-debian-dbg/llvm-project/libc/src/__support/OSUtil/linux/fcntl.cpp:64:24: error: implicit conversion loses integer precision: '__off64_t' (aka 'long long') to '__off_t' (aka 'long') [-Werror,-Wshorten-64-to-32] flk->l_len = flk64.l_len; ~ ~~~~~~^~~~~

llvmbot added the libc label Jun 3, 2024

simonzgx marked this pull request as draft June 3, 2024 05:37

simonzgx force-pushed the implement-fdopen branch 2 times, most recently from ac070ea to 6460fba Compare June 3, 2024 17:55

simonzgx mentioned this pull request Jun 3, 2024

[libc] Implement fdopen #93711

Closed

simonzgx force-pushed the implement-fdopen branch from c5550a8 to e06d269 Compare June 4, 2024 15:59

simonzgx added 4 commits June 5, 2024 13:59

[libc] Add the implementation of the fdopen function

15834c5

Update unit test

7eee9b3

Add a blank line

0251116

Update cmake dependency

a926d0f

simonzgx force-pushed the implement-fdopen branch from e06d269 to a926d0f Compare June 5, 2024 06:16

simonzgx marked this pull request as ready for review June 5, 2024 06:17

Remove unused header files

9dd1429

michaelrj-google reviewed Jun 5, 2024

View reviewed changes

lntue reviewed Jun 6, 2024

View reviewed changes

Move implementation of fcntl to OSUtil dir & resolve comments

a535a9d

simonzgx requested a review from michaelrj-google June 7, 2024 17:14

michaelrj-google reviewed Jun 7, 2024

View reviewed changes

simonzgx requested a review from michaelrj-google June 7, 2024 19:08

Move the implementation of 'fdopen' to 'linux/file.cpp'

5f70faa

simonzgx force-pushed the implement-fdopen branch 2 times, most recently from 63cc961 to 8203762 Compare June 7, 2024 19:36

michaelrj-google reviewed Jun 7, 2024

View reviewed changes

Move the declaration of create_file_from_fd to linux/file.h

0f7bdf2

simonzgx force-pushed the implement-fdopen branch from 07c2329 to 0f7bdf2 Compare June 8, 2024 01:36

simonzgx requested a review from michaelrj-google June 8, 2024 15:58

lntue reviewed Jun 13, 2024

View reviewed changes

lntue requested review from SchrodingerZhu June 13, 2024 15:40

michaelrj-google approved these changes Jun 14, 2024

View reviewed changes

michaelrj-google merged commit 0b24b47 into llvm:main Jun 14, 2024
6 checks passed

lntue reviewed Jun 15, 2024

View reviewed changes

jpienaar mentioned this pull request Jun 17, 2024

[mlirc] Add missing extern C #95829

Merged

nickdesaulniers reviewed Jun 18, 2024

View reviewed changes

[libc] Add the implementation of the fdopen function #94186

[libc] Add the implementation of the fdopen function #94186

Uh oh!

Conversation

simonzgx commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jun 3, 2024

Uh oh!

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

Uh oh!

Uh oh!

simonzgx commented Jun 5, 2024

Uh oh!

michaelrj-google left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelrj-google left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelrj-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonzgx commented Jun 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelrj-google left a comment

Choose a reason for hiding this comment

Uh oh!

simonzgx commented Jun 14, 2024

Uh oh!

Uh oh!

michaelrj-google commented Jun 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simonzgx commented Jun 3, 2024 •

edited

Loading

github-actions bot commented Jun 3, 2024 •

edited

Loading