[libc] Add Printf FormatSection Matcher

[michaelrj]

Summary

This patch changes the printf parser tests to use a more robust matcher.
This allows for better debugging of parsing issues. This does not affect
the actual printf code at all, only the tests.

[lntue]

libc/utils/UnitTest/PrintfMatcher.cpp

72

You might want to wrap the template int_to_hex and display function inside an anonymous namespace.

libc/utils/UnitTest/PrintfMatcher.h

39

Since *form* is also an actual word, it might be better to change these macros to EXPECT/ASSERT_FORMAT_EQ

[michaelrj]

address comments

[sivachandra]

libc/utils/UnitTest/PrintfMatcher.cpp

17

Instead of all the comparisons in this function, can you define operator== on FormatSection?

24

We can use cpp::StringView to perform this comparison.

52

We have this: https://github.com/llvm/llvm-project/blob/main/libc/utils/UnitTest/FPMatcher.cpp#L23

You can move it to a common place may be? You can use std::string in this file.

66

Wrap this in a do-while?

117

"Expected format section: "

120

"Actual format section: "

[michaelrj]

add a util file for int_to_hex and address other comments

libc/utils/UnitTest/PrintfMatcher.cpp

17

I think it's better for final code size if FormatSection doesn't have the comparison operator since it's never used in actual printf code.

24

we could create StringViews for these, but I don't see a reason to, since this is closer to the way these objects are actually used.

52

I've done that, does the bazel also need an update?

[sivachandra]

libc/utils/UnitTest/PrintfMatcher.cpp

17

Unused inline functions/methods will not contribute to code size. Also, applications which care about code size will/should use LTO which will kick out all unused functions. You can add a comment along side operator== to make it clear that it is used only for testing.

24

It could be used differently elsewhere but you are comparing two string views here. So, the following is much better and avoids duplication of code:

if (cpp::StringView(expected.raw_string, expected.raw_len) != cpp::StringView(expected.raw_string, expected.raw_len))
  return false;

It also makes the conditional on line 23 simpler:

if (expected.has_conv != actual.has_conv)
  return false;

[michaelrj]

address comments

libc/utils/UnitTest/PrintfMatcher.cpp

17

alright, in that case I'll just do it.

24

Ah, I thought StringView was more complicated than that. I've done that.

[lntue]

libc/src/stdio/printf_core/core_structs.h

55 ↗

(On Diff #424332)

FormatSection is quite big, you should use const-ref:

bool operator==(const FormatSection &other) const { ... }

[sivachandra]

OK after addressing @lntue's comment.

[sivachandra]

libc/utils/UnitTest/CMakeLists.txt

20

Ah, if you add this with add_header_library, you will not require the empty cpp file.

[michaelrj]

address comments

[michaelrj]

final clean up before land

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)

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

 //===-- Unittests for the printf Parser -----------------------------------===//
 //
 // 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/__support/CPP/Bit.h"
 #include "src/__support/arg_list.h"
 #include "src/stdio/printf_core/parser.h"
 
 #include <stdarg.h>
 
+#include "utils/UnitTest/PrintfMatcher.h"
 #include "utils/UnitTest/Test.h"
 
