Skip to content

[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

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

JDevlieghere
Copy link
Member

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

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
@llvmbot
Copy link
Member

llvmbot commented Jan 15, 2025

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

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


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

1 Files Affected:

  • (modified) lldb/source/Symbol/CompilerType.cpp (+1-1)
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)
Copy link
Contributor

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

Copy link
Collaborator

@adrian-prantl adrian-prantl left a 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.

@JDevlieghere JDevlieghere merged commit 8965dd4 into llvm:main Jan 16, 2025
7 checks passed
@JDevlieghere JDevlieghere deleted the zero_byte_size branch January 16, 2025 17:34
JDevlieghere added a commit to swiftlang/llvm-project that referenced this pull request Jan 16, 2025
…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)
adrian-prantl added a commit to swiftlang/llvm-project that referenced this pull request Jan 16, 2025
…8965dd40c63c

[🍒 swift/release/6.1] [lldb] Handle a byte size of zero in CompilerType::GetValueAsScalar (llvm#123107)
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.

4 participants