[libc] add printf writer

[michaelrj]

Summary

The printf implmentation is made up of three main pieces, the parser,
the converter, and the writer. This patch adds the implementation for
the writer, as well as the function for writing to a string, along with
tests.

[lntue]

libc/src/stdio/printf_core/writer.cpp

23

the check chars_written + length >= max_length is redundant. You can move it to an inline comment as reminding.

[michaelrj]

address comment to remove redundant condition

[sivachandra]

libc/src/stdio/printf_core/CMakeLists.txt

28

Indent.

libc/src/stdio/printf_core/string_writer.h

25

It seems to me like you want a stateful writer. So, why not just make it a class?

libc/src/stdio/printf_core/writer.h

29

Is this required only for testing or do you need it for something else also?

31

Can you explain the need for max_length? It seems to me like it is something a specific writer, say StringWriter should care about, but not a generic writer?

At this point, I would actually ask the need for a generic Writer class. It seems to me like all you need is a writer class which implements a method with a specific signature. For example, you want StringWriter like this:

class StringWriter {
private:
  ... // State as suggested in another comment
public:

  // This method attempts to write |len| bytes from |data| and return the actual
  // number of bytes written.
  size_t write(const void *data, size_t len);
};

FWIW, the File class already implements the write protocol. So, based on the printf function, sprintf or fprintf, one either uses the StringWriter or File.

[sivachandra]

libc/src/stdio/printf_core/writer.h

31

Actually, I am beginning think there is benefit with the generic Writer indirection. Consider the File example again - file writer might want to explicitly lock and use unlocked file methods to write to the file. Having a generic writer allows us to hide such details in the file writer.

[michaelrj]

add StringWriter class and adjust tests to match

libc/src/stdio/printf_core/string_writer.h

25

it felt like overkill for tracking only one variable, but if I'm moving max_len in there as well, then it makes more sense.

libc/src/stdio/printf_core/writer.h

29

chars_written is the return value of printf, as well as the value to be written for %n

31

File doesn't support all of the functions I need, specifically it doesn't have chars_written, which represents the number of characters written by this specific call to printf and is used both in the %n conversion and as the return value for all printf functions.

I have moved max_length into the StringWriter class now though.

31

I agree, and theoretically on platforms that don't care as much about code size we could have two string writer versions, one with a max length and one without, since that would improve performance.

[sivachandra]

libc/src/stdio/printf_core/string_writer.h

21

Nit: May be call it available_capacity as that is how it is being used internally?

27

Nit: Why not s/init_output/buffer and call the class member as buffer_ptr?

28

Nit: Space between line 27 and 28.

libc/src/stdio/printf_core/writer.cpp

21

The + operator can potentially lead to a rollover so the check should be done the other way around?

Couple of other points:

1. It seems like all this logic redundant? For, the specific writer will/should handle this anyway?
2. max_length of 0 is also handled by the specific writer anyway?

May be the raw_write should be a "raw" writer anyway. As in, it will just do the writing business and all higher level logic is up to this Writer class.

28

The + operation here can also rollover. Is it OK?

[michaelrj]

fix max_length still being in the core writer class, and the tests not using the max_length functionality in the string writer.

libc/src/stdio/printf_core/string_writer.h

27

I went with cur_buffer since I felt that more accurately described how it was being used.

libc/src/stdio/printf_core/writer.cpp

21

The max length here should have been removed in the previous patch because I moved the max length logic to the string writer class, so yes it was redundant. As for if raw_write should be relatively logic free, I think it makes sense to have the length handled in the string writer, since the max length is only used for strings. Other than that it is relatively raw, only storing a pointer and the number of bytes left to write to it.

28

chars_written is supposed to store the value that will be returned when printf completes. The return value of printf is the number of characters written by that call as represented by an int, or a negative number if there was an error. For this purpose, all chars_written needs to do is be at least as large as an int, and if there would be overflow, return a negative number to represent the error. For this purpose I've changed chars_written to an unsigned long long so that it will definitely be bigger than an int, and the actual printf code can return a negative number if it would overflow that int. Finally, unsigned long long is guaranteed to have a maximum value of at least 18446744073709551615 (2^64 - 1), and if any call to printf writes more than that many bytes I'm declaring it undefined behavior.

