From de939c6cd80c1e88913f1ef12be11aea501eb89b Mon Sep 17 00:00:00 2001 From: Michael Jones Date: Mon, 13 Mar 2023 16:43:05 -0700 Subject: [PATCH] [libc] enable printf using system FILE 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. Reviewed By: sivachandra, lntue Differential Revision: https://reviews.llvm.org/D146001 --- libc/config/linux/x86_64/entrypoints.txt | 4 +- libc/src/stdio/CMakeLists.txt | 35 +++++++---- libc/src/stdio/fprintf.cpp | 10 ++- libc/src/stdio/printf.cpp | 12 +++- libc/src/stdio/printf_core/CMakeLists.txt | 16 ++--- libc/src/stdio/printf_core/file_writer.cpp | 54 ---------------- libc/src/stdio/printf_core/file_writer.h | 79 ++++++++++++++++++++---- libc/src/stdio/printf_core/vfprintf_internal.cpp | 32 ---------- libc/src/stdio/printf_core/vfprintf_internal.h | 18 +++++- libc/test/src/stdio/CMakeLists.txt | 17 ++++- libc/test/src/stdio/fprintf_test.cpp | 32 +++++++--- 11 files changed, 174 insertions(+), 135 deletions(-) delete mode 100644 libc/src/stdio/printf_core/file_writer.cpp delete mode 100644 libc/src/stdio/printf_core/vfprintf_internal.cpp diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt index b301733..5c0b310 100644 --- a/libc/config/linux/x86_64/entrypoints.txt +++ b/libc/config/linux/x86_64/entrypoints.txt @@ -111,6 +111,8 @@ set(TARGET_LIBC_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 @@ -412,10 +414,8 @@ if(LLVM_LIBC_FULL_BUILD) 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 diff --git a/libc/src/stdio/CMakeLists.txt b/libc/src/stdio/CMakeLists.txt index 5f8d179..7ccbf9a 100644 --- a/libc/src/stdio/CMakeLists.txt +++ b/libc/src/stdio/CMakeLists.txt @@ -480,29 +480,42 @@ add_entrypoint_object( libc.src.stdio.printf_core.writer ) +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 + ) +else() + set(printf_copts "-DLIBC_COPT_PRINTF_USE_SYSTEM_FILE") +endif() + add_entrypoint_object( - fprintf + printf SRCS - fprintf.cpp + printf.cpp HDRS - fprintf.h + printf.h DEPENDS - libc.src.__support.arg_list - libc.src.stdio.printf_core.vfprintf_internal + ${printf_deps} + COMPILE_OPTIONS + ${printf_copts} ) - add_entrypoint_object( - printf + fprintf SRCS - printf.cpp + fprintf.cpp HDRS - printf.h + fprintf.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 + COMPILE_OPTIONS + ${printf_copts} ) add_entrypoint_object( diff --git a/libc/src/stdio/fprintf.cpp b/libc/src/stdio/fprintf.cpp index 796d5b5..da8fabf 100644 --- a/libc/src/stdio/fprintf.cpp +++ b/libc/src/stdio/fprintf.cpp @@ -13,9 +13,16 @@ #include "src/stdio/printf_core/vfprintf_internal.h" #include +#include namespace __llvm_libc { +#ifndef LIBC_COPT_PRINTF_USE_SYSTEM_FILE +using FileT = __llvm_libc::File; +#else // defined(LIBC_COPT_PRINTF_USE_SYSTEM_FILE) +using FileT = ::FILE; +#endif // LIBC_COPT_PRINTF_USE_SYSTEM_FILE + LLVM_LIBC_FUNCTION(int, fprintf, (::FILE *__restrict stream, const char *__restrict format, ...)) { @@ -25,7 +32,8 @@ LLVM_LIBC_FUNCTION(int, fprintf, // and pointer semantics, as well as handling // destruction automatically. va_end(vlist); - int ret_val = printf_core::vfprintf_internal(stream, format, args); + int ret_val = printf_core::vfprintf_internal( + reinterpret_cast(stream), format, args); return ret_val; } diff --git a/libc/src/stdio/printf.cpp b/libc/src/stdio/printf.cpp index 8fd8b9c..ca6f61e 100644 --- a/libc/src/stdio/printf.cpp +++ b/libc/src/stdio/printf.cpp @@ -8,11 +8,18 @@ #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 +#include + +#ifndef LIBC_COPT_PRINTF_USE_SYSTEM_FILE +#include "src/__support/File/file.h" +#define PRINTF_STDOUT __llvm_libc::stdout +#else // LIBC_COPT_PRINTF_USE_SYSTEM_FILE +#define PRINTF_STDOUT ::stdout +#endif // LIBC_COPT_PRINTF_USE_SYSTEM_FILE namespace __llvm_libc { @@ -23,8 +30,7 @@ LLVM_LIBC_FUNCTION(int, printf, (const char *__restrict format, ...)) { // 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; } diff --git a/libc/src/stdio/printf_core/CMakeLists.txt b/libc/src/stdio/printf_core/CMakeLists.txt index 31db8ad..1093997 100644 --- a/libc/src/stdio/printf_core/CMakeLists.txt +++ b/libc/src/stdio/printf_core/CMakeLists.txt @@ -116,35 +116,31 @@ add_object_library( 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() -add_object_library( +add_header_library( file_writer - SRCS - file_writer.cpp HDRS file_writer.h DEPENDS + libc.include.stdio libc.src.__support.File.file libc.src.__support.CPP.string_view libc.src.string.memory_utils.memset_implementation .core_structs ) -add_object_library( +add_header_library( vfprintf_internal - SRCS - vfprintf_internal.cpp HDRS vfprintf_internal.h 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 diff --git a/libc/src/stdio/printf_core/file_writer.cpp b/libc/src/stdio/printf_core/file_writer.cpp deleted file mode 100644 index 0e07e1c..0000000 --- a/libc/src/stdio/printf_core/file_writer.cpp +++ /dev/null @@ -1,54 +0,0 @@ -//===-- 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 - -namespace __llvm_libc { -namespace printf_core { - -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(len) || result.has_error()) - written = FILE_WRITE_ERROR; - if (file->error_unlocked()) - written = FILE_STATUS_ERROR; - return written; -} - -int FileWriter::write_str(void *raw_pointer, cpp::string_view new_string) { - FileWriter *file_writer = reinterpret_cast(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(raw_pointer); - constexpr size_t BUFF_SIZE = 8; - char buff[BUFF_SIZE] = {new_char}; - int result; - while (len > BUFF_SIZE) { - result = file_writer->write(buff, BUFF_SIZE); - if (result < 0) - return result; - len -= BUFF_SIZE; - } - return file_writer->write(buff, len); -} - -// TODO(michaelrj): Move this to putc_unlocked once that is available. -int FileWriter::write_char(void *raw_pointer, char new_char) { - FileWriter *file_writer = reinterpret_cast(raw_pointer); - return file_writer->write(&new_char, 1); -} - -} // namespace printf_core -} // namespace __llvm_libc diff --git a/libc/src/stdio/printf_core/file_writer.h b/libc/src/stdio/printf_core/file_writer.h index 6ba1428..0fd6d11 100644 --- a/libc/src/stdio/printf_core/file_writer.h +++ b/libc/src/stdio/printf_core/file_writer.h @@ -11,6 +11,8 @@ #include "src/__support/CPP/string_view.h" #include "src/__support/File/file.h" +#include "src/__support/macros/attributes.h" // For LIBC_INLINE +#include "src/stdio/printf_core/core_structs.h" #include #include @@ -18,26 +20,81 @@ namespace __llvm_libc { namespace printf_core { -class FileWriter { - __llvm_libc::File *file; +template class FileWriter { + file_t *file; public: - FileWriter(::FILE *init_file) { - file = reinterpret_cast<__llvm_libc::File *>(init_file); - file->lock(); - } + LIBC_INLINE FileWriter(file_t *init_file); - ~FileWriter() { file->unlock(); } + LIBC_INLINE ~FileWriter(); - int write(const char *__restrict to_write, size_t len); + LIBC_INLINE 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); + static int write_str(void *raw_pointer, cpp::string_view new_string) { + FileWriter *file_writer = reinterpret_cast(raw_pointer); + return file_writer->write(new_string.data(), new_string.size()); + } + static int write_chars(void *raw_pointer, char new_char, size_t len) { + FileWriter *file_writer = reinterpret_cast(raw_pointer); + constexpr size_t BUFF_SIZE = 8; + char buff[BUFF_SIZE] = {new_char}; + int result; + while (len > BUFF_SIZE) { + result = file_writer->write(buff, BUFF_SIZE); + if (result < 0) + return result; + len -= BUFF_SIZE; + } + return file_writer->write(buff, len); + } + static int write_char(void *raw_pointer, char new_char) { + FileWriter *file_writer = reinterpret_cast(raw_pointer); + return file_writer->write(&new_char, 1); + } }; +// The interface for using our internal file implementation. +template <> +LIBC_INLINE +FileWriter<__llvm_libc::File>::FileWriter(__llvm_libc::File *init_file) { + file = init_file; + file->lock(); +} +template <> LIBC_INLINE FileWriter<__llvm_libc::File>::~FileWriter() { + file->unlock(); +} +template <> +LIBC_INLINE int +FileWriter<__llvm_libc::File>::write(const char *__restrict to_write, + size_t len) { + auto result = file->write_unlocked(to_write, len); + 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; +} + +// The interface for using the system's file implementation. +template <> LIBC_INLINE FileWriter<::FILE>::FileWriter(::FILE *init_file) { + file = init_file; + ::flockfile(file); +} +template <> LIBC_INLINE FileWriter<::FILE>::~FileWriter() { + ::funlockfile(file); +} +template <> +LIBC_INLINE int FileWriter<::FILE>::write(const char *__restrict to_write, + size_t len) { + size_t written = ::fwrite_unlocked(to_write, 1, len, file); + if (written != len || ::ferror_unlocked(file)) + written = FILE_WRITE_ERROR; + return written; +} + } // namespace printf_core } // namespace __llvm_libc diff --git a/libc/src/stdio/printf_core/vfprintf_internal.cpp b/libc/src/stdio/printf_core/vfprintf_internal.cpp deleted file mode 100644 index b25d545..0000000 --- a/libc/src/stdio/printf_core/vfprintf_internal.cpp +++ /dev/null @@ -1,32 +0,0 @@ -//===-- Internal implementation of vfprintf ---------------------*- 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/vfprintf_internal.h" - -#include "src/__support/arg_list.h" -#include "src/stdio/printf_core/file_writer.h" -#include "src/stdio/printf_core/printf_main.h" -#include "src/stdio/printf_core/writer.h" - -#include - -namespace __llvm_libc { -namespace printf_core { - -int vfprintf_internal(::FILE *__restrict stream, const char *__restrict format, - internal::ArgList &args) { - FileWriter file_writer(stream); - printf_core::Writer writer(reinterpret_cast(&file_writer), - printf_core::FileWriter::write_str, - printf_core::FileWriter::write_chars, - printf_core::FileWriter::write_char); - return printf_core::printf_main(&writer, format, args); -} - -} // namespace printf_core -} // namespace __llvm_libc diff --git a/libc/src/stdio/printf_core/vfprintf_internal.h b/libc/src/stdio/printf_core/vfprintf_internal.h index b837ebb..762018f 100644 --- a/libc/src/stdio/printf_core/vfprintf_internal.h +++ b/libc/src/stdio/printf_core/vfprintf_internal.h @@ -9,15 +9,29 @@ #ifndef LLVM_LIBC_SRC_STDIO_PRINTF_CORE_VFPRINTF_INTERNAL_H #define LLVM_LIBC_SRC_STDIO_PRINTF_CORE_VFPRINTF_INTERNAL_H +#include "src/__support/File/file.h" #include "src/__support/arg_list.h" +#include "src/__support/macros/attributes.h" // For LIBC_INLINE +#include "src/stdio/printf_core/file_writer.h" +#include "src/stdio/printf_core/printf_main.h" +#include "src/stdio/printf_core/writer.h" #include namespace __llvm_libc { namespace printf_core { -int vfprintf_internal(::FILE *__restrict stream, const char *__restrict format, - internal::ArgList &args); +template +LIBC_INLINE int vfprintf_internal(file_t *__restrict stream, + const char *__restrict format, + internal::ArgList &args) { + FileWriter file_writer(stream); + Writer writer(reinterpret_cast(&file_writer), + FileWriter::write_str, FileWriter::write_chars, + FileWriter::write_char); + return printf_main(&writer, format, args); +} + } // namespace printf_core } // namespace __llvm_libc diff --git a/libc/test/src/stdio/CMakeLists.txt b/libc/test/src/stdio/CMakeLists.txt index 8747f18..a4b5a9b 100644 --- a/libc/test/src/stdio/CMakeLists.txt +++ b/libc/test/src/stdio/CMakeLists.txt @@ -134,6 +134,8 @@ add_libc_unittest( 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 @@ -147,7 +149,20 @@ add_libc_unittest( 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 diff --git a/libc/test/src/stdio/fprintf_test.cpp b/libc/test/src/stdio/fprintf_test.cpp index 286c516..20b3c0f 100644 --- a/libc/test/src/stdio/fprintf_test.cpp +++ b/libc/test/src/stdio/fprintf_test.cpp @@ -6,10 +6,12 @@ // //===----------------------------------------------------------------------===// +#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" @@ -17,9 +19,23 @@ #include +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; @@ -37,31 +53,31 @@ TEST(LlvmLibcFPrintfTest, WriteToFile) { 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); } -- 2.7.4