-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[lldb] Handle a byte size of zero in CompilerType::GetValueAsScalar #123107
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
A bit or byte size of 0 is not a bug. It can legitimately (and frequently) happen in Swift and C, just not in C++. However, it doesn't make sense to read a scalar of zero bytes. Currently, when this happens, we trigger an lldb_assert in the data extractor and return 0, which isn't accurate. I only a bunch of reports of the lldb_assert triggering but no actual example that I could turn into a test. rdar://141630334
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesA bit or byte size of 0 is not a bug. It can legitimately (and frequently) happen in Swift and C, just not in C++. However, it doesn't make sense to read a scalar of zero bytes. Currently, when this happens, we trigger an rdar://141630334 Full diff: https://github.com/llvm/llvm-project/pull/123107.diff 1 Files Affected:
diff --git a/lldb/source/Symbol/CompilerType.cpp b/lldb/source/Symbol/CompilerType.cpp
index e9e6e3bf2600ce..3c4390b89a125a 100644
--- a/lldb/source/Symbol/CompilerType.cpp
+++ b/lldb/source/Symbol/CompilerType.cpp
@@ -1105,7 +1105,7 @@ bool CompilerType::GetValueAsScalar(const lldb_private::DataExtractor &data,
return false;
std::optional<uint64_t> byte_size = GetByteSize(exe_scope);
- if (!byte_size)
+ if (!byte_size || *byte_size == 0)
return false;
lldb::offset_t offset = data_byte_offset;
switch (encoding) {
|
@@ -1105,7 +1105,7 @@ bool CompilerType::GetValueAsScalar(const lldb_private::DataExtractor &data, | |||
return false; | |||
|
|||
std::optional<uint64_t> byte_size = GetByteSize(exe_scope); | |||
if (!byte_size) | |||
if (!byte_size || *byte_size == 0) |
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.
Nit: I would add a comment with brief context on how this appears invalid. Otherwise LGTM
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.
Either returning true or false is perfectly fine. The only argument I can come up for returning true is that now all call sites need to replicate this check as to not call this function with a size of 0.
…lvm#123107) A bit or byte size of 0 is not a bug. It can legitimately (and frequently) happen in Swift and C, just not in C++. However, it doesn't make sense to read a scalar of zero bytes. Currently, when this happens, we trigger an `lldb_assert` in the data extractor and return 0, which isn't accurate. I have a bunch of reports of the assert triggering, but nobody has been able to provide me with a reproducer that I can turn into a test and I wasn't able to concoct a test case by reverse-engineering the code. rdar://141630334 (cherry picked from commit 8965dd4)
…8965dd40c63c [🍒 swift/release/6.1] [lldb] Handle a byte size of zero in CompilerType::GetValueAsScalar (llvm#123107)
A bit or byte size of 0 is not a bug. It can legitimately (and frequently) happen in Swift and C, just not in C++. However, it doesn't make sense to read a scalar of zero bytes.
Currently, when this happens, we trigger an
lldb_assert
in the data extractor and return 0, which isn't accurate. I have a bunch of reports of the assert triggering, but nobody has been able to provide me with a reproducer that I can turn into a test and I wasn't able to concoct a test case by reverse-engineering the code.rdar://141630334