[sivachandra]

libc/src/stdio/printf_core/string_writer.h

43

I suppose this method will be used by s*printf functions explicitly?

libc/src/stdio/printf_core/writer.cpp

28

heh!

[michaelrj]

libc/src/stdio/printf_core/string_writer.h

43

yes.

libc/src/stdio/printf_core/CMakeLists.txt

add_object_library(
   DEPENDS
     .core_structs
     libc.src.__support.arg_list
     libc.src.__support.ctype_utils
     libc.src.__support.str_to_integer
     libc.src.__support.CPP.bit
 
 )
+
+add_header_library(
+  string_writer
+  HDRS
+    string_writer.h
+  DEPENDS
+    libc.src.string.memory_utils.memcpy_implementation
+)
+
+add_object_library(
+  writer
+  SRCS
+    writer.cpp
+  HDRS
+    writer.h
+  DEPENDS
+    libc.src.string.memory_utils.memset_implementation
+)

libc/src/stdio/printf_core/string_writer.h

+//===-- String Writer class 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_STDIO_PRINTF_CORE_STRING_WRITER_H
+#define LLVM_LIBC_SRC_STDIO_PRINTF_CORE_STRING_WRITER_H
+
+#include "src/string/memory_utils/memcpy_implementations.h"
+#include <stddef.h>
+
+namespace __llvm_libc {
+namespace printf_core {
+
+class StringWriter {
+  char *__restrict cur_buffer;
+  size_t available_capacity;
+
+public:
+  // StringWriter is intended to take a copy of the cur_buffer pointer, as well
+  // as the maximum length of the string. This maximum length should not include
+  // the null terminator, since that's written separately.
+  StringWriter(char *__restrict buffer, size_t max_len = ~size_t(0))
+      : cur_buffer(buffer), available_capacity(max_len) {}
+
+  void write(const char *__restrict to_write, size_t len) {
+    if (len > available_capacity)
+      len = available_capacity;
+
+    if (len > 0) {
+      inline_memcpy(cur_buffer, to_write, len);
+      cur_buffer += len;
+      available_capacity -= len;
+    }
+  }
+  // Terminate should only be called if the original max length passed to
+  // snprintf was greater than 0. It writes a null byte to the end of the
+  // cur_buffer, regardless of available_capacity.
+  void terminate() { *cur_buffer = '\0'; }
+};
+
+// write_to_string treats raw_pointer as a StringWriter and calls its write
+// function.
+void write_to_string(void *raw_pointer, const char *__restrict to_write,
+                     size_t len) {
+  StringWriter *string_writer = reinterpret_cast<StringWriter *>(raw_pointer);
+  string_writer->write(to_write, len);
+}
+
+} // namespace printf_core
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_STDIO_PRINTF_CORE_STRING_WRITER_H

libc/src/stdio/printf_core/writer.h

-//===-- String writer for printf --------------------------------*- C++ -*-===//
+//===-- Writer definition 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIBC_SRC_STDIO_PRINTF_CORE_WRITER_H
class Writer final {
   // to.
   void *output;
 
   // raw_write is a function that, when called on output with a char* and
   // length, will copy the number of bytes equal to the length from the char*
   // onto the end of output.
   WriteFunc raw_write;
 
-  size_t max_length;
-  size_t chars_written;
+  unsigned long long chars_written = 0;
 
 public:
-  Writer(void *output, WriteFunc raw_write, size_t max_length);
+  Writer(void *init_output, WriteFunc init_raw_write)
+      : output(init_output), raw_write(init_raw_write) {}
 
   // write will copy length bytes from new_string into output using
   // raw_write, unless that would cause more bytes than max_length to be
   // written. It always increments chars_written by length.
   void write(const char *new_string, size_t length);
 
   // write_chars will copy length copies of new_char into output using raw_write
   // unless that would cause more bytes than max_length to be written. It always
   // increments chars_written by length.
   void write_chars(char new_char, size_t length);
 
-  size_t get_chars_written();
+  unsigned long long get_chars_written() { return chars_written; }
 };
 
 } // namespace printf_core
 } // namespace __llvm_libc
 
 #endif // LLVM_LIBC_SRC_STDIO_PRINTF_CORE_WRITER_H

