Skip to content

[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

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

Jlalond
Copy link
Contributor

@Jlalond Jlalond commented Jun 24, 2024

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.

@Jlalond Jlalond requested a review from JDevlieghere as a code owner June 24, 2024 22:00
@Jlalond Jlalond changed the title [LLDB][Minidum] Change expected directories to the correct type; size_t [LLDB][Minidump] Change expected directories to the correct type; size_t Jun 24, 2024
@llvmbot llvmbot added the lldb label Jun 24, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 24, 2024

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/96564.diff

2 Files Affected:

  • (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp (+1-1)
  • (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h (+1-1)
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

Copy link
Member

@petrhosek petrhosek left a 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!

@Jlalond Jlalond merged commit 361543e into llvm:main Jun 24, 2024
6 of 7 checks passed
Jlalond added a commit that referenced this pull request Jun 26, 2024
![image](https://github.com/llvm/llvm-project/assets/25160653/2044cc8e-72d5-49ec-9439-256555f5fd2b)

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.
@DavidSpickett
Copy link
Collaborator

DavidSpickett commented Jun 26, 2024

size_t will be 32 bit on 32 bit platforms, is this ok given the expected range of this variable?

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.

@Jlalond
Copy link
Contributor Author

Jlalond commented Jun 26, 2024

@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 size_t max, it would be a corrupted minidump

AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
…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.
AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
![image](https://github.com/llvm/llvm-project/assets/25160653/2044cc8e-72d5-49ec-9439-256555f5fd2b)

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.
@Jlalond Jlalond deleted the minidump-uint-fix branch August 6, 2024 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants