-
Notifications
You must be signed in to change notification settings - Fork 14.2k
[LLDB][Minidump] Change expected directories to the correct type; size_t #96564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesIn !95312 I incorrectly set
Full diff: https://github.com/llvm/llvm-project/pull/96564.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 7a09c6104d08c..de212c6b20da7 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -109,7 +109,7 @@ Status MinidumpFileBuilder::AddDirectory(StreamType type,
if (m_directories.size() + 1 > m_expected_directories) {
error.SetErrorStringWithFormat(
"Unable to add directory for stream type %x, exceeded expected number "
- "of directories %d.",
+ "of directories %zu.",
(uint32_t)type, m_expected_directories);
return error;
}
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index b606f925f9912..20564e0661f2a 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -146,7 +146,7 @@ class MinidumpFileBuilder {
lldb_private::DataBufferHeap m_data;
lldb::ProcessSP m_process_sp;
- uint m_expected_directories = 0;
+ size_t m_expected_directories = 0;
uint64_t m_saved_data_size = 0;
lldb::offset_t m_thread_list_start = 0;
// We set the max write amount to 128 mb, this is arbitrary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for a quick fix!
 In #95312 uint and `#include <unistd.h>` were introduced. These broke the windows build. I addressed uint in #96564, but include went unfixed. So I acquired myself a windows machine to validate.
I don't see any failures on 32 bit Arm right now but worth asking in case it causes flaky tests later trying to read 64 bit minidumps. |
@DavidSpickett We'll be fine with 32b size_t. Directories in minidumps have to all have their offsets be 32b addressable, so if you were to fill the directory list up to the |
…e_t (llvm#96564) In llvm#95312 I incorrectly set `m_expected_directories` to uint, this broke the windows build and is the incorrect type. `size_t` is more accurate because this value only ever represents the expected upper bound of the directory vector.
 In llvm#95312 uint and `#include <unistd.h>` were introduced. These broke the windows build. I addressed uint in llvm#96564, but include went unfixed. So I acquired myself a windows machine to validate.
In #95312 I incorrectly set
m_expected_directories
to uint, this broke the windows build and is the incorrect type.size_t
is more accurate because this value only ever represents the expected upper bound of the directory vector.