[libc][test] Improve memory check reporting

[gchatelet]

• Remove leftover from debugging session

[sivachandra]

libc/test/src/string/memory_utils/memory_check_utils.h

111

I did not look enough to see if the logging parts are runtime code or test only code. If it is test only, may be add a comment saying that? If not, then a better approach is to use LIBC_ASSERT: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/libc_assert.h

[gchatelet]

libc/test/src/string/memory_utils/memory_check_utils.h

111

I did not look enough to see if the logging parts are runtime code or test only code. If it is test only, may be add a comment saying that? If not, then a better approach is to use LIBC_ASSERT: https://github.com/llvm/llvm-project/blob/main/libc/src/__support/libc_assert.h

testing::tlog is the logging infrastructure that is used in LibUnitTest and this file is used in unit tests only (libc/*test*/src/string/memory_utils/memory_check_utils.h) so I think this is OK. Are you suggesting that I comment all testing::log lines in this file?

[sivachandra]

libc/test/src/string/memory_utils/memory_check_utils.h

111

Sorry, I did not compose my comment correctly. I should have said:

... if the logging parts are **for testing runtime code**...

What I meant was, if these messages are for catching test failures, then they should be in some EXPECT_* as the raw log messages do not come with line numbers. Or, if they are only providing assert like protection, using a real assert like LIBC_ASSERT is better because they also come with line numbers. About comments, if you do not want to use EXPECT_* macros, add comments why.

[courbet]

libc/test/src/string/memcmp_test.cpp

54

ASSERT_TRUE(CheckMemcmp<Impl>(span1, span2, size)) << "size=" << size; ?

[gchatelet]

libc/test/src/string/memcmp_test.cpp

54

The libc version of ASSERT_* and EXPECT_* do not allow logging yet.

libc/test/src/string/memory_utils/memory_check_utils.h

111

Ha ok : ) Now I understand.

So this is indeed to catch test failures. I did not use EXPECT_* for several reasons:

• Getting the faulty line in this function is not interesting for debugging as you need more context. For instance, the function can be called several times in one test and you really need to know which of the call site is failing (hence the ASSERT_TRUE in the test function).
• The function is usually called in a loop to test many different configurations, EXPECT_* will produce a huge log making it hard to identify the root cause. It would need to be an ASSERT_* but it is only supposed to be called from the test itself since it returns.

Now another solution could be to use EXPECT_* here, to return false and to ASSERT_TRUE at the call site. But we would still need to log additional information. Currently, EXPECT_* and ASSERT_* do not allow logging.

I think the solution here is to create custom matchers.

libc/test/src/string/memcmp_test.cpp

 //===-- Unittests for memcmp ----------------------------------------------===//
 //
 // 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 "memory_utils/memory_check_utils.h"
 #include "src/string/memcmp.h"
 #include "test/UnitTest/Test.h"
-#include "test/UnitTest/TestLogger.h"
 
 namespace __llvm_libc {
 
 TEST(LlvmLibcMemcmpTest, CmpZeroByte) {
   const char *lhs = "ab";
   const char *rhs = "yz";
   EXPECT_EQ(__llvm_libc::memcmp(lhs, rhs, 0), 0);
 }
TEST(LlvmLibcMemcmpTest, SizeSweep) {
   static constexpr size_t kMaxSize = 1024;
   static constexpr auto Impl = CmpAdaptor<__llvm_libc::memcmp>;
   Buffer Buffer1(kMaxSize);
   Buffer Buffer2(kMaxSize);
   Randomize(Buffer1.span());
   for (size_t size = 0; size < kMaxSize; ++size) {
     auto span1 = Buffer1.span().subspan(0, size);
     auto span2 = Buffer2.span().subspan(0, size);
-    const bool OK = CheckMemcmp<Impl>(span1, span2, size);
-    if (!OK)
-      testing::tlog << "Failed at size=" << size << '\n';
-    ASSERT_TRUE(OK);
+    ASSERT_TRUE(CheckMemcmp<Impl>(span1, span2, size));
   }
 }
 
 } // namespace __llvm_libc

libc/test/src/string/memory_utils/memory_check_utils.h

 //===-- Utils to test conformance of mem functions ------------------------===//
 //
 // 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_TEST_SRC_STRING_MEMORY_UTILS_MEMORY_CHECK_UTILS_H
 #define LIBC_TEST_SRC_STRING_MEMORY_UTILS_MEMORY_CHECK_UTILS_H
 
 #include "src/__support/CPP/span.h"
 #include "src/__support/macros/sanitizer.h"
 #include "src/string/memory_utils/utils.h"
