[libc] Add very basic stdio FILE and fwrite

[abrachet]

Summary

This patch adds a very basic FILE type and basic fwrite.

It also removes snprintf from StdIO's function spec because VarArgType was causing the generation to fail.

[sivachandra]

I have been busy looking into the ARM math test setup. I will review this before the end of the week.

[sivachandra]

Did a highlevel pass and dropped a couple of questions. Will do a more fine grained review later in the day.

libc/src/stdio/fwrite.h

18

Asking for my own knowledge: when/where would an internal unlock version be useful?

libc/test/config/linux/x86_64/syscall_test.cpp

12 ↗

(On Diff #255574)

Does it make sense to move it utils/CPP? It is incomplete right now, but may be utils/CPP is a more appropriate place.

[sivachandra]

libc/config/linux/api.td

23

I understand all public API deals with FILE*. But, would an empty struct trigger an incorrect optimization in user code somewhere? I have no idea, but just checking since such a thought comes to my mind.

165

I added this as an example long time back and never checked if it works presently.

libc/src/stdio/FILE.h

21

May be this struct is scalable. As in, this structure can handle all kinds of FILE objects. But, few questions below.

24

For a simple use like this, a name like cookie seems alright. But, would it make sense to call it write_ptr?

25

Should we actually specify the type of the writing function? Let it be set by the FILE creation function to the appropriate writer type?

libc/test/config/linux/x86_64/syscall_test.cpp

1 ↗

(On Diff #255574)

Can you move the changes in this file to a separate patch?

[abrachet]

libc/src/stdio/FILE.h

21

Yes I think we could have internal types which inherit from this base __llvm_libc::FILE.

24

I think the name isn't great either but its from [[ http://man7.org/linux/man-pages/man3/fopencookie.3.html | fopencookie ]] which is a GNU extension, I think is worthwhile to support. Wether we do ever export fopencookie or not it is a good internal scheme.

25

What do you mean? Templatize it for internal use?

libc/src/stdio/fwrite.h

18

Possibly in functions like printf which might have separated writes but the stream would need to remain locked to make sure it is written continuously. This may not be ever called internally though. Should I remove its prototype from the internal header?

libc/test/config/linux/x86_64/syscall_test.cpp

12 ↗

(On Diff #255574)

Possibly, I just have this here for the type checking. All it really needs is an operator() and it would be pretty similar to std::function, so we could put it there.

Also, while we are talking about it. Would utils/CPP be an appropriate place to add an analog to std::unique_lock for internal use around mtx_t?

[abrachet]

• Change FILE's external definition from empty struct to void
• Remove change to syscall_test.cpp

[abrachet]

libc/config/linux/api.td

23

Actually it turns out that empty struct is undefined behavior in C. I've used void here, which will of course fail if someone does FILE f. Which I think is preferable, but we can make it a complete object type like int if thats better.

[MaskRay]

libc/config/linux/api.td

23

You can just use the real underlying type typedef struct _IO_FILE FILE;

It is not required to be complete.

libc/src/stdio/FILE.h

21

FILE needs to be a typedef to a struct.

25

fopencookie is a use case. The type does not need to mention cookie_*.

[abrachet]

• Use incomplete struct FILE as FILE's definition not void
• Remove cookie from write_function_t

[abrachet]

libc/config/linux/api.td

23

I've gone with typedef struct FILE FILE

libc/src/stdio/FILE.h

21

This is an internal (C++) header.

25

cookie_write_function_t is a type which must be exposed for fopencookie. That doesn't mean we need to refer to the internal type by that name though so I have just called it write_function_t

[sivachandra]

libc/src/stdio/FILE.h

17

We probably want to avoid dependence on sys/types.h here to keep the definition of FILE platform independent. See my other comment below.

25

I think having the cookie pointer is also a bit confusing. For example, it feels like you are saying all FILE objects should follow the cookie protocol (so to say). What do you think about this:

struct FILE {
  lock
  functions for file operations on FILE* objects
};

For each specific kind of stream we will have a derived class of FILE which will contain stream specific data (For example, cookie in your case). Also, the stream creation functions like fopen will install the appropriate file operation functions in the FILE object. An operation function, say seek, will look like:

class MyFileType : public File {
   ....
};

int seek(FILE *f, long offset, ...) {
  // This seek function is specific to MyFileType objects so the static down cast is safe.
  MyFileType *myFile = static_cast<MyFileType *>(f):
  ...
}

As for fopencookie, I think you can wrap the cookie_io_functions_t functions in other functions which match the signature in the base struct FILE and avoid any type mismatch issues.

[abrachet]

Remove cookie from the base FILE type

[abrachet]

libc/src/stdio/FILE.h

17

ssize_t is what cookie_write_function_t returns and this is defined in sys/types.h. The idea is that write(2) returns that I believe. I've removed that TODO for now.

25

This is what I was planning, so you're right cookie isn't necessary in the base FILE type. Also it makes more sense for the functions to take FILE I was thinking I would need to put that in the cookie which would have been a pain. Much better :)

[sivachandra]

libc/src/stdio/FILE.h

17

SG. AFAIK, write is not a std C function so handling all such functions outside of this common struct SGTM.

libc/src/stdio/fwrite.h

18

Sorry, I missed this previously. Going by the spirit of "unless required", probably a good idea to take this and the implementation off?

[abrachet]

Remove fwrite_unlocked from internal header
Internally refer to FILE by its fully qualified name __llvm_libc::FILE NFC

libc/config/linux/api.td

 
 def OffT : TypeDecl<"off_t"> {
   let Decl = [{
     #define __need_off_t
     #include <__posix-types.h>
   }];
 }
 
+def FILE : TypeDecl<"FILE"> {
+  let Decl = [{
+    typedef struct FILE FILE;
+  }];
+}
+
 def AssertMacro : MacroDef<"assert"> {
   let Defn = [{
     #undef assert
 
     #ifdef NDEBUG
     #define assert(e) (void)0
     #else
 
def StringAPI : PublicAPI<"string.h"> {
   let Macros = [
     NullMacro,
   ];
 }
 
 def StdIOAPI : PublicAPI<"stdio.h"> {
   let TypeDeclarations = [
     SizeT,
+    FILE,
   ];
 
   let Functions = [
-    "snprintf",
+    "fwrite",
   ];
 }
 
 def StdlibAPI : PublicAPI<"stdlib.h"> {
   let Functions = [
     "_Exit",
     "abort",
   ];

libc/include/CMakeLists.txt

add_gen_header(
   PARAMS
     platform_signal=../config/${LIBC_TARGET_OS}/signal.h.in
   GEN_HDR signal.h
   DATA_FILES
     ../config/${LIBC_TARGET_OS}/signal.h.in
 )
 
 add_gen_header(
+  stdio
+  DEF_FILE stdio.h.def
+  GEN_HDR stdio.h
+  DEPENDS
+    llvm_libc_common_h
+)
+
+add_gen_header(
   stdlib
   DEF_FILE stdlib.h.def
   GEN_HDR stdlib.h
   DEPENDS
     .llvm_libc_common_h
 )
 
 # TODO: Not all platforms will have a include/sys directory. Add the sys

libc/include/stdio.h.def

+//===-- C standard library header stdio.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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_STDIO_H
+#define LLVM_LIBC_STDIO_H
+
+#include <__llvm-libc-common.h>
+
+%%public_api()
+
+#endif // LLVM_LIBC_STDIO_H

libc/spec/stdc.td

def StdC : StandardSpec<"stdc"> {
   NamedType ThrdStartTType = NamedType<"thrd_start_t">;
   NamedType ThrdTType = NamedType<"thrd_t">;
   PtrType ThrdTTypePtr = PtrType<ThrdTType>;
 
   PtrType IntPtr = PtrType<IntType>;
 
   NamedType SigHandlerT = NamedType<"__sighandler_t">;
 
+  NamedType FILE = NamedType<"FILE">;
+  PtrType FILEPtr = PtrType<FILE>;
+  RestrictedPtrType FILERestrictedPtr = RestrictedPtrType<FILE>;
+
   HeaderSpec Assert = HeaderSpec<
       "assert.h",
       [
           Macro<"static_assert">,
           Macro<"assert">,
       ],
       [], // Types
       [], // Enumerations
HeaderSpec Math = HeaderSpec<
       ]
   >;
 
   HeaderSpec StdIO = HeaderSpec<
       "stdio.h",
       [], // Macros
       [ // Types
           SizeTType,
+          FILE,
       ],
       [], // Enumerations
       [
           FunctionSpec<
-              "snprintf",
-              RetValSpec<IntType>,
-              [ArgSpec<CharPtr>,
-               ArgSpec<SizeTType>,
-               ArgSpec<ConstCharRestrictedPtr>,
-               ArgSpec<VarArgType>]
+              "fwrite",
+              RetValSpec<SizeTType>,
+              [ArgSpec<ConstVoidRestrictedPtr>,
+              ArgSpec<SizeTType>,
+              ArgSpec<FILERestrictedPtr>]
           >,
       ]
   >;
 
   HeaderSpec StdLib = HeaderSpec<
       "stdlib.h",
       [], // Macros
       [], // Types

libc/src/CMakeLists.txt

 add_subdirectory(assert)
 add_subdirectory(errno)
 add_subdirectory(math)
 add_subdirectory(signal)
+add_subdirectory(stdio)
 add_subdirectory(stdlib)
 add_subdirectory(string)
 # TODO: Add this target conditional to the target OS.
 add_subdirectory(sys)
 add_subdirectory(threads)
 
 add_subdirectory(__support)

libc/src/stdio/CMakeLists.txt

+add_entrypoint_object(
+  fwrite
+  SRCS
+    fwrite.cpp
+  HDRS
+    fwrite.h
+  DEPENDS
+    libc.src.threads.mtx_lock
+    libc.src.threads.mtx_unlock
+)

libc/src/stdio/FILE.h

+//===-- Internal definition of FILE -----------------------------*- 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_FILE_H
+#define LLVM_LIBC_SRC_STDIO_FILE_H
+
+#include "include/threads.h"
+#include <stddef.h>
+
+namespace __llvm_libc {
+
+// TOOD: This should return ssize_t but it's defined in sys/types.h
+using write_function_t = size_t(void *, const char *, size_t);
+
+struct FILE {
+  mtx_t lock;
+
+  void *cookie;
+  write_function_t *write;
+};
+
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_STDIO_FILE_H

libc/src/stdio/fwrite.h

+//===-- Implementation header of fwrite and fwrite_unlocked -----*- 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_FWRITE_H
+#define LLVM_LIBC_SRC_STDIO_FWRITE_H
+
+#include "src/stdio/FILE.h"
+#include <stddef.h>
+
+namespace __llvm_libc {
+
+size_t fwrite_unlocked(const void *__restrict ptr, size_t size, size_t nmeb,
+                       FILE *__restrict stream);
+size_t fwrite(const void *__restrict ptr, size_t size, size_t nmeb,
+              FILE *__restrict stream);
+
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_STDIO_FWRITE_H

libc/src/stdio/fwrite.cpp

+//===-- Implementation of fwrite and fwrite_unlocked ------------*- 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/fwrite.h"
+#include "src/stdio/FILE.h"
+#include "src/threads/mtx_lock.h"
+#include "src/threads/mtx_unlock.h"
+
+namespace __llvm_libc {
+
+size_t fwrite_unlocked(const void *__restrict ptr, size_t size, size_t nmeb,
+                       FILE *__restrict stream) {
+  return stream->write(stream->cookie, reinterpret_cast<const char *>(ptr),
+                       size * nmeb);
+}
+
+size_t fwrite(const void *__restrict ptr, size_t size, size_t nmeb,
+              FILE *__restrict stream) {
+  __llvm_libc::mtx_lock(&stream->lock);
+  size_t written = fwrite_unlocked(ptr, size, nmeb, stream);
+  __llvm_libc::mtx_unlock(&stream->lock);
+  return written;
+}
+
+} // namespace __llvm_libc

libc/test/src/CMakeLists.txt

 add_subdirectory(assert)
 add_subdirectory(errno)
 add_subdirectory(signal)
+add_subdirectory(stdio)
 add_subdirectory(stdlib)
 add_subdirectory(string)
 add_subdirectory(sys)
 add_subdirectory(threads)

libc/test/src/stdio/CMakeLists.txt

+add_libc_testsuite(libc_stdio_unittests)
+
+add_libc_unittest(
+  fwrite_test
+  SUITE
+    libc_stdio_unittests
+  SRCS
+    fwrite_test.cpp
+  DEPENDS
+    libc.src.stdio.fwrite
+    # TODO(sivachandra): remove private dependencies of fwrite
+    libc.src.threads.mtx_lock
+    libc.src.threads.mtx_unlock
+)

libc/test/src/stdio/fwrite_test.cpp

+//===-- Unittests for fwrite ----------------------------------------------===//
+//
+// 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/FILE.h"
+#include "src/stdio/fwrite.h"
+#include "utils/CPP/Array.h"
+#include "utils/UnitTest/Test.h"
+
+TEST(Stdio, FWriteBasic) {
+  __llvm_libc::FILE f;
+  char array[6];
+  f.cookie = array;
+  f.write = +[](void *cookie, const char *ptr, size_t size) {
+    char *buf = reinterpret_cast<char *>(cookie);
+    for (size_t i = 0; i < size; ++i)
+      buf[i] = ptr[i];
+    return size;
+  };
+  EXPECT_EQ(fwrite("hello", 1, 6, &f), 6UL);
+  EXPECT_STREQ(array, "hello");
+}