[libc] fix behavior of strrchr(x, '\0') #112620
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-libc Author: George Burgess IV (gburgessiv) Changes
While I'm here, refactor the test slightly to check for NULL more specifically. I considered adding fancier Full diff: https://github.com/llvm/llvm-project/pull/112620.diff 2 Files Affected:
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/src/string/StrchrTest.h b/libc/test/src/string/StrchrTest.h
index 74e172de95953e..c6267f042f59ea 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_TRUE(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_TRUE(Func("123?", 'z') == nullptr);
}
void theSourceShouldNotChange() {
@@ -74,11 +76,12 @@ 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_TRUE(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_TRUE(Func("", 'Z') == nullptr);
+ ASSERT_TRUE(Func("", '3') == nullptr);
+ ASSERT_TRUE(Func("", '*') == nullptr);
}
};
@@ -114,7 +117,7 @@ template <auto Func> struct StrrchrTest : public LIBC_NAMESPACE::testing::Test {
const char *src = "abcde";
// Should return null terminator.
- ASSERT_STREQ(Func(src, '\0'), "");
+ ASSERT_TRUE(Func(src, '\0') != nullptr);
// Source string should not change.
ASSERT_STREQ(src, "abcde");
}
@@ -122,9 +125,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_TRUE(Func(src, 'b') == nullptr);
// Same goes for 'c'.
- ASSERT_STREQ(Func(src, 'c'), nullptr);
+ ASSERT_TRUE(Func(src, 'c') == nullptr);
// Should find the second of the two a's.
ASSERT_STREQ(Func(src, 'a'), "a");
@@ -132,7 +135,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_TRUE(Func("123?", 'z') == nullptr);
}
void shouldFindLastOfDuplicates() {
@@ -146,11 +149,12 @@ 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_TRUE(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_TRUE(Func("", 'A') == nullptr);
+ ASSERT_TRUE(Func("", '2') == nullptr);
+ ASSERT_TRUE(Func("", '*') == nullptr);
}
};
|
|
Thanks for the patch! Yes, that's a bug! I thought the nice thing about |
libc/test/src/string/StrchrTest.h
Outdated
| @@ -114,25 +117,25 @@ template <auto Func> struct StrrchrTest : public LIBC_NAMESPACE::testing::Test { | |||
| const char *src = "abcde"; | |||
|
|
|||
| // Should return null terminator. | |||
| ASSERT_STREQ(Func(src, '\0'), ""); | |||
| ASSERT_TRUE(Func(src, '\0') != nullptr); | |||
There was a problem hiding this comment.
I think this hunk specifically no longer aligns with the commend above it?
We do define a (currently unused) ASSERT_STRNE.
There was a problem hiding this comment.
maybe Func(src, '\0') != nullptr && *Func(src, '\0') == '\0'?
There was a problem hiding this comment.
Ah, good catch. I mirrored the // Should return null terminator. change from above. Happy to do Func(src, '\0') != nullptr && *Func(src, '\0') == '\0' if you all would prefer; just want to keep the two in sync
gburgessiv
force-pushed
the
libc-fix-bug-in-strrchr
branch
from
b2cba66 to
0d3e459
Compare
|
friendly ping :) |
1 similar comment
|
friendly ping :) |
|
Oh, I didn't realize that I missed this:
My mistake - I'll see what I can do :) |
`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.
gburgessiv
force-pushed
the
libc-fix-bug-in-strrchr
branch
from
0d3e459 to
74120f2
Compare
|
So, better response:
I tried to use
|
nickdesaulniers
approved these changes
Oct 30, 2024
|
Thanks! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/71/builds/9495 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/171/builds/9306 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/131/builds/9388 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/179/builds/9233 Here is the relevant piece of the build log for the reference |
|
@gburgessiv I think you need to replace |
| // 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, |
There was a problem hiding this comment.
Looks like this is breaking the buildbots.
/home/libc_worker/libc-riscv64-debian/libc-riscv64-debian-fullbuild-dbg/llvm-project/libc/test/UnitTest/LibcTest.h:168:41: error: use of undeclared identifier 'std'
bool test(TestCond Cond, ValType LHS, std::nullptr_t, const char *LHSStr,
^
mind reverting if you can't fix forward quickly?
There was a problem hiding this comment.
We have libc/src/__support/CPP/type_traits/is_null_pointer.h which defines cpp::nullptr_t. That include and that value should be used in place of std::nullptr_t I suspect.
There was a problem hiding this comment.
Thanks for helping with this forward fix, and apologies for the breakage :)
smallp-o-p
pushed a commit
to smallp-o-p/llvm-project
that referenced
this pull request
Nov 3, 2024
`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.
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
`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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
strrchr("foo", '\0')is defined to point to the end offoo, rather than returning NULL. This wasn't caught by tests, since llvm-libc'sASSERT_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
ASSERTs (and changing the semantics ofASSERT_STREQ), but opted for a more local fix by fair dice roll.