+#include "test/UnitTest/TestLogger.h"
 #include <stddef.h> // size_t
 #include <stdint.h> // uintxx_t
 #include <stdlib.h> // malloc/free
 
 namespace __llvm_libc {
 
 // Simple structure to allocate a buffer of a particular size.
 // When ASAN is present it also poisons the whole memory.

 }
 
 // Checks that FnImpl implements the memcpy semantic.
 template <auto FnImpl>
 bool CheckMemcpy(cpp::span<char> dst, cpp::span<char> src, size_t size) {
   Randomize(dst);
   FnImpl(dst, src, size);
   for (size_t i = 0; i < size; ++i)
-    if (dst[i] != src[i])
+    if (dst[i] != src[i]) {
+      testing::tlog << "memcpy error\n"
+                    << "size=" << size << ", mismatch_index=" << i << '\n';
       return false;
+    }
   return true;
 }
 
 // Checks that FnImpl implements the memset semantic.
 template <auto FnImpl>
 bool CheckMemset(cpp::span<char> dst, uint8_t value, size_t size) {
   Randomize(dst);
   FnImpl(dst, value, size);
-  for (char c : dst)
-    if (c != (char)value)
+  for (size_t i = 0; i < size; ++i)
+    if (dst[i] != (char)value) {
+      testing::tlog << "memset error\n"
+                    << "size=" << size << ", mismatch_index=" << i << '\n';
       return false;
+    }
   return true;
 }
 
 // Checks that FnImpl implements the bcmp semantic.
 template <auto FnImpl>
 bool CheckBcmp(cpp::span<char> span1, cpp::span<char> span2, size_t size) {
+  const auto error = [=](const char *msg, int cmp,
+                         size_t mismatch_index) -> bool {
+    testing::tlog << "bcmp error : " << msg << '\n'
+                  << "cmp=" << cmp << ", size=" << size
+                  << ", mismatch_index=" << mismatch_index << '\n';
+    return false;
+  };
   ReferenceCopy(span2, span1);
   // Compare equal
   if (int cmp = FnImpl(span1, span2, size); cmp != 0)
-    return false;
+    return error("buffer are equal expected cmp == 0", cmp, 0);
   // Compare not equal if any byte differs
   for (size_t i = 0; i < size; ++i) {
     ++span2[i];
     if (int cmp = FnImpl(span1, span2, size); cmp == 0)
-      return false;
+      return error("buffer are equal expected cmp != 0", cmp, i);
     if (int cmp = FnImpl(span2, span1, size); cmp == 0)
-      return false;
+      return error("buffer are equal expected cmp != 0", cmp, i);
     --span2[i];
   }
   return true;
 }
 
 // Checks that FnImpl implements the memcmp semantic.
 template <auto FnImpl>
 bool CheckMemcmp(cpp::span<char> span1, cpp::span<char> span2, size_t size) {
+  const auto error = [=](const char *msg, int cmp,
+                         size_t mismatch_index) -> bool {
+    testing::tlog << "memcmp error : " << msg << '\n'
+                  << "cmp=" << cmp << ", size=" << size
+                  << ", mismatch_index=" << mismatch_index << '\n';
+    return false;
+  };
   ReferenceCopy(span2, span1);
   // Compare equal
   if (int cmp = FnImpl(span1, span2, size); cmp != 0)
-    return false;
+    return error("buffer are equal expected cmp == 0", cmp, 0);
   // Compare not equal if any byte differs
   for (size_t i = 0; i < size; ++i) {
     ++span2[i];
     int ground_truth = __builtin_memcmp(span1.data(), span2.data(), size);
     if (ground_truth > 0) {
       if (int cmp = FnImpl(span1, span2, size); cmp <= 0)
-        return false;
+        return error("buffer are different, expected cmp > 0", cmp, i);
       if (int cmp = FnImpl(span2, span1, size); cmp >= 0)
-        return false;
+        return error("buffer are different, expected cmp < 0", cmp, i);
     } else {
       if (int cmp = FnImpl(span1, span2, size); cmp >= 0)
-        return false;
+        return error("buffer are different, expected cmp < 0", cmp, i);
       if (int cmp = FnImpl(span2, span1, size); cmp <= 0)
-        return false;
+        return error("buffer are different, expected cmp > 0", cmp, i);
     }
     --span2[i];
   }
   return true;
 }
 
 // TODO: Also implement the memmove semantic
 
 } // namespace __llvm_libc
 
 #endif // LIBC_TEST_SRC_STRING_MEMORY_UTILS_MEMORY_CHECK_UTILS_H