[libc-commits] [libc] [libc] fix behavior of strrchr(x, '\0') (PR #112620)

[George Burgess IV via libc-commits]

https://github.com/gburgessiv updated https://github.com/llvm/llvm-project/pull/112620

>From 74120f257f571b2ddec403d08cb1e84f39bf2a01 Mon Sep 17 00:00:00 2001
From: George Burgess IV <george.burgess.iv at gmail.com>
Date: Wed, 16 Oct 2024 15:17:07 -0600
Subject: [PATCH] [libc] fix behavior of strrchr(x, '\0')

`strrchr("foo", '\0')` is defined to point to the end of `foo`, rather
than returning NULL. This wasn't caught by tests, since llvm-libc's
`ASSERT_STREQ(nullptr, "");` is not an assertion error.

While I'm here, refactor the test slightly to check for NULL more
specifically. I considered adding fancier `ASSERT`s (and changing the
semantics of `ASSERT_STREQ`), but opted for a more local fix by fair
dice roll.
---
 libc/src/string/string_utils.h    |  6 ++++--
 libc/test/UnitTest/LibcTest.h     |  8 +++++++
 libc/test/src/string/StrchrTest.h | 36 +++++++++++++++++++------------
 3 files changed, 34 insertions(+), 16 deletions(-)

diff --git a/libc/src/string/string_utils.h b/libc/src/string/string_utils.h
index 78381e46e480dd..90f6e4e710e5c4 100644
--- a/libc/src/string/string_utils.h
+++ b/libc/src/string/string_utils.h
@@ -239,11 +239,13 @@ LIBC_INLINE constexpr static char *strrchr_implementation(const char *src,
                                                           int c) {
   char ch = static_cast<char>(c);
   char *last_occurrence = nullptr;
-  for (; *src; ++src) {
+  while (true) {
     if (*src == ch)
       last_occurrence = const_cast<char *>(src);
+    if (!*src)
+      return last_occurrence;
+    ++src;
   }
-  return last_occurrence;
 }
 
 } // namespace internal
diff --git a/libc/test/UnitTest/LibcTest.h b/libc/test/UnitTest/LibcTest.h
index 2b972004e9eeaa..1707c3c0fdcfad 100644
--- a/libc/test/UnitTest/LibcTest.h
+++ b/libc/test/UnitTest/LibcTest.h
@@ -162,6 +162,14 @@ class Test {
                           (unsigned long long)RHS, LHSStr, RHSStr, Loc);
   }
 
+  // Helper to allow macro invocations like `ASSERT_EQ(foo, nullptr)`.
+  template <typename ValType,
+            cpp::enable_if_t<cpp::is_pointer_v<ValType>, ValType> = nullptr>
+  bool test(TestCond Cond, ValType LHS, std::nullptr_t, const char *LHSStr,
+            const char *RHSStr, internal::Location Loc) {
+    return test(Cond, LHS, static_cast<ValType>(nullptr), LHSStr, RHSStr, Loc);
+  }
+
   template <
       typename ValType,
       cpp::enable_if_t<
diff --git a/libc/test/src/string/StrchrTest.h b/libc/test/src/string/StrchrTest.h
index 74e172de95953e..8c3fe5293008a1 100644
--- a/libc/test/src/string/StrchrTest.h
+++ b/libc/test/src/string/StrchrTest.h
@@ -40,14 +40,16 @@ template <auto Func> struct StrchrTest : public LIBC_NAMESPACE::testing::Test {
     const char *src = "abcde";
 
     // Should return null terminator.
-    ASSERT_STREQ(Func(src, '\0'), "");
+    const char *nul_terminator = Func(src, '\0');
+    ASSERT_NE(nul_terminator, nullptr);
+    ASSERT_STREQ(nul_terminator, "");
     // Source string should not change.
     ASSERT_STREQ(src, "abcde");
   }
 
   void characterNotWithinStringShouldReturnNullptr() {
     // Since 'z' is not within the string, should return nullptr.
-    ASSERT_STREQ(Func("123?", 'z'), nullptr);
+    ASSERT_EQ(Func("123?", 'z'), nullptr);
   }
 
   void theSourceShouldNotChange() {
@@ -74,11 +76,13 @@ template <auto Func> struct StrchrTest : public LIBC_NAMESPACE::testing::Test {
 
   void emptyStringShouldOnlyMatchNullTerminator() {
     // Null terminator should match.
-    ASSERT_STREQ(Func("", '\0'), "");
+    const char empty_string[] = "";
+    ASSERT_EQ(static_cast<const char *>(Func(empty_string, '\0')),
+              empty_string);
     // All other characters should not match.
-    ASSERT_STREQ(Func("", 'Z'), nullptr);
-    ASSERT_STREQ(Func("", '3'), nullptr);
-    ASSERT_STREQ(Func("", '*'), nullptr);
+    ASSERT_EQ(Func("", 'Z'), nullptr);
+    ASSERT_EQ(Func("", '3'), nullptr);
+    ASSERT_EQ(Func("", '*'), nullptr);
   }
 };
 
@@ -114,7 +118,9 @@ template <auto Func> struct StrrchrTest : public LIBC_NAMESPACE::testing::Test {
     const char *src = "abcde";
 
     // Should return null terminator.
-    ASSERT_STREQ(Func(src, '\0'), "");
+    const char *nul_terminator = Func(src, '\0');
+    ASSERT_NE(nul_terminator, nullptr);
+    ASSERT_STREQ(nul_terminator, "");
     // Source string should not change.
     ASSERT_STREQ(src, "abcde");
   }
@@ -122,9 +128,9 @@ template <auto Func> struct StrrchrTest : public LIBC_NAMESPACE::testing::Test {
   void findsLastBehindFirstNullTerminator() {
     static const char src[6] = {'a', 'a', '\0', 'b', '\0', 'c'};
     // 'b' is behind a null terminator, so should not be found.
-    ASSERT_STREQ(Func(src, 'b'), nullptr);
+    ASSERT_EQ(Func(src, 'b'), nullptr);
     // Same goes for 'c'.
-    ASSERT_STREQ(Func(src, 'c'), nullptr);
+    ASSERT_EQ(Func(src, 'c'), nullptr);
 
     // Should find the second of the two a's.
     ASSERT_STREQ(Func(src, 'a'), "a");
@@ -132,7 +138,7 @@ template <auto Func> struct StrrchrTest : public LIBC_NAMESPACE::testing::Test {
 
   void characterNotWithinStringShouldReturnNullptr() {
     // Since 'z' is not within the string, should return nullptr.
-    ASSERT_STREQ(Func("123?", 'z'), nullptr);
+    ASSERT_EQ(Func("123?", 'z'), nullptr);
   }
 
   void shouldFindLastOfDuplicates() {
@@ -146,11 +152,13 @@ template <auto Func> struct StrrchrTest : public LIBC_NAMESPACE::testing::Test {
 
   void emptyStringShouldOnlyMatchNullTerminator() {
     // Null terminator should match.
-    ASSERT_STREQ(Func("", '\0'), "");
+    const char empty_string[] = "";
+    ASSERT_EQ(static_cast<const char *>(Func(empty_string, '\0')),
+              empty_string);
     // All other characters should not match.
-    ASSERT_STREQ(Func("", 'A'), nullptr);
-    ASSERT_STREQ(Func("", '2'), nullptr);
-    ASSERT_STREQ(Func("", '*'), nullptr);
+    ASSERT_EQ(Func("", 'A'), nullptr);
+    ASSERT_EQ(Func("", '2'), nullptr);
+    ASSERT_EQ(Func("", '*'), nullptr);
   }
 };