[lldb] Make ValueObjectDynamicValue::UpdateValue() point to a host b… #125143
Conversation
|
@llvm/pr-subscribers-lldb Author: Augusto Noronha (augusto2112) Changes…uffer ValueObjectDynamicValue::UpdateValue() assumes that the dynamic type found by GetDynamicTypeAndAddress() would return an address in the inferior. This commit makes it so it can deal with being passed a host address instead. This is needed downstream by the Swift fork. rdar://143357274 Full diff: https://github.com/llvm/llvm-project/pull/125143.diff 2 Files Affected:
diff --git a/lldb/include/lldb/Target/LanguageRuntime.h b/lldb/include/lldb/Target/LanguageRuntime.h
index 4a0214b04e235e..08db8a17a67e69 100644
--- a/lldb/include/lldb/Target/LanguageRuntime.h
+++ b/lldb/include/lldb/Target/LanguageRuntime.h
@@ -105,7 +105,9 @@ class LanguageRuntime : public Runtime, public PluginInterface {
"language doesn't support getting vtable information");
}
- // this call should return true if it could set the name and/or the type
+ // This call should return true if it could set the name and/or the type.
+ // address can be either a legitimate address on the inferior, or an address
+ // in lldb, if value_type == HostAddress.
virtual bool GetDynamicTypeAndAddress(ValueObject &in_value,
lldb::DynamicValueType use_dynamic,
TypeAndOrName &class_type_or_name,
diff --git a/lldb/source/ValueObject/ValueObjectDynamicValue.cpp b/lldb/source/ValueObject/ValueObjectDynamicValue.cpp
index 588c644bbfd07b..10a5a9d0b76919 100644
--- a/lldb/source/ValueObject/ValueObjectDynamicValue.cpp
+++ b/lldb/source/ValueObject/ValueObjectDynamicValue.cpp
@@ -239,11 +239,19 @@ bool ValueObjectDynamicValue::UpdateValue() {
if (m_address.IsValid())
SetValueDidChange(true);
- // We've moved, so we should be fine...
- m_address = dynamic_address;
- lldb::TargetSP target_sp(GetTargetSP());
- lldb::addr_t load_address = m_address.GetLoadAddress(target_sp.get());
- m_value.GetScalar() = load_address;
+ // If we found a host address, point to the buffer in host memory.
+ // Later on this function will copy the buffer over.
+ if (value_type == Value::ValueType::HostAddress) {
+ m_value.GetScalar() = dynamic_address.GetOffset();
+ m_address = LLDB_INVALID_ADDRESS;
+ } else {
+ // Otherwise we have a legitimate address on the target. Point to the load
+ // address.
+ m_address = dynamic_address;
+ lldb::TargetSP target_sp(GetTargetSP());
+ lldb::addr_t load_address = m_address.GetLoadAddress(target_sp.get());
+ m_value.GetScalar() = load_address;
+ }
}
if (runtime)
@@ -258,7 +266,11 @@ bool ValueObjectDynamicValue::UpdateValue() {
LLDB_LOGF(log, "[%s %p] has a new dynamic type %s", GetName().GetCString(),
static_cast<void *>(this), GetTypeName().GetCString());
- if (m_address.IsValid() && m_dynamic_type_info) {
+ // m_address could be invalid but we could still have a local buffer
+ // containing the dynamic value.
+ if ((m_address.IsValid() ||
+ m_value.GetValueType() == Value::ValueType::HostAddress) &&
+ m_dynamic_type_info) {
// The variable value is in the Scalar value inside the m_value. We can
// point our m_data right to it.
m_error = m_value.GetValueAsData(&exe_ctx, m_data, GetModule().get());
|
|
I worry a bit about the fact that in the host case, GetValueAsData is going to end up calling: where address is the host data buffer and byte_size is the size of the new dynamic type. But in the case where the Value has data in the m_data_buffer, address points to a buffer. The code makes sure that the destination buffer (pointed to by dst) is big enough to fit byte_size, but I don't see the guarantee that the original contents are not smaller than the new dynamic type byte size. |
I implemented that check inside |
|
Maybe I'm missing something. In the host case, m_address is set to invalid, but m_value has the address in it and is a host address, so then we get to: // m_address could be invalid but we could still have a local buffer So that's going to try to call GetValueAsData on a value you've added the dynamic type info to, and that type could be bigger than the static value, causing us to read too much from the data buffer. |
What I mean is If you're still not comfortable to that I'm open to changing the API of |
|
I think it's worth making it as hard as possible to get the length you're supposed to read from that buffer right. |
|
This is a very common crash point. I suspect that in most of the cases it's because we flub whether something is a load or a host address, but I still don't want to add other mistakes we could make here. |
|
Ok, fair point. I'll come up with a more robust solution. |
|
Jim I updated the PR. |
|
Is there's no way to test this with the languages supported by the upstream lldb? |
|
Unfortunately I wasn't able to construct a test case that would trigger this with upstream lldb. |
|
What are the ownership rules for that reference to bytes that you are passing out? Who's keeping it alive and for how long? |
Since I could change Or are you asking me to clarify in the documentation that callers have to copy out the data if they want to own it? |
|
On Feb 3, 2025, at 3:48 PM, Augusto Noronha ***@***.***> wrote:
What are the ownership rules for that reference to bytes that you are passing out? Who's keeping it alive and for how long?
Since ValueObjectDynamicValue::UpdateValue immediately copies the buffer over I don't think that's a big concern...
I could change ArrayRef to an actual buffer that owns the data, but I think that would be a bit wasteful since UpdateValue always copies it anyway.
Or are you asking me to clarify in the documentation that callers have to copy out the data if they want to own it?
The latter.
Jim
… —
Reply to this email directly, view it on GitHub <#125143 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW4AHJQP55XVZ2J56BT2N755LAVCNFSM6AAAAABWGPIRJGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMZSGQZTENZXHE>.
You are receiving this because your review was requested.
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
augusto2112
force-pushed
the
host-buffer-dyn-valobj
branch
from
384ba61 to
50d39c0
Compare
|
This is a little weird - why would the value of a ValueObject when interpreted as its Dynamic Value ever be in host memory? That seems odd, made doubly so because it's unused in the current sources. So I really worry about this getting broken. Would it be possible to cons up a unittest that makes this artificially? |
|
To make sure I understand, you want me to write a unittest where I have a value object, and make sure it's dynamic value object child uses the local buffer to store the contents? What should I test? That it prints correctly? |
|
If possible, yes. It would be even more apropos if you could convince yourself that the dynamic type was bigger than the buffer for the static type, and you refused to read past the buffer...
Jim
… On Feb 4, 2025, at 3:12 PM, Augusto Noronha ***@***.***> wrote:
To make sure I understand, you want me to write a unittest where I have a value object, and make sure it's dynamic value object child uses the local buffer to store the contents? What should I test? That it prints correctly?
—
Reply to this email directly, view it on GitHub <#125143 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW4757UFB2E5GUBHVDL2OFCMVAVCNFSM6AAAAABWGPIRJGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMZVGI3TANBUGU>.
You are receiving this because your review was requested.
|
augusto2112
force-pushed
the
host-buffer-dyn-valobj
branch
from
04e392f to
441192a
Compare
|
Jim I was able to write a unit test. |
| @@ -0,0 +1,237 @@ | |||
| //===-- DumpValueObjectOptionsTests.cpp -----------------------------------===// | |||
augusto2112
force-pushed
the
host-buffer-dyn-valobj
branch
2 times, most recently
from
ceb193b to
e8124c1
Compare
…ffer ValueObjectDynamicValue::UpdateValue() assumes that the dynamic type found by GetDynamicTypeAndAddress() would return an address in the inferior. This commit makes it so it can deal with being passed a host address instead. This is needed downstream by the Swift fork. rdar://143357274
augusto2112
force-pushed
the
host-buffer-dyn-valobj
branch
from
e8124c1 to
937fdea
Compare
…llvm#125143) …uffer ValueObjectDynamicValue::UpdateValue() assumes that the dynamic type found by GetDynamicTypeAndAddress() would return an address in the inferior. This commit makes it so it can deal with being passed a host address instead. This is needed downstream by the Swift fork. rdar://143357274 (cherry picked from commit 0cbc498)
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/11097 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/197/builds/1717 Here is the relevant piece of the build log for the reference |
|
Please fix ASAP https://lab.llvm.org/buildbot/#/builders/18/builds/11097 |
|
I will push something shortly for this. |
…uffer
ValueObjectDynamicValue::UpdateValue() assumes that the dynamic type found by GetDynamicTypeAndAddress() would return an address in the inferior. This commit makes it so it can deal with being passed a host address instead.
This is needed downstream by the Swift fork.
rdar://143357274