[asan] Use InternalMmapVectorNoCtor for error_message_buffer, reallocate if needed #77488
Conversation
This file contains 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
…en the size of error_message_buffer is not enough
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Enna1 (Enna1) Changes…en the size of error_message_buffer is not enough Full diff: https://github.com/llvm/llvm-project/pull/77488.diff 1 Files Affected:
diff --git a/compiler-rt/lib/asan/asan_report.cpp b/compiler-rt/lib/asan/asan_report.cpp
index 7603e8131154ba..1cd21bfbc0bfeb 100644
--- a/compiler-rt/lib/asan/asan_report.cpp
+++ b/compiler-rt/lib/asan/asan_report.cpp
@@ -32,8 +32,7 @@ namespace __asan {
// -------------------- User-specified callbacks ----------------- {{{1
static void (*error_report_callback)(const char*);
-static char *error_message_buffer = nullptr;
-static uptr error_message_buffer_pos = 0;
+static InternalMmapVector<char> *error_message_buffer = nullptr;
static Mutex error_message_buf_mutex;
static const unsigned kAsanBuggyPcPoolSize = 25;
static __sanitizer::atomic_uintptr_t AsanBuggyPcPool[kAsanBuggyPcPoolSize];
@@ -42,17 +41,15 @@ void AppendToErrorMessageBuffer(const char *buffer) {
Lock l(&error_message_buf_mutex);
if (!error_message_buffer) {
error_message_buffer =
- (char*)MmapOrDieQuietly(kErrorMessageBufferSize, __func__);
- error_message_buffer_pos = 0;
+ new (GetGlobalLowLevelAllocator()) InternalMmapVector<char>();
+ error_message_buffer->reserve(kErrorMessageBufferSize);
+ error_message_buffer->push_back('\0');
}
- uptr length = internal_strlen(buffer);
- RAW_CHECK(kErrorMessageBufferSize >= error_message_buffer_pos);
- uptr remaining = kErrorMessageBufferSize - error_message_buffer_pos;
- internal_strncpy(error_message_buffer + error_message_buffer_pos,
- buffer, remaining);
- error_message_buffer[kErrorMessageBufferSize - 1] = '\0';
- // FIXME: reallocate the buffer instead of truncating the message.
- error_message_buffer_pos += Min(remaining, length);
+ uptr error_message_buffer_len = error_message_buffer->size() - 1;
+ uptr buffer_len = internal_strlen(buffer);
+ error_message_buffer->resize(error_message_buffer_len + buffer_len + 1);
+ internal_memcpy(error_message_buffer->data() + error_message_buffer_len,
+ buffer, buffer_len + 1);
}
// ---------------------- Helper functions ----------------------- {{{1
@@ -158,14 +155,14 @@ class ScopedInErrorReport {
// Copy the message buffer so that we could start logging without holding a
// lock that gets acquired during printing.
- InternalMmapVector<char> buffer_copy(kErrorMessageBufferSize);
+ InternalScopedString buffer_copy;
{
Lock l(&error_message_buf_mutex);
- internal_memcpy(buffer_copy.data(),
- error_message_buffer, kErrorMessageBufferSize);
+ buffer_copy.Append(error_message_buffer->data());
// Clear error_message_buffer so that if we find other errors
// we don't re-log this error.
- error_message_buffer_pos = 0;
+ error_message_buffer->clear();
+ error_message_buffer->push_back('\0');
}
LogFullErrorReport(buffer_copy.data());
|
|
Not sure if InternalScopedString is more appropriate for error_message_buffer, |
…ocate when the size of error_message_buffer is not enough" This reverts commit 37e6da6.
vitalybuka
reviewed
Jan 10, 2024
compiler-rt/lib/asan/asan_report.cpp
Outdated
| @@ -42,17 +41,15 @@ void AppendToErrorMessageBuffer(const char *buffer) { | |||
| Lock l(&error_message_buf_mutex); | |||
| if (!error_message_buffer) { | |||
| error_message_buffer = | |||
| (char*)MmapOrDieQuietly(kErrorMessageBufferSize, __func__); | |||
| error_message_buffer_pos = 0; | |||
| new (GetGlobalLowLevelAllocator()) InternalMmapVector<char>(); | |||
There was a problem hiding this comment.
instead of Allocator, can you use instead approach used for thread_registry_placeholder?
compiler-rt/lib/asan/asan_report.cpp
Outdated
| error_message_buffer_pos = 0; | ||
| new (GetGlobalLowLevelAllocator()) InternalMmapVector<char>(); | ||
| error_message_buffer->reserve(kErrorMessageBufferSize); | ||
| error_message_buffer->push_back('\0'); |
compiler-rt/lib/asan/asan_report.cpp
Outdated
| error_message_buffer_pos += Min(remaining, length); | ||
| uptr error_message_buffer_len = error_message_buffer->size() - 1; | ||
| uptr buffer_len = internal_strlen(buffer); | ||
| error_message_buffer->resize(error_message_buffer_len + buffer_len + 1); |
There was a problem hiding this comment.
error_message_buffer->Append() ?
I believe we need some refactoring to avoid dynamic initializer of InternalScopedString global. If not, please try thread_registry_placeholder approach |
|
Hi @vitalybuka , thanks for looking into this. This is because when asan reports an OOM, the PrintfAndReportCallback The following is the deadlock stack trace: So in the lastest patch, I manually call If we can provide a variant InternalMmapVector, which calls |
This reverts commit 3cf10f3.
…ocate when the size of error_message_buffer is not enough" Fix deadlock Address review comments
PTAL. |
|
ping |
|
ping :) |
|
ping @vitalybuka |
|
ping! BTW, do you think move But with this approach, there will be a behavior change:
WDYT? --- a/compiler-rt/lib/asan/asan_report.cpp
+++ b/compiler-rt/lib/asan/asan_report.cpp
@@ -24,6 +24,7 @@
// -------------------- User-specified callbacks ----------------- {{{1
static void (*error_report_callback)(const char*);
-static char *error_message_buffer = nullptr;
-static uptr error_message_buffer_pos = 0;
+typedef InternalMmapVectorNoCtor<char, true> ErrorMessageBuffer;
+static ErrorMessageBuffer *error_message_buf_ptr = nullptr;
static Mutex error_message_buf_mutex;
static const unsigned kAsanBuggyPcPoolSize = 25;
static __sanitizer::atomic_uintptr_t AsanBuggyPcPool[kAsanBuggyPcPoolSize];
void AppendToErrorMessageBuffer(const char *buffer) {
Lock l(&error_message_buf_mutex);
- if (!error_message_buffer) {
- error_message_buffer =
- (char*)MmapOrDieQuietly(kErrorMessageBufferSize, __func__);
- error_message_buffer_pos = 0;
- }
- uptr length = internal_strlen(buffer);
- RAW_CHECK(kErrorMessageBufferSize >= error_message_buffer_pos);
- uptr remaining = kErrorMessageBufferSize - error_message_buffer_pos;
- internal_strncpy(error_message_buffer + error_message_buffer_pos,
- buffer, remaining);
- error_message_buffer[kErrorMessageBufferSize - 1] = '\0';
- // FIXME: reallocate the buffer instead of truncating the message.
- error_message_buffer_pos += Min(remaining, length);
+ if (!error_message_buf_ptr)
+ return;
+ uptr error_message_buf_len = error_message_buf_ptr->size();
+ uptr buffer_len = internal_strlen(buffer);
+ error_message_buf_ptr->resize(error_message_buf_len + buffer_len);
+ internal_memcpy(error_message_buf_ptr->data() + error_message_buf_len, buffer,
+ buffer_len);
}
// ---------------------- Helper functions ----------------------- {{{1
@@ -130,6 +125,11 @@ class ScopedInErrorReport {
// We can lock them only here to avoid self-deadlock in case of
// recursive reports.
asanThreadRegistry().Lock();
+ {
+ Lock l(&error_message_buf_mutex);
+ error_message_buffer_.Initialize(kErrorMessageBufferSize);
+ error_message_buf_ptr = &error_message_buffer_;
+ }
Printf(
"=================================================================\n");
}
@@ -156,22 +156,16 @@ class ScopedInErrorReport {
if (common_flags()->print_module_map == 2)
DumpProcessMap();
- // Copy the message buffer so that we could start logging without holding a
- // lock that gets acquired during printing.
- InternalMmapVector<char> buffer_copy(kErrorMessageBufferSize);
{
Lock l(&error_message_buf_mutex);
- internal_memcpy(buffer_copy.data(),
- error_message_buffer, kErrorMessageBufferSize);
- // Clear error_message_buffer so that if we find other errors
- // we don't re-log this error.
- error_message_buffer_pos = 0;
+ error_message_buf_ptr = nullptr;
+ error_message_buffer_.push_back('\0');
}
- LogFullErrorReport(buffer_copy.data());
+ LogFullErrorReport(error_message_buffer_.data());
if (error_report_callback) {
- error_report_callback(buffer_copy.data());
+ error_report_callback(error_message_buffer_.data());
}
if (halt_on_error_ && common_flags()->abort_on_error) {
@@ -179,7 +173,7 @@ class ScopedInErrorReport {
// FIXME: implement "compact" error format, possibly without, or with
// highly compressed stack traces?
// FIXME: or just use the summary line as abort message?
- SetAbortMessage(buffer_copy.data());
+ SetAbortMessage(error_message_buffer_.data());
}
// In halt_on_error = false mode, reset the current error object (before
@@ -205,6 +199,7 @@ class ScopedInErrorReport {
private:
ScopedErrorReportLock error_report_lock_;
+ ErrorMessageBuffer error_message_buffer_;
// Error currently being reported. This enables the destructor to interact
// with the debugger and point it to an error description.
static ErrorDescription current_error_; |
|
ping :) @vitalybuka |
vitalybuka
reviewed
Feb 17, 2024
compiler-rt/lib/asan/asan_report.cpp
Outdated
| @@ -32,8 +33,11 @@ namespace __asan { | |||
|
|
|||
| // -------------------- User-specified callbacks ----------------- {{{1 | |||
| static void (*error_report_callback)(const char*); | |||
| static char *error_message_buffer = nullptr; | |||
| static uptr error_message_buffer_pos = 0; | |||
| typedef InternalMmapVectorNoCtor<char, true> ErrorMessageBuffer; | |||
vitalybuka
approved these changes
Feb 17, 2024
Enna1
changed the title
Enna1
changed the title
Enna1
added a commit
that referenced
this pull request
Feb 19, 2024
In #77488, a param `raw_report` is added for function `UnmapOrDie()`. But missing the corresponding change for fuchsia, causes the buildbot failure, see https://lab.llvm.org/buildbot/#/builders/98/builds/33593. This patch should fix the fuchsia buildbot failure.
|
Windows also needs a fix https://lab.llvm.org/buildbot/#/builders/127/builds/62395 |
|
Thanks, will done soon! |
Enna1
added a commit
that referenced
this pull request
Feb 19, 2024
In #77488, a param raw_report is added for function UnmapOrDie(), causes the Windows buildbot failure, see https://lab.llvm.org/buildbot/#/builders/127/builds/62395. This patch should fix the Windows buildbot failure.
Enna1
added a commit
that referenced
this pull request
Feb 19, 2024
In #77488, a param raw_report is added for function UnmapOrDie(), causes the Windows buildbot failure: - https://lab.llvm.org/buildbot/#/builders/127/builds/62395 - https://lab.llvm.org/buildbot/#/builders/265/builds/2662 This patch should fix the Windows buildbot failure.
PiJoules
added a commit
to PiJoules/llvm-project
that referenced
this pull request
Feb 22, 2024
As of llvm#77488, UnmapOrDie now accepts raw_report which allows the program to crash without calling Report(). We should propogate this value through UnmapOrDieVmar and have that call ReportMunmapFailureAndDie which uses `raw_report`.
PiJoules
added a commit
that referenced
this pull request
Feb 22, 2024
As of #77488, UnmapOrDie now accepts raw_report which allows the program to crash without calling Report(). We should propogate this value through UnmapOrDieVmar and have that call ReportMunmapFailureAndDie which uses `raw_report`.
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.
raw_reportwhich defaults to false for functionUnmapOrDie()like we do forMmapOrDie()raw_reportwhich defaults to false for classInternalMmapVectorNoCtor.raw_reportwill be passed toMmapOrDie()andUnmapOrDie()when they are called in classInternalMmapVectorNoCtormember functions.InternalMmapVectorNoCtor<char, true>forerror_message_bufferand reallocate if needed.