[libc] enable printf using system FILE

[michaelrj]

Summary

The printf and fprintf implementations use our internal implementation
to improve performance when it's available, but this patch enables using
the public FILE API for overlay mode.

[lntue]

libc/src/stdio/printf_core/file_writer.cpp

34

Can this #include be outside of the namespaces?

libc/test/src/stdio/CMakeLists.txt

163

@sivachandra : I would assume the current bazel build is kind of equivalent to overlay mode. So do we need to add this flag to the bazel overlay?

[michaelrj]

libc/test/src/stdio/CMakeLists.txt

163

the current bazel overlay doesn't have stdio yet, so this flag will be added after both this patch and the printf bazel patch (https://reviews.llvm.org/D146100) land.

[michaelrj]

clean up includes to address comments

[sivachandra]

libc/src/stdio/CMakeLists.txt

520

Use a more focused if:

list(APPEND printf_deps libc.src.__support.arg_list libc.src.stdio.printf_core.vfprintf_internal)
if(LLVM_LIBC_FULL_BUILD)
 list(APPEND printf_deps  libc.src.__support.File.file libc.src.__support.File.platform_file libc.src.__support.arg_list)
 set(printf_copts "COMPILE_OPTIONS -D<...>)
endif()

add_entrypoint_object(
  printf
  SRCS
    printf.cpp
  HDRS
    printf.h
  DEPENDS
    ${printf_deps}
  ${printf_copts}
)

libc/src/stdio/printf.cpp

14

s/PUBLIC_FILE/SYSTEM_FILE

32–33

Have you considered making vfprintf_internal a template function which takes a suitable FileWriter object? The goal should be to confine #ifdef to only one place in the code. Since only one of the templates will get instantiated, it will not lead to code size bloat.

libc/test/src/stdio/fprintf_test.cpp

22

s/file_test/printf_test

[michaelrj]

simplify and clarify the conditionals

[michaelrj]

libc/src/stdio/printf.cpp

32–33

vfprintf_internal already doesn't have this #ifdef, this condition only changes where the stdout comes from. I've changed the code to clarify that.

[sivachandra]

libc/src/stdio/printf.cpp

32–33

Sorry, my comment here is more general. Right now, you have a #ifdef conditional here, and two more in file_writer.cpp and file_writer.h. Using appropriate templates, you should be able to limit the #ifdef to just this file.

libc/src/stdio/printf_core/CMakeLists.txt

123

Even this if should be modified like the one in the other file.

[michaelrj]

move vfprintf_internal and file_writer to templates

libc/src/stdio/printf.cpp

32–33

I can't limit it to just this file, since fprintf also needs to select the correct template, but it's just in the entrypoints now.

[sivachandra]

libc/src/stdio/fprintf.cpp

21 ↗

(On Diff #506673)

Nit: Use UpperCase names for internal types.

libc/src/stdio/printf.cpp

19

Make this a macro, say PRINTF_STDOUT. Otherwise, you are adding a new symbol and an indirection.

libc/src/stdio/printf_core/file_writer.h

91

You can call flockfile in the constructor and funlockfile in the destructor, and make this a call to fwrite_unlocked.

[michaelrj]

address comments

libc/config/linux/x86_64/entrypoints.txt

set(TARGET_LIBC_ENTRYPOINTS
     libc.src.stdlib.realloc
     libc.src.stdlib.aligned_alloc
     libc.src.stdlib.free
 
     # stdio.h entrypoints
     libc.src.stdio.remove
     libc.src.stdio.sprintf
     libc.src.stdio.snprintf
+    libc.src.stdio.fprintf
+    libc.src.stdio.printf
 
     # sys/mman.h entrypoints
     libc.src.sys.mman.madvise
     libc.src.sys.mman.mmap
     libc.src.sys.mman.mprotect
     libc.src.sys.mman.munmap
     libc.src.sys.mman.posix_madvise
 
list(APPEND TARGET_LIBC_ENTRYPOINTS
     libc.src.stdio.fopencookie
     libc.src.stdio.fread
     libc.src.stdio.fread_unlocked
     libc.src.stdio.fseek
     libc.src.stdio.ftell
     libc.src.stdio.funlockfile
     libc.src.stdio.fwrite
     libc.src.stdio.fwrite_unlocked
-    libc.src.stdio.fprintf
     libc.src.stdio.getc
     libc.src.stdio.getc_unlocked
-    libc.src.stdio.printf
     libc.src.stdio.sscanf
     libc.src.stdio.scanf
     libc.src.stdio.fscanf
     libc.src.stdio.putc
     libc.src.stdio.putchar
     libc.src.stdio.puts
     libc.src.stdio.setbuf
     libc.src.stdio.setvbuf

libc/src/stdio/CMakeLists.txt

SRCS
     fprintf.cpp
   HDRS
     fprintf.h
   DEPENDS
     libc.src.__support.arg_list
     libc.src.stdio.printf_core.vfprintf_internal
 )
 
+list(APPEND printf_deps 
+      libc.src.__support.arg_list 
+      libc.src.stdio.printf_core.vfprintf_internal
+)
+if(LLVM_LIBC_FULL_BUILD)
+ list(APPEND printf_deps  
+      libc.src.__support.File.file 
+      libc.src.__support.File.platform_file 
+      libc.src.__support.arg_list
+  )
+else()
+ set(printf_copts "-DLIBC_COPT_PRINTF_USE_SYSTEM_FILE")
+endif()
 
 add_entrypoint_object(
   printf
   SRCS
     printf.cpp
   HDRS
     printf.h
   DEPENDS
-    libc.src.__support.File.file
-    libc.src.__support.File.platform_file
-    libc.src.__support.arg_list
-    libc.src.stdio.printf_core.vfprintf_internal
+    ${printf_deps}
+  COMPILE_OPTIONS
+    ${printf_copts}
 )
 
 add_entrypoint_object(
   ftell
   SRCS
     ftell.cpp
   HDRS
     ftell.h
   DEPENDS
     libc.src.errno.errno
     libc.include.stdio

libc/src/stdio/printf.cpp

 //===-- Implementation of printf --------------------------------*- 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/printf.h"
 
-#include "src/__support/File/file.h"
 #include "src/__support/arg_list.h"
 #include "src/stdio/printf_core/vfprintf_internal.h"
 
 #include <stdarg.h>
+#include <stdio.h>
+
+#ifndef LIBC_COPT_PRINTF_USE_SYSTEM_FILE
+#include "src/__support/File/file.h"
+static ::FILE *printf_stdout = reinterpret_cast<::FILE *>(__llvm_libc::stdout);
+#else  // LIBC_COPT_PRINTF_USE_SYSTEM_FILE
+static ::FILE *printf_stdout = ::stdout;
+#endif // LIBC_COPT_PRINTF_USE_SYSTEM_FILE
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(int, printf, (const char *__restrict format, ...)) {
   va_list vlist;
   va_start(vlist, format);
   internal::ArgList args(vlist); // This holder class allows for easier copying
                                  // and pointer semantics, as well as handling
                                  // destruction automatically.
   va_end(vlist);
-  int ret_val = printf_core::vfprintf_internal(
-      reinterpret_cast<::FILE *>(__llvm_libc::stdout), format, args);
+  int ret_val = printf_core::vfprintf_internal(printf_stdout, format, args);
   return ret_val;
 }
 
 } // namespace __llvm_libc

libc/src/stdio/printf_core/CMakeLists.txt

add_object_library(
   DEPENDS
     .parser
     .converter
     .writer
     .core_structs
     libc.src.__support.arg_list
 )
 
-if(NOT (TARGET libc.src.__support.File.file))
-  # Not all platforms have a file implementation. If file is unvailable,
-  # then we must skip all file based printf sections.
+if(NOT (TARGET libc.src.__support.File.file) AND LLVM_LIBC_FULL_BUILD)
+  # Not all platforms have a file implementation. If file is unvailable, and a 
+  # full build is requested, then we must skip all file based printf sections.
   return()
 endif()
 
+# In full build mode, the file writer will talk directly to the internal file 
+# implementation to improve performance.
+if(LLVM_LIBC_FULL_BUILD)
 add_object_library(
   file_writer
   SRCS
     file_writer.cpp
   HDRS
     file_writer.h
   DEPENDS
     libc.src.__support.File.file
DEPENDS
     libc.include.stdio
     libc.src.__support.File.file
     libc.src.__support.File.platform_file
     libc.src.__support.arg_list
     libc.src.stdio.printf_core.printf_main
     libc.src.stdio.printf_core.file_writer
     libc.src.stdio.printf_core.writer
 )
+else()
+# Else we're in overlay mode so we use the system's FILE. This will require a
+# lock on every write, which is slower.
+add_object_library(
+  file_writer
+  SRCS
+    file_writer.cpp
+  HDRS
+    file_writer.h
+  DEPENDS
+    libc.src.__support.CPP.string_view
+    libc.src.string.memory_utils.memset_implementation
+    .core_structs
+  COMPILE_OPTIONS
+    -DLIBC_COPT_PRINTF_USE_SYSTEM_FILE
+)
+
+add_object_library(
+  vfprintf_internal
+  SRCS
+    vfprintf_internal.cpp
+  HDRS
+    vfprintf_internal.h
+  DEPENDS
+    libc.include.stdio
+    libc.src.__support.arg_list
+    libc.src.stdio.printf_core.printf_main
+    libc.src.stdio.printf_core.file_writer
+    libc.src.stdio.printf_core.writer
+  COMPILE_OPTIONS
+    -DLIBC_COPT_PRINTF_USE_SYSTEM_FILE
+)
+endif()

libc/src/stdio/printf_core/file_writer.h

 #include "src/__support/File/file.h"
 
 #include <stddef.h>
 #include <stdio.h>
 
 namespace __llvm_libc {
 namespace printf_core {
 
+#ifndef LIBC_COPT_PRINTF_USE_SYSTEM_FILE
 class FileWriter {
   __llvm_libc::File *file;
 
 public:
   FileWriter(::FILE *init_file) {
     file = reinterpret_cast<__llvm_libc::File *>(init_file);
     file->lock();
   }
 
   ~FileWriter() { file->unlock(); }
 
   int write(const char *__restrict to_write, size_t len);
 
   // These write functions take a FileWriter as a void* in raw_pointer, and
   // call the appropriate write function on it.
   static int write_str(void *raw_pointer, cpp::string_view new_string);
   static int write_chars(void *raw_pointer, char new_char, size_t len);
   static int write_char(void *raw_pointer, char new_char);
 };
+#else  // defined(LIBC_COPT_PRINTF_USE_SYSTEM_FILE)
+class FileWriter {
+  ::FILE *file;
+
+public:
+  FileWriter(::FILE *init_file) { file = init_file; }
+
+  ~FileWriter() = default;
+
+  int write(const char *__restrict to_write, size_t len);
+
+  // These write functions take a FileWriter as a void* in raw_pointer, and
+  // call the appropriate write function on it.
+  static int write_str(void *raw_pointer, cpp::string_view new_string);
+  static int write_chars(void *raw_pointer, char new_char, size_t len);
+  static int write_char(void *raw_pointer, char new_char);
+};
+#endif // LIBC_COPT_PRINTF_USE_SYSTEM_FILE
 
 } // namespace printf_core
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_STDIO_PRINTF_CORE_FILE_WRITER_H

libc/src/stdio/printf_core/file_writer.cpp

 //===-- FILE Writer implementation for printf -------------------*- 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/printf_core/file_writer.h"
 #include "src/__support/CPP/string_view.h"
 #include "src/__support/File/file.h"
 #include "src/stdio/printf_core/core_structs.h"
 #include <stddef.h>
 
+#ifdef LIBC_COPT_PRINTF_USE_SYSTEM_FILE
+#include <stdio.h>
+#endif // LIBC_COPT_PRINTF_USE_SYSTEM_FILE
+
 namespace __llvm_libc {
 namespace printf_core {
 
+#ifndef LIBC_COPT_PRINTF_USE_SYSTEM_FILE
 int FileWriter::write(const char *__restrict to_write, size_t len) {
   auto result = file->write_unlocked(to_write, len);
-  int written = result.value;
-  if (written != static_cast<int>(len) || result.has_error())
+  size_t written = result.value;
+  if (written != len || result.has_error())
     written = FILE_WRITE_ERROR;
   if (file->error_unlocked())
     written = FILE_STATUS_ERROR;
   return written;
 }
 
+#else // defined(LIBC_COPT_PRINTF_USE_SYSTEM_FILE)
+int FileWriter::write(const char *__restrict to_write, size_t len) {
+  size_t written = ::fwrite(to_write, 1, len, file);
+  if (written != len || ::ferror(file))
+    written = FILE_WRITE_ERROR;
+  return written;
+}
+
+#endif // LIBC_COPT_PRINTF_USE_SYSTEM_FILE
+
 int FileWriter::write_str(void *raw_pointer, cpp::string_view new_string) {
   FileWriter *file_writer = reinterpret_cast<FileWriter *>(raw_pointer);
   return file_writer->write(new_string.data(), new_string.size());
 }
 
 int FileWriter::write_chars(void *raw_pointer, char new_char, size_t len) {
   FileWriter *file_writer = reinterpret_cast<FileWriter *>(raw_pointer);
   constexpr size_t BUFF_SIZE = 8;

libc/test/src/stdio/CMakeLists.txt

add_libc_unittest(
   SUITE
     libc_stdio_unittests
   SRCS
     snprintf_test.cpp
   DEPENDS
     libc.src.stdio.snprintf
 )
 
+# In fullbuild mode, fprintf's tests use the internal FILE for other functions.
+if(LLVM_LIBC_FULL_BUILD)
 add_libc_unittest(
   fprintf_test
   SUITE
     libc_stdio_unittests
   SRCS
     fprintf_test.cpp
   DEPENDS
     libc.src.stdio.fprintf
     libc.src.stdio.fclose
     libc.src.stdio.ferror
     libc.src.stdio.fopen
     libc.src.stdio.fread
 )
-
+else()
+# Else in overlay mode they use the system's FILE.
+add_libc_unittest(
+  fprintf_test
+  SUITE
+    libc_stdio_unittests
+  SRCS
+    fprintf_test.cpp
+  DEPENDS
+    libc.src.stdio.fprintf
+  COMPILE_OPTIONS
+    -DLIBC_COPT_PRINTF_USE_SYSTEM_FILE
+)
+endif()
 
 add_libc_unittest(
   printf_test
   SUITE
     libc_stdio_unittests
   SRCS
     printf_test.cpp
   DEPENDS

libc/test/src/stdio/fprintf_test.cpp

 //===-- Unittests for fprintf ---------------------------------------------===//
 //
 // 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 LIBC_COPT_PRINTF_USE_SYSTEM_FILE
 #include "src/stdio/fclose.h"
 #include "src/stdio/ferror.h"
 #include "src/stdio/fopen.h"
 #include "src/stdio/fread.h"
+#endif // LIBC_COPT_PRINTF_USE_SYSTEM_FILE
 
 #include "src/stdio/fprintf.h"
 
 #include "test/UnitTest/Test.h"
 
 #include <stdio.h>
 
+namespace printf_test {
+#ifndef LIBC_COPT_PRINTF_USE_SYSTEM_FILE
+using __llvm_libc::fclose;
+using __llvm_libc::ferror;
+using __llvm_libc::fopen;
+using __llvm_libc::fread;
+#else  // defined(LIBC_COPT_PRINTF_USE_SYSTEM_FILE)
+using ::fclose;
+using ::ferror;
+using ::fopen;
+using ::fread;
+#endif // LIBC_COPT_PRINTF_USE_SYSTEM_FILE
+} // namespace printf_test
+
 TEST(LlvmLibcFPrintfTest, WriteToFile) {
   constexpr char FILENAME[] = "testdata/fprintf_output.test";
-  ::FILE *file = __llvm_libc::fopen(FILENAME, "w");
+  ::FILE *file = printf_test::fopen(FILENAME, "w");
   ASSERT_FALSE(file == nullptr);
 
   int written;
 
   constexpr char simple[] = "A simple string with no conversions.\n";
   written = __llvm_libc::fprintf(file, simple);
   EXPECT_EQ(written, 37);
 
   constexpr char numbers[] = "1234567890\n";
   written = __llvm_libc::fprintf(file, "%s", numbers);
   EXPECT_EQ(written, 11);
 
   constexpr char format_more[] = "%s and more\n";
   constexpr char short_numbers[] = "1234";
   written = __llvm_libc::fprintf(file, format_more, short_numbers);
   EXPECT_EQ(written, 14);
 
-  ASSERT_EQ(0, __llvm_libc::fclose(file));
+  ASSERT_EQ(0, printf_test::fclose(file));
 
-  file = __llvm_libc::fopen(FILENAME, "r");
+  file = printf_test::fopen(FILENAME, "r");
   ASSERT_FALSE(file == nullptr);
 
   char data[50];
-  ASSERT_EQ(__llvm_libc::fread(data, 1, sizeof(simple) - 1, file),
+  ASSERT_EQ(printf_test::fread(data, 1, sizeof(simple) - 1, file),
             sizeof(simple) - 1);
   data[sizeof(simple) - 1] = '\0';
   ASSERT_STREQ(data, simple);
-  ASSERT_EQ(__llvm_libc::fread(data, 1, sizeof(numbers) - 1, file),
+  ASSERT_EQ(printf_test::fread(data, 1, sizeof(numbers) - 1, file),
             sizeof(numbers) - 1);
   data[sizeof(numbers) - 1] = '\0';
   ASSERT_STREQ(data, numbers);
-  ASSERT_EQ(__llvm_libc::fread(
+  ASSERT_EQ(printf_test::fread(
                 data, 1, sizeof(format_more) + sizeof(short_numbers) - 4, file),
             sizeof(format_more) + sizeof(short_numbers) - 4);
   data[sizeof(format_more) + sizeof(short_numbers) - 4] = '\0';
   ASSERT_STREQ(data, "1234 and more\n");
 
-  ASSERT_EQ(__llvm_libc::ferror(file), 0);
+  ASSERT_EQ(printf_test::ferror(file), 0);
 
   written =
       __llvm_libc::fprintf(file, "Writing to a read only file should fail.");
   EXPECT_LT(written, 0);
 
-  ASSERT_EQ(__llvm_libc::fclose(file), 0);
+  ASSERT_EQ(printf_test::fclose(file), 0);
 }