libc/src/stdio/printf_core/writer.cpp

+//===-- Writer definition 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 "writer.h"
+#include "src/string/memory_utils/memset_implementations.h"
+#include <stddef.h>
+
+namespace __llvm_libc {
+namespace printf_core {
+
+void Writer::write(const char *new_string, size_t length) {
+
+  raw_write(output, new_string, length);
+
+  // chars_written tracks the number of chars that would have been written
+  // regardless of what the raw_write call does.
+  chars_written += length;
+}
+
+void Writer::write_chars(char new_char, size_t length) {
+  constexpr size_t BUFF_SIZE = 8;
+  char buff[BUFF_SIZE];
+  inline_memset(buff, new_char, BUFF_SIZE);
+  while (length > BUFF_SIZE) {
+    write(buff, BUFF_SIZE);
+    length -= BUFF_SIZE;
+  }
+  write(buff, length);
+}
+
+} // namespace printf_core
+} // namespace __llvm_libc

libc/test/src/stdio/printf_core/CMakeLists.txt

 add_libc_unittest(
   parser_test
   SUITE
     libc_stdio_unittests
   SRCS
     parser_test.cpp
   DEPENDS
     libc.src.stdio.printf_core.parser
     libc.src.__support.arg_list
 )
 
 target_link_libraries(libc.test.src.stdio.printf_core.parser_test PRIVATE LibcPrintfHelpers)
+
+add_libc_unittest(
+  string_writer_test
+  SUITE
+    libc_stdio_unittests
+  SRCS
+    string_writer_test.cpp
+  DEPENDS
+    libc.src.stdio.printf_core.writer
+    libc.src.stdio.printf_core.string_writer
+)

libc/test/src/stdio/printf_core/string_writer_test.cpp

