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

[via libc-commits]

Author: George Burgess IV
Date: 2024-10-30T15:08:03-07:00
New Revision: 50c44478fe3f680374edf1363d2a3617b8ff2a0b

URL: https://github.com/llvm/llvm-project/commit/50c44478fe3f680374edf1363d2a3617b8ff2a0b
DIFF: https://github.com/llvm/llvm-project/commit/50c44478fe3f680374edf1363d2a3617b8ff2a0b.diff

LOG: [libc] fix behavior of strrchr(x, '\0') (#112620)

`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.

Added: 
    

Modified: 
    libc/src/string/string_utils.h
    libc/test/UnitTest/LibcTest.h
    libc/test/src/string/StrchrTest.h

Removed: 
    


################################################################################
diff  --git a/libc/src/string/string_utils.h b/libc/src/string/string_utils.h
index 240b28f15718a8..22a1876da5369c 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);
   }
 };