[libc] add printf

[michaelrj]

Summary

This patch adds the entrypoint for printf. With this, building a
"hello world" program with just LLVM-libc is possible.

[sivachandra]

libc/src/stdio/printf.cpp

35

You don't need to cast to FILE * and call ferror. You can directly call the error method.

Also, a use case like this makes me feel that the writer functions should return an int value which can be consumed by printf_main to process the error. That way, the file writer can write_unlocked and error_unlocked under a single lock/unlock.

[michaelrj]

switch to unlocked writes with the lock aquisition happening before the parsing. Additionally track errors better.

[michaelrj]

rebase

[sivachandra]

libc/src/stdio/printf.cpp

33

Nit: There is an RAII file lock:

__llvm_libc::File::FileLock lock(__llvm_libc::stdout);

Couple of things more:

1. It seems to me like this is not the place to do the lock/unlock and call to error_unlocked. It should be managed by the file writer and the errors should be surfaced to printf_main. See another comment below.
2. printf and fprintf should share code in some manner instead of being mostly duplicated.

libc/src/stdio/printf_core/writer.h

41 ↗

(On Diff #435960)

If this method returns an int value, which can be negative on error, printf_main can do an early return as soon as it sees an error. If this is OK, you should do the same with the converter functions. Why is that not a preferred design?

[michaelrj]

move shared fprintf functionality into internal function, as well as other cleanups

libc/src/stdio/printf.cpp

33

For the file lock class, that isn't a public class so I can't access it here.

I've moved some error handling to printf_main.
As for managing the long in the file writer, the problem is that there is no file writer. Since the FILE class already exposes the interface needed, the write_to_file function just uses the raw FILE. While I could create a class just to hold a lock and destroy it on exit, that seems a bit overkill when I can just call the lock and unlock functions by hand.

I've moved the shared printf and fprintf implementation into vfprintf_internal.

libc/src/stdio/printf_core/writer.h

41 ↗

(On Diff #435960)

It's not a preferred design because write may be called multiple times in a single function. Int conversion uses up to 4 calls. Having an if result < 0 after each of those would be very inefficient, and more importantly not particularly helpful. Usually if a write fails, then all subsequent writes will also fail, so I think it's best to not check every write, instead check the status of the writer every loop. I've added a condition in printf_main to do just that.

[sivachandra]

libc/src/stdio/printf.cpp

33

No, that is not an overkill - you are keeping all file operations cohesively managed by the FileWriter.

+1 for the vfprintf_internal refactoring.

libc/src/stdio/printf_core/writer.h

41 ↗

(On Diff #435960)

Normal design practice would be to return as soon as you see an error. We should follow that until we have sufficient evidence that conditionals like that are the source of a performance problem. Even if we can conclude that conditionals like that are a performance problem, a potential solution should investigate other kind of refactorings which reduce conditionals while stopping as soon as an error is detected.

[michaelrj]

I split the file handling rewrite into its own separate patch after addressing the comments (https://reviews.llvm.org/D127773), this is now just adding true printf.

libc/config/linux/x86_64/entrypoints.txt

list(APPEND TARGET_LIBC_ENTRYPOINTS
     libc.src.stdio.fread_unlocked
     libc.src.stdio.fseek
     libc.src.stdio.funlockfile
     libc.src.stdio.fwrite
     libc.src.stdio.fwrite_unlocked
     libc.src.stdio.sprintf
     libc.src.stdio.snprintf
     libc.src.stdio.fprintf
+    libc.src.stdio.printf
     libc.src.stdio.stderr
     libc.src.stdio.stdout
 
     # signal.h entrypoints
     # TODO: Enable signal.h entrypoints after fixing signal.h
     # libc.src.signal.raise
     # libc.src.signal.sigaction
     # libc.src.signal.sigdelset

libc/src/stdio/CMakeLists.txt

add_entrypoint_object(
   SRCS
     fprintf.cpp
   HDRS
     fprintf.h
   DEPENDS
     libc.src.__support.arg_list
     libc.src.stdio.printf_core.vfprintf_internal
 )
+
+
+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
+)

libc/src/stdio/printf.h

+//===-- Implementation header 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDIO_PRINTF_H
+#define LLVM_LIBC_SRC_STDIO_PRINTF_H
+
+#include <stdio.h>
+
+namespace __llvm_libc {
+
+int printf(const char *__restrict format, ...);
+
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_STDIO_PRINTF_H

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>
+
+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);
+  return ret_val;
+}
+
+} // namespace __llvm_libc

libc/test/src/stdio/CMakeLists.txt

add_libc_unittest(
   DEPENDS
     libc.src.stdio.fprintf
     libc.src.stdio.fclose
     libc.src.stdio.ferror
     libc.src.stdio.fopen
     libc.src.stdio.fread
 )
 
+
+add_libc_unittest(
+  printf_test
+  SUITE
+    libc_stdio_unittests
+  SRCS
+    printf_test.cpp
+  DEPENDS
+    libc.src.stdio.printf
+)
+
 add_subdirectory(printf_core)
 
 add_subdirectory(testdata)

libc/test/src/stdio/printf_test.cpp

+//===-- Unittests for printf ---------------------------------------------===//
+//
+// 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 "utils/UnitTest/Test.h"
+
+TEST(LlvmLibcPrintfTest, PrintOut) {
+  int written;
+
+  constexpr char simple[] = "A simple string with no conversions.\n";
+  written = __llvm_libc::printf(simple);
+  EXPECT_EQ(written, static_cast<int>(sizeof(simple) - 1));
+
+  constexpr char numbers[] = "1234567890\n";
+  written = __llvm_libc::printf("%s", numbers);
+  EXPECT_EQ(written, static_cast<int>(sizeof(numbers) - 1));
+
+  constexpr char format_more[] = "%s and more\n";
+  constexpr char short_numbers[] = "1234";
+  written = __llvm_libc::printf(format_more, short_numbers);
+  EXPECT_EQ(written,
+            static_cast<int>(sizeof(format_more) + sizeof(short_numbers) - 4));
+}