+//===-- Unittests for the printf String Writer ----------------------------===//
+//
+// 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/string_writer.h"
+#include "src/stdio/printf_core/writer.h"
+
+#include "utils/UnitTest/Test.h"
+
+TEST(LlvmLibcPrintfStringWriterTest, Constructor) {
+  char str[10];
+  __llvm_libc::printf_core::StringWriter str_writer(str);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+}
+
+TEST(LlvmLibcPrintfStringWriterTest, Write) {
+  char str[4] = {'D', 'E', 'F', 'G'};
+  __llvm_libc::printf_core::StringWriter str_writer(str);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+  writer.write("abc", 3);
+
+  EXPECT_EQ(str[3], 'G');
+  // This null terminates the string. The writer has no indication when the
+  // string is done, so it relies on the user to tell it when to null terminate
+  // the string. Importantly, it can't tell the difference between an intended
+  // max length of 0 (write nothing) or 1 (write just a null byte), and so it
+  // relies on the caller to do that bounds check.
+  str_writer.terminate();
+
+  ASSERT_STREQ("abc", str);
+  ASSERT_EQ(writer.get_chars_written(), 3ull);
+}
+
+TEST(LlvmLibcPrintfStringWriterTest, WriteMultipleTimes) {
+  char str[10];
+  __llvm_libc::printf_core::StringWriter str_writer(str);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+  writer.write("abc", 3);
+  writer.write("DEF", 3);
+  writer.write("1234", 3);
+
+  str_writer.terminate();
+
+  ASSERT_STREQ("abcDEF123", str);
+  ASSERT_EQ(writer.get_chars_written(), 9ull);
+}
+
+TEST(LlvmLibcPrintfStringWriterTest, WriteChars) {
+  char str[4] = {'D', 'E', 'F', 'G'};
+  __llvm_libc::printf_core::StringWriter str_writer(str);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+  writer.write_chars('a', 3);
+
+  EXPECT_EQ(str[3], 'G');
+  str_writer.terminate();
+
+  ASSERT_STREQ("aaa", str);
+  ASSERT_EQ(writer.get_chars_written(), 3ull);
+}
+
+TEST(LlvmLibcPrintfStringWriterTest, WriteCharsMultipleTimes) {
+  char str[10];
+  __llvm_libc::printf_core::StringWriter str_writer(str);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+  writer.write_chars('a', 3);
+  writer.write_chars('D', 3);
+  writer.write_chars('1', 3);
+
+  str_writer.terminate();
+
+  ASSERT_STREQ("aaaDDD111", str);
+  ASSERT_EQ(writer.get_chars_written(), 9ull);
+}
+
+TEST(LlvmLibcPrintfStringWriterTest, WriteManyChars) {
+  char str[100];
+  __llvm_libc::printf_core::StringWriter str_writer(str);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+  writer.write_chars('Z', 99);
+
+  str_writer.terminate();
+
+  ASSERT_STREQ("ZZZZZZZZZZ"
+               "ZZZZZZZZZZ"
+               "ZZZZZZZZZZ"
+               "ZZZZZZZZZZ"
+               "ZZZZZZZZZZ"
+               "ZZZZZZZZZZ"
+               "ZZZZZZZZZZ"
+               "ZZZZZZZZZZ"
+               "ZZZZZZZZZZ"
+               "ZZZZZZZZZ",
+               str);
+  ASSERT_EQ(writer.get_chars_written(), 99ull);
+}
+
+TEST(LlvmLibcPrintfStringWriterTest, MixedWrites) {
+  char str[13];
+  __llvm_libc::printf_core::StringWriter str_writer(str);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+  writer.write_chars('a', 3);
+  writer.write("DEF", 3);
+  writer.write_chars('1', 3);
+  writer.write("456", 3);
+
+  str_writer.terminate();
+
+  ASSERT_STREQ("aaaDEF111456", str);
+  ASSERT_EQ(writer.get_chars_written(), 12ull);
+}
+
+TEST(LlvmLibcPrintfStringWriterTest, WriteWithMaxLength) {
+  char str[11];
+  __llvm_libc::printf_core::StringWriter str_writer(str, 10);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+  writer.write("abcDEF123456", 12);
+
+  str_writer.terminate();
+
+  ASSERT_STREQ("abcDEF1234", str);
+  ASSERT_EQ(writer.get_chars_written(), 12ull);
+}
+
+TEST(LlvmLibcPrintfStringWriterTest, WriteCharsWithMaxLength) {
+  char str[11];
+  __llvm_libc::printf_core::StringWriter str_writer(str, 10);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+
+  writer.write_chars('1', 15);
+
+  str_writer.terminate();
+
+  ASSERT_STREQ("1111111111", str);
+  ASSERT_EQ(writer.get_chars_written(), 15ull);
+}
+
+TEST(LlvmLibcPrintfStringWriterTest, MixedWriteWithMaxLength) {
+  char str[11];
+  __llvm_libc::printf_core::StringWriter str_writer(str, 10);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+  writer.write_chars('a', 3);
+  writer.write("DEF", 3);
+  writer.write_chars('1', 3);
+  writer.write("456", 3);
+
+  str_writer.terminate();
+
+  ASSERT_STREQ("aaaDEF1114", str);
+  ASSERT_EQ(writer.get_chars_written(), 12ull);
+}
+
+TEST(LlvmLibcPrintfStringWriterTest, StringWithMaxLengthOne) {
+  char str[1];
+  __llvm_libc::printf_core::StringWriter str_writer(str, 0);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+  // This is because the max length should be at most 1 less than the size of
+  // the buffer it's writing to.
+  writer.write_chars('a', 3);
+  writer.write("DEF", 3);
+  writer.write_chars('1', 3);
+  writer.write("456", 3);
+
+  str_writer.terminate();
+
+  ASSERT_STREQ("", str);
+  ASSERT_EQ(writer.get_chars_written(), 12ull);
+}
+
+TEST(LlvmLibcPrintfStringWriterTest, NullStringWithZeroMaxLength) {
+  __llvm_libc::printf_core::StringWriter str_writer(nullptr, 0);
+  __llvm_libc::printf_core::Writer writer(
+      reinterpret_cast<void *>(&str_writer),
+      __llvm_libc::printf_core::write_to_string);
+  writer.write_chars('a', 3);
+  writer.write("DEF", 3);
+  writer.write_chars('1', 3);
+  writer.write("456", 3);
+
+  ASSERT_EQ(writer.get_chars_written(), 12ull);
+}