@@ -29096,6 +29096,17 @@ TEST_F(FormatTest, BreakBeforeClassName) {
"ArenaSafeUniquePtr {};");
}
+TEST_F(FormatTest, DoesNotCrashOnNonNullTerminatedStringRefs) {
+ llvm::StringRef TwoLines = "namespace foo {}\n"
+ "namespace bar
@@ -2031,6 +2031,7 @@ class SourceManagerForFile {
// The order of these fields are important - they should be in the same order
// as they are created in `createSourceManagerForFile` so that they can be
// deleted in the reverse order as they are created.
+ std::string
@@ -29096,6 +29096,17 @@ TEST_F(FormatTest, BreakBeforeClassName) {
"ArenaSafeUniquePtr {};");
}
+TEST_F(FormatTest, DoesNotCrashOnNonNullTerminatedStringRefs) {
+ llvm::StringRef TwoLines = "namespace foo {}\n"
+ "namespace bar
@@ -29096,6 +29096,17 @@ TEST_F(FormatTest, BreakBeforeClassName) {
"ArenaSafeUniquePtr {};");
}
+TEST_F(FormatTest, DoesNotCrashOnNonNullTerminatedStringRefs) {
+ llvm::StringRef TwoLines = "namespace foo {}\n"
ilya-biryukov wrote:
Done.
https://github.com/ilya-biryukov updated
https://github.com/llvm/llvm-project/pull/131299
>From 1acb9b0717c0f55e59abca104bbb710375a67610 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov
Date: Fri, 14 Mar 2025 10:53:09 +0100
Subject: [PATCH 1/3] [Format] Do not crash on non-null terminated strings
ilya-biryukov wrote:
> Can you add a reproducer?
I'm not sure I can do better than the test I've added.
My colleague caught this by accidentally getting an assertion failure when
using this API downstream.
The test illustrates how it got used and I'm not sure if `clang-format` binary
or any ot
@@ -29096,6 +29096,17 @@ TEST_F(FormatTest, BreakBeforeClassName) {
"ArenaSafeUniquePtr {};");
}
+TEST_F(FormatTest, DoesNotCrashOnNonNullTerminatedStringRefs) {
+ llvm::StringRef TwoLines = "namespace foo {}\n"
+ "namespace bar
https://github.com/owenca edited
https://github.com/llvm/llvm-project/pull/131299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/owenca commented:
Can you add a reproducer?
https://github.com/llvm/llvm-project/pull/131299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -29096,6 +29096,17 @@ TEST_F(FormatTest, BreakBeforeClassName) {
"ArenaSafeUniquePtr {};");
}
+TEST_F(FormatTest, DoesNotCrashOnNonNullTerminatedStringRefs) {
+ llvm::StringRef TwoLines = "namespace foo {}\n"
owenca wrote:
```suggestion
https://github.com/ilya-biryukov edited
https://github.com/llvm/llvm-project/pull/131299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -2382,14 +2382,20 @@ size_t SourceManager::getDataStructureSizes() const {
SourceManagerForFile::SourceManagerForFile(StringRef FileName,
StringRef Content) {
+ // We copy to `std::string` for Context instead of StringRef because
@@ -2382,14 +2382,20 @@ size_t SourceManager::getDataStructureSizes() const {
SourceManagerForFile::SourceManagerForFile(StringRef FileName,
StringRef Content) {
+ // We copy to `std::string` for Context instead of StringRef because
@@ -2382,14 +2382,20 @@ size_t SourceManager::getDataStructureSizes() const {
SourceManagerForFile::SourceManagerForFile(StringRef FileName,
StringRef Content) {
+ // We copy to `std::string` for Context instead of StringRef because
https://github.com/ilya-biryukov updated
https://github.com/llvm/llvm-project/pull/131299
>From 1acb9b0717c0f55e59abca104bbb710375a67610 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov
Date: Fri, 14 Mar 2025 10:53:09 +0100
Subject: [PATCH 1/2] [Format] Do not crash on non-null terminated strings
@@ -2382,14 +2382,20 @@ size_t SourceManager::getDataStructureSizes() const {
SourceManagerForFile::SourceManagerForFile(StringRef FileName,
StringRef Content) {
+ // We copy to `std::string` for Context instead of StringRef because
@@ -2031,6 +2031,7 @@ class SourceManagerForFile {
// The order of these fields are important - they should be in the same order
// as they are created in `createSourceManagerForFile` so that they can be
// deleted in the reverse order as they are created.
+ std::string
@@ -2382,14 +2382,20 @@ size_t SourceManager::getDataStructureSizes() const {
SourceManagerForFile::SourceManagerForFile(StringRef FileName,
StringRef Content) {
+ // We copy to `std::string` for Context instead of StringRef because
cor3ntin wrote:
It would be useful to have a repro or a stack trace here.
In particular, in `SourceManagerForFile`, `RequiresNullTerminator` is `false`,
so the assert should _not_ fire on a non-null terminated file
https://github.com/llvm/llvm-project/pull/131299
__
ilya-biryukov wrote:
I am going with the path of least resistance in this change and I would welcome
any concerns or alternative suggestions.
It definitely has trade-offs as we now incur an extra copy, but this seems
acceptable for a convenience API that `SourceManagerForFile` aims to be.
htt
@@ -2031,6 +2031,7 @@ class SourceManagerForFile {
// The order of these fields are important - they should be in the same order
// as they are created in `createSourceManagerForFile` so that they can be
// deleted in the reverse order as they are created.
+ std::string
@@ -2382,14 +2382,20 @@ size_t SourceManager::getDataStructureSizes() const {
SourceManagerForFile::SourceManagerForFile(StringRef FileName,
StringRef Content) {
+ // We copy to `std::string` for Context instead of StringRef because
@@ -29096,6 +29096,17 @@ TEST_F(FormatTest, BreakBeforeClassName) {
"ArenaSafeUniquePtr {};");
}
+TEST_F(FormatTest, DoesNotCrashOnNonNullTerminatedStringRefs) {
+ llvm::StringRef TwoLines = "namespace foo {}\n"
+ "namespace bar
@@ -2382,14 +2382,20 @@ size_t SourceManager::getDataStructureSizes() const {
SourceManagerForFile::SourceManagerForFile(StringRef FileName,
StringRef Content) {
+ // We copy to `std::string` for Context instead of StringRef because
ilya-biryukov wrote:
> It would be useful to have a repro or a stack trace here. In particular, in
> `SourceManagerForFile`, `RequiresNullTerminator` is `false`, so the assert
> should _not_ fire on a non-null terminated file
The repro is attached to the commit. Here is the full stack trace:
llvmbot wrote:
@llvm/pr-subscribers-clang-format
Author: Ilya Biryukov (ilya-biryukov)
Changes
The `format` API receives a StringRef, but crashes whenever it is
non-null-terminated with the corresponding assertion:
```
FormatTests: llvm/lib/Support/MemoryBuffer.cpp:53:
void llvm::MemoryB
https://github.com/ilya-biryukov created
https://github.com/llvm/llvm-project/pull/131299
The `format` API receives a StringRef, but crashes whenever it is
non-null-terminated with the corresponding assertion:
```
FormatTests: llvm/lib/Support/MemoryBuffer.cpp:53:
void llvm::MemoryBuffer::init
27 matches
Mail list logo