-class LlvmLibcPrintfParserTest : public __llvm_libc::testing::Test {
-public:
-  void assert_eq_fs(__llvm_libc::printf_core::FormatSection expected,
-                    __llvm_libc::printf_core::FormatSection actual) {
-    ASSERT_EQ(expected.has_conv, actual.has_conv);
-    ASSERT_EQ(expected.raw_len, actual.raw_len);
-
-    for (size_t i = 0; i < expected.raw_len; ++i) {
-      EXPECT_EQ(expected.raw_string[i], actual.raw_string[i]);
-    }
-
-    if (expected.has_conv) {
-      ASSERT_EQ(static_cast<uint8_t>(expected.flags),
-                static_cast<uint8_t>(actual.flags));
-      ASSERT_EQ(expected.min_width, actual.min_width);
-      ASSERT_EQ(expected.precision, actual.precision);
-      ASSERT_TRUE(expected.length_modifier == actual.length_modifier);
-      ASSERT_EQ(expected.conv_name, actual.conv_name);
-
-      if (expected.conv_name == 'p' || expected.conv_name == 'n' ||
-          expected.conv_name == 's') {
-        ASSERT_EQ(expected.conv_val_ptr, actual.conv_val_ptr);
-      } else if (expected.conv_name != '%') {
-        ASSERT_EQ(expected.conv_val_raw, actual.conv_val_raw);
-      }
-    }
-  }
-};
-
 void init(const char *__restrict str, ...) {
   va_list vlist;
   va_start(vlist, str);
   __llvm_libc::internal::ArgList v(vlist);
   va_end(vlist);
 
   __llvm_libc::printf_core::Parser parser(str, v);
 }
void evaluate(__llvm_libc::printf_core::FormatSection *format_arr,
 
   for (auto cur_section = parser.get_next_section(); cur_section.raw_len > 0;
        cur_section = parser.get_next_section()) {
     *format_arr = cur_section;
     ++format_arr;
   }
 }
 
-TEST_F(LlvmLibcPrintfParserTest, Constructor) { init("test", 1, 2); }
+TEST(LlvmLibcPrintfParserTest, Constructor) { init("test", 1, 2); }
 
-TEST_F(LlvmLibcPrintfParserTest, EvalRaw) {
+TEST(LlvmLibcPrintfParserTest, EvalRaw) {
   __llvm_libc::printf_core::FormatSection format_arr[10];
   const char *str = "test";
   evaluate(format_arr, str);
 
   __llvm_libc::printf_core::FormatSection expected;
   expected.has_conv = false;
   expected.raw_len = 4;
   expected.raw_string = str;
 
-  assert_eq_fs(expected, format_arr[0]);
+  ASSERT_FORMAT_EQ(expected, format_arr[0]);
 }
 
-TEST_F(LlvmLibcPrintfParserTest, EvalSimple) {
+TEST(LlvmLibcPrintfParserTest, EvalSimple) {
   __llvm_libc::printf_core::FormatSection format_arr[10];
   const char *str = "test %% test";
   evaluate(format_arr, str);
 
   __llvm_libc::printf_core::FormatSection expected0, expected1, expected2;
   expected0.has_conv = false;
   expected0.raw_len = 5;
   expected0.raw_string = str;
 
-  assert_eq_fs(expected0, format_arr[0]);
+  ASSERT_FORMAT_EQ(expected0, format_arr[0]);
 
   expected1.has_conv = true;
   expected1.raw_len = 2;
   expected1.raw_string = str + 5;
   expected1.conv_name = '%';
 
-  assert_eq_fs(expected1, format_arr[1]);
+  ASSERT_FORMAT_EQ(expected1, format_arr[1]);
 
   expected2.has_conv = false;
   expected2.raw_len = 5;
   expected2.raw_string = str + 7;
 
-  assert_eq_fs(expected2, format_arr[2]);
+  ASSERT_FORMAT_EQ(expected2, format_arr[2]);
 }
 
-TEST_F(LlvmLibcPrintfParserTest, EvalOneArg) {
+TEST(LlvmLibcPrintfParserTest, EvalOneArg) {
   __llvm_libc::printf_core::FormatSection format_arr[10];
   const char *str = "%d";
   int arg1 = 12345;
   evaluate(format_arr, str, arg1);
 
   __llvm_libc::printf_core::FormatSection expected;
   expected.has_conv = true;
   expected.raw_len = 2;
   expected.raw_string = str;
   expected.conv_val_raw = arg1;
   expected.conv_name = 'd';
 
-  assert_eq_fs(expected, format_arr[0]);
+  ASSERT_FORMAT_EQ(expected, format_arr[0]);
 }
 
-TEST_F(LlvmLibcPrintfParserTest, EvalOneArgWithFlags) {
+TEST(LlvmLibcPrintfParserTest, EvalOneArgWithFlags) {
   __llvm_libc::printf_core::FormatSection format_arr[10];
   const char *str = "%+-0 #d";
   int arg1 = 12345;
   evaluate(format_arr, str, arg1);
 
   __llvm_libc::printf_core::FormatSection expected;
   expected.has_conv = true;
   expected.raw_len = 7;
   expected.raw_string = str;
   expected.flags = static_cast<__llvm_libc::printf_core::FormatFlags>(
       __llvm_libc::printf_core::FormatFlags::FORCE_SIGN |
       __llvm_libc::printf_core::FormatFlags::LEFT_JUSTIFIED |
       __llvm_libc::printf_core::FormatFlags::LEADING_ZEROES |
       __llvm_libc::printf_core::FormatFlags::SPACE_PREFIX |
       __llvm_libc::printf_core::FormatFlags::ALTERNATE_FORM);
   expected.conv_val_raw = arg1;
   expected.conv_name = 'd';
 
-  assert_eq_fs(expected, format_arr[0]);
+  ASSERT_FORMAT_EQ(expected, format_arr[0]);
 }
 
-TEST_F(LlvmLibcPrintfParserTest, EvalOneArgWithWidth) {
+TEST(LlvmLibcPrintfParserTest, EvalOneArgWithWidth) {
   __llvm_libc::printf_core::FormatSection format_arr[10];
   const char *str = "%12d";
   int arg1 = 12345;
   evaluate(format_arr, str, arg1);
 
   __llvm_libc::printf_core::FormatSection expected;
   expected.has_conv = true;
   expected.raw_len = 4;
   expected.raw_string = str;
   expected.min_width = 12;
   expected.conv_val_raw = arg1;
   expected.conv_name = 'd';
 
-  assert_eq_fs(expected, format_arr[0]);
+  ASSERT_FORMAT_EQ(expected, format_arr[0]);
 }
 
-TEST_F(LlvmLibcPrintfParserTest, EvalOneArgWithPrecision) {
+TEST(LlvmLibcPrintfParserTest, EvalOneArgWithPrecision) {
   __llvm_libc::printf_core::FormatSection format_arr[10];
   const char *str = "%.34d";
   int arg1 = 12345;
   evaluate(format_arr, str, arg1);
 
   __llvm_libc::printf_core::FormatSection expected;
   expected.has_conv = true;
   expected.raw_len = 5;
   expected.raw_string = str;
   expected.precision = 34;
   expected.conv_val_raw = arg1;
   expected.conv_name = 'd';
 
-  assert_eq_fs(expected, format_arr[0]);
+  ASSERT_FORMAT_EQ(expected, format_arr[0]);
 }
 
-TEST_F(LlvmLibcPrintfParserTest, EvalOneArgWithTrivialPrecision) {
+TEST(LlvmLibcPrintfParserTest, EvalOneArgWithTrivialPrecision) {
   __llvm_libc::printf_core::FormatSection format_arr[10];
   const char *str = "%.d";
   int arg1 = 12345;
   evaluate(format_arr, str, arg1);
 
   __llvm_libc::printf_core::FormatSection expected;
   expected.has_conv = true;
   expected.raw_len = 3;
   expected.raw_string = str;
   expected.precision = 0;
   expected.conv_val_raw = arg1;
   expected.conv_name = 'd';
 
-  assert_eq_fs(expected, format_arr[0]);
+  ASSERT_FORMAT_EQ(expected, format_arr[0]);
 }
 
-TEST_F(LlvmLibcPrintfParserTest, EvalOneArgWithShortLengthModifier) {
+TEST(LlvmLibcPrintfParserTest, EvalOneArgWithShortLengthModifier) {
   __llvm_libc::printf_core::FormatSection format_arr[10];
   const char *str = "%hd";
   int arg1 = 12345;
   evaluate(format_arr, str, arg1);
 
   __llvm_libc::printf_core::FormatSection expected;
   expected.has_conv = true;
   expected.raw_len = 3;
   expected.raw_string = str;
   expected.length_modifier = __llvm_libc::printf_core::LengthModifier::h;
   expected.conv_val_raw = arg1;
   expected.conv_name = 'd';
 
-  assert_eq_fs(expected, format_arr[0]);
+  ASSERT_FORMAT_EQ(expected, format_arr[0]);
 }
 
-TEST_F(LlvmLibcPrintfParserTest, EvalOneArgWithLongLengthModifier) {
+TEST(LlvmLibcPrintfParserTest, EvalOneArgWithLongLengthModifier) {
   __llvm_libc::printf_core::FormatSection format_arr[10];
   const char *str = "%lld";
   int arg1 = 12345;
   evaluate(format_arr, str, arg1);
 
   __llvm_libc::printf_core::FormatSection expected;
   expected.has_conv = true;
   expected.raw_len = 4;
   expected.raw_string = str;
   expected.length_modifier = __llvm_libc::printf_core::LengthModifier::ll;
   expected.conv_val_raw = arg1;
   expected.conv_name = 'd';
 
-  assert_eq_fs(expected, format_arr[0]);
+  ASSERT_FORMAT_EQ(expected, format_arr[0]);
 }
 
-TEST_F(LlvmLibcPrintfParserTest, EvalOneArgWithAllOptions) {
+TEST(LlvmLibcPrintfParserTest, EvalOneArgWithAllOptions) {
   __llvm_libc::printf_core::FormatSection format_arr[10];
   const char *str = "% -056.78jd";
   int arg1 = 12345;
   evaluate(format_arr, str, arg1);
 
   __llvm_libc::printf_core::FormatSection expected;
   expected.has_conv = true;
   expected.raw_len = 11;
   expected.raw_string = str;
   expected.flags = static_cast<__llvm_libc::printf_core::FormatFlags>(
       __llvm_libc::printf_core::FormatFlags::LEFT_JUSTIFIED |
       __llvm_libc::printf_core::FormatFlags::LEADING_ZEROES |
       __llvm_libc::printf_core::FormatFlags::SPACE_PREFIX);
   expected.min_width = 56;
   expected.precision = 78;
   expected.length_modifier = __llvm_libc::printf_core::LengthModifier::j;
   expected.conv_val_raw = arg1;
   expected.conv_name = 'd';
 
-  assert_eq_fs(expected, format_arr[0]);
+  ASSERT_FORMAT_EQ(expected, format_arr[0]);
 }
 
-TEST_F(LlvmLibcPrintfParserTest, EvalThreeArgs) {
+TEST(LlvmLibcPrintfParserTest, EvalThreeArgs) {
   __llvm_libc::printf_core::FormatSection format_arr[10];
   const char *str = "%d%f%s";
   int arg1 = 12345;
   double arg2 = 123.45;
   const char *arg3 = "12345";
   evaluate(format_arr, str, arg1, arg2, arg3);
 
   __llvm_libc::printf_core::FormatSection expected0, expected1, expected2;
   expected0.has_conv = true;
   expected0.raw_len = 2;
   expected0.raw_string = str;
   expected0.conv_val_raw = arg1;
   expected0.conv_name = 'd';
 
-  assert_eq_fs(expected0, format_arr[0]);
+  ASSERT_FORMAT_EQ(expected0, format_arr[0]);
 
   expected1.has_conv = true;
   expected1.raw_len = 2;
   expected1.raw_string = str + 2;
   expected1.conv_val_raw = __llvm_libc::bit_cast<uint64_t>(arg2);
   expected1.conv_name = 'f';
 
-  assert_eq_fs(expected1, format_arr[1]);
+  ASSERT_FORMAT_EQ(expected1, format_arr[1]);
 
   expected2.has_conv = true;
   expected2.raw_len = 2;
   expected2.raw_string = str + 4;
   expected2.conv_val_ptr = const_cast<char *>(arg3);
   expected2.conv_name = 's';
 
-  assert_eq_fs(expected2, format_arr[2]);
+  ASSERT_FORMAT_EQ(expected2, format_arr[2]);
 }

libc/utils/UnitTest/CMakeLists.txt

add_library(
   LibcUnitTestMain
   LibcTestMain.cpp
 )
 
 target_include_directories(LibcUnitTestMain PUBLIC ${LIBC_SOURCE_DIR})
 add_dependencies(LibcUnitTestMain LibcUnitTest)
 target_link_libraries(LibcUnitTestMain PUBLIC LibcUnitTest libc_test_utils)
 
 add_library(
   LibcFPTestHelpers
     FPExceptMatcher.cpp
     FPExceptMatcher.h
     FPMatcher.cpp
     FPMatcher.h
 )
 target_include_directories(LibcFPTestHelpers PUBLIC ${LIBC_SOURCE_DIR})
 target_link_libraries(LibcFPTestHelpers LibcUnitTest)

 )
 target_include_directories(LibcMemoryHelpers PUBLIC ${LIBC_SOURCE_DIR})
 target_link_libraries(LibcMemoryHelpers LibcUnitTest)
 add_dependencies(
   LibcMemoryHelpers
   LibcUnitTest
   libc.src.__support.CPP.array_ref
 )
+
+add_library(
+  LibcPrintfHelpers
+    PrintfMatcher.h
+    PrintfMatcher.cpp
+)
+target_include_directories(LibcPrintfHelpers PUBLIC ${LIBC_SOURCE_DIR})
+target_link_libraries(LibcPrintfHelpers LibcUnitTest)
+add_dependencies(
+  LibcPrintfHelpers
+  LibcUnitTest
+  libc.src.stdio.printf_core.core_structs
+)

libc/utils/UnitTest/PrintfMatcher.h

+//===-- PrintfMatcher.h -----------------------------------------*- 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_UTILS_UNITTEST_PRINTF_MATCHER_H
+#define LLVM_LIBC_UTILS_UNITTEST_PRINTF_MATCHER_H
+
+#include "src/stdio/printf_core/core_structs.h"
+#include "utils/UnitTest/Test.h"
+
+#include <errno.h>
+
+namespace __llvm_libc {
+namespace printf_core {
+namespace testing {
+
+class FormatSectionMatcher
+    : public __llvm_libc::testing::Matcher<FormatSection> {
+  FormatSection expected;
+  FormatSection actual;
+
+public:
+  FormatSectionMatcher(FormatSection expectedValue) : expected(expectedValue) {}
+
+  bool match(FormatSection actualValue);
+
+  void explainError(testutils::StreamWrapper &stream) override;
+};
+
+} // namespace testing
+} // namespace printf_core
+} // namespace __llvm_libc
+
+#define EXPECT_FORMAT_EQ(expected, actual)                                     \
+  EXPECT_THAT(actual, __llvm_libc::printf_core::testing::FormatSectionMatcher( \
+                          expected))
+
+#define ASSERT_FORMAT_EQ(expected, actual)                                     \
+  ASSERT_THAT(actual, __llvm_libc::printf_core::testing::FormatSectionMatcher( \
+                          expected))
+
+#endif // LLVM_LIBC_UTILS_UNITTEST_PRINTF_MATCHER_H

libc/utils/UnitTest/PrintfMatcher.cpp

+//===-- PrintfMatcher.cpp ---------------------------------------*- 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 "PrintfMatcher.h"
+#include "src/stdio/printf_core/core_structs.h"
+#include <stdint.h>
+
+namespace __llvm_libc {
+namespace printf_core {
+namespace testing {
+
+bool FormatSectionMatcher::match(FormatSection actualValue) {
+  actual = actualValue;
+
+  if (!((expected.has_conv == actual.has_conv) &&
+        (expected.raw_len == actual.raw_len)))
+    return false;
+
+  for (size_t i = 0; i < expected.raw_len; ++i) {
+    if (!(expected.raw_string[i] == actual.raw_string[i]))
+      return false;
+  }
+
+  if (expected.has_conv) {
+    if (!((static_cast<uint8_t>(expected.flags) ==
+           static_cast<uint8_t>(actual.flags)) &&
+          (expected.min_width == actual.min_width) &&
+          (expected.precision == actual.precision) &&
+          (expected.length_modifier == actual.length_modifier) &&
+          (expected.conv_name == actual.conv_name)))
+      return false;
+
+    if (expected.conv_name == 'p' || expected.conv_name == 'n' ||
+        expected.conv_name == 's')
+      return (expected.conv_val_ptr == actual.conv_val_ptr);
+    else if (expected.conv_name != '%')
+      return (expected.conv_val_raw == actual.conv_val_raw);
+  }
+  return true;
+}
+
+namespace {
+
+template <typename T>
+// int_to_hex takes an integer and a buffer and writes the value of that integer
+// to buff in hex. It also returns buff for ease of use.
+char *int_to_hex(T num, char *buff) {
+  constexpr size_t len = sizeof(T) * 2;
+  buff[len + 2] = '\0';
+  for (size_t i = len + 1; i > 1; --i) {
+    uint8_t digit = num % 16;
+    num = num / 16;
+    buff[i] = (digit < 10 ? '0' + digit : 'A' + digit - 10);
+  }
+  buff[1] = 'x';
+  buff[0] = '0';
+  return buff;
+}
+
+#define IF_FLAG_SHOW_FLAG(flag_name)                                           \
+  if ((form.flags & FormatFlags::flag_name) == FormatFlags::flag_name)         \
+  stream << "\n\t\t" << #flag_name
+#define CASE_LM(lm)                                                            \
+  case (LengthModifier::lm):                                                   \
+    stream << #lm;                                                             \
+    break
+
+void display(testutils::StreamWrapper &stream, FormatSection form) {
+  char buffer[40];
+  stream << "Raw String (len " << form.raw_len << "): \"";
+  for (size_t i = 0; i < form.raw_len; ++i) {
+    stream << form.raw_string[i];
+  }
+  stream << "\"";
+  if (form.has_conv) {
+    stream << "\n\tHas Conv\n\tFlags:";
+    IF_FLAG_SHOW_FLAG(LEFT_JUSTIFIED);
+    IF_FLAG_SHOW_FLAG(FORCE_SIGN);
+    IF_FLAG_SHOW_FLAG(SPACE_PREFIX);
+    IF_FLAG_SHOW_FLAG(ALTERNATE_FORM);
+    IF_FLAG_SHOW_FLAG(LEADING_ZEROES);
+    stream << "\n";
+    stream << "\tmin width: " << form.min_width << "\n";
+    stream << "\tprecision: " << form.precision << "\n";
+    stream << "\tlength modifier: ";
+    switch (form.length_modifier) {
+      CASE_LM(none);
+      CASE_LM(l);
+      CASE_LM(ll);
+      CASE_LM(h);
+      CASE_LM(hh);
+      CASE_LM(j);
+      CASE_LM(z);
+      CASE_LM(t);
+      CASE_LM(L);
+    }
+    stream << "\n";
+    stream << "\tconversion name: " << form.conv_name << "\n";
+    if (form.conv_name == 'p' || form.conv_name == 'n' || form.conv_name == 's')
+      stream << "\tpointer value: "
+             << int_to_hex<uintptr_t>(
+                    reinterpret_cast<uintptr_t>(form.conv_val_ptr), buffer)
+             << "\n";
+    else if (form.conv_name != '%')
+      stream << "\tvalue: "
+             << int_to_hex<__uint128_t>(form.conv_val_raw, buffer) << "\n";
+  }
+}
+} // anonymous namespace
+
+void FormatSectionMatcher::explainError(testutils::StreamWrapper &stream) {
+  stream << "expected : ";
+  display(stream, expected);
+  stream << '\n';
+  stream << "actual   : ";
+  display(stream, actual);
+  stream << '\n';
+}
+
+} // namespace testing
+} // namespace printf_core
+} // namespace __llvm_libc