[LLDB][Value] Require type size when reading a scalar #153386
Conversation
|
@llvm/pr-subscribers-lldb Author: nerix (Nerixyz) ChangesWhen reading a value as a scalar, the type size is required. It's returned as a This came up in the Shell/Process/Windows/msstl_smoke.cpp test. There, LLDB breaks at the function entry, so all locals aren't initialized yet. Most values will contain garbage. The Full diff: https://github.com/llvm/llvm-project/pull/153386.diff 1 Files Affected:
diff --git a/lldb/source/Core/Value.cpp b/lldb/source/Core/Value.cpp
index 028f0587c5790..00750f51693d1 100644
--- a/lldb/source/Core/Value.cpp
+++ b/lldb/source/Core/Value.cpp
@@ -347,6 +347,9 @@ Status Value::GetValueAsData(ExecutionContext *exe_ctx, DataExtractor &data,
else
data.SetAddressByteSize(sizeof(void *));
+ if (!type_size)
+ return Status::FromErrorString("type does not have a size");
+
uint32_t result_byte_size = *type_size;
if (m_value.GetData(data, result_byte_size))
return error; // Success;
|
| @@ -347,6 +347,9 @@ Status Value::GetValueAsData(ExecutionContext *exe_ctx, DataExtractor &data, | |||
| else | |||
| data.SetAddressByteSize(sizeof(void *)); | |||
|
|
|||
| if (!type_size) | |||
| return Status::FromErrorString("type does not have a size"); | |||
There was a problem hiding this comment.
Looks like this is a regression from (#151350):
Author: Ilia Kuklin <ikuklin@accesssoftek.com>
Date: Wed Aug 6 14:32:19 2025 +0500
[lldb] Add `ValueObject::CreateValueObjectFromScalar` and fix `Scalar::GetData` (#151350)
Add `ValueObject::CreateValueObjectFromScalar` function and adjust
`Scalar::GetData` to be able to both extend and truncate the data bytes
in Scalar to the specified size.
Previously we would return an error if type_size was unset.
So this pretty much brings us back to previous behaviour.
Any way we can test this in lldb/unittests/Utility/ScalarTest.cpp? I guess the tricky bit will be creating a type with invalid byte_size. Maybe with an incomplete AST type? Haven't looked at what's available in that test though
There was a problem hiding this comment.
I don't know about the rest, but testing something about the ValueObject class inside ScalarTest.cpp sounds very wrong.
There was a problem hiding this comment.
Might be missing something but I was thinking just testing Value::GetValueAsData. Which we could accomplish by creating a Scalar.
The PR I linked was fixing something for ValueObject, but did so by adjusting something in Value. Maybe that's where the confusion came from?
There was a problem hiding this comment.
I don't think we need an incomplete type here. A value without a compiler type should suffice. So something like
Value v(Scalar(42));
DataExtractor extractor;
Status status = v.GetValueAsData(nullptr, extractor, nullptr);
ASSERT_TRUE(status.Fail());I could add a file in unittests/ValueObject. There are no unittests that call GetValueAsData yet.
Some functions don't have the `FunctionType` set. We can't look these up and won't be able to call them, so ignore them when caching the function names. This does fix the failures caused by #153160 mentioned in #153160 (comment). However, in `lldb-shell::msstl_smoke.cpp` there's another failure not introduced by #153160 (fixed with #153386).
When reading a value as a scalar, the type size is required. It's returned as a
std::optional. This optional isn't checked for scalar values, where it is unconditionally accessed.This came up in the Shell/Process/Windows/msstl_smoke.cpp test. There, LLDB breaks at the function entry, so all locals aren't initialized yet. Most values will contain garbage. The
std::listsynthetic provider tries to read the value usingGetData. However, inValueObject::GetData,ValueObjectChild::UpdateValuefails because the parent already failed to read its data, som_valuewon't have a compiler type, thus the size can't be read.