https://github.com/labath created
https://github.com/llvm/llvm-project/pull/137311
The function was always trying to dereference both the synthetic and
non-synthetic view of the object. This is wrong as the caller should be able to
determine which view of the object it wants to access, as is done e.g. for
child member access.
This patch removes the nonsynthetic->synthetic fallback, which is the more
surprising path, and fixes the callers to try both versions of the object (when
appropriate). I also snuck in simplification of the member access code path
because it was possible to use the same helper function for that, and I wanted
to be sure I understand the logic correctly.
I've left the synthetic->nonsynthetic fallback in place. I think we may want to
keep that one as we often have synthetic child providers for pointer types.
They usually don't provide an explicit dereference operation but I think users
would expect that a dereference operation on those objects would work. What we
may want to do is to try the *synthetic* operation first in this case, so that
the nonsynthetic case is really a fallback.
>From fc40cf1387e3ee825834e715cbe3b2742bfed3ce Mon Sep 17 00:00:00 2001
From: Pavel Labath <pa...@labath.sk>
Date: Fri, 25 Apr 2025 11:21:52 +0200
Subject: [PATCH] [lldb] Make ValueObject::Dereference less aggressive
The function was always trying to dereference both the synthetic
and non-synthetic view of the object. This is wrong as the caller should
be able to determine which view of the object it wants to access, as is
done e.g. for child member access.
This patch removes the nonsynthetic->synthetic fallback, which is the
more surprising path, and fixes the callers to try both versions of the
object (when appropriate). I also snuck in simplification of the member
access code path because it was possible to use the same helper function
for that, and I wanted to be sure I understand the logic correctly.
I've left the synthetic->nonsynthetic fallback in place. I think we may
want to keep that one as we often have synthetic child providers for
pointer types. They usually don't provide an explicit dereference
operation but I think users would expect that a dereference operation on
those objects would work. What we may want to do is to try the
*synthetic* operation first in this case, so that the nonsynthetic case
is really a fallback.
---
lldb/source/Target/StackFrame.cpp | 20 +--
lldb/source/ValueObject/ValueObject.cpp | 206 +++++++++---------------
2 files changed, 84 insertions(+), 142 deletions(-)
diff --git a/lldb/source/Target/StackFrame.cpp
b/lldb/source/Target/StackFrame.cpp
index 0306f68169a98..634cc35337121 100644
--- a/lldb/source/Target/StackFrame.cpp
+++ b/lldb/source/Target/StackFrame.cpp
@@ -709,21 +709,17 @@ ValueObjectSP
StackFrame::LegacyGetValueForVariableExpressionPath(
// we have a synthetic dereference specified.
if (!valobj_sp->IsPointerType() && valobj_sp->HasSyntheticValue()) {
Status deref_error;
- if (valobj_sp->GetCompilerType().IsReferenceType()) {
- valobj_sp = valobj_sp->GetSyntheticValue()->Dereference(deref_error);
- if (!valobj_sp || deref_error.Fail()) {
- error = Status::FromErrorStringWithFormatv(
- "Failed to dereference reference type: {0}", deref_error);
- return ValueObjectSP();
- }
+ if (ValueObjectSP synth_deref_sp =
+ valobj_sp->GetSyntheticValue()->Dereference(deref_error);
+ synth_deref_sp && deref_error.Success()) {
+ valobj_sp = std::move(synth_deref_sp);
}
-
- valobj_sp = valobj_sp->Dereference(deref_error);
if (!valobj_sp || deref_error.Fail()) {
error = Status::FromErrorStringWithFormatv(
"Failed to dereference synthetic value: {0}", deref_error);
return ValueObjectSP();
}
+
// Some synthetic plug-ins fail to set the error in Dereference
if (!valobj_sp) {
error =
@@ -1129,6 +1125,12 @@ ValueObjectSP
StackFrame::LegacyGetValueForVariableExpressionPath(
if (valobj_sp) {
if (deref) {
ValueObjectSP deref_valobj_sp(valobj_sp->Dereference(error));
+ if (!deref_valobj_sp && !no_synth_child) {
+ if (ValueObjectSP synth_obj_sp = valobj_sp->GetSyntheticValue()) {
+ error.Clear();
+ deref_valobj_sp = synth_obj_sp->Dereference(error);
+ }
+ }
valobj_sp = deref_valobj_sp;
} else if (address_of) {
ValueObjectSP address_of_valobj_sp(valobj_sp->AddressOf(error));
diff --git a/lldb/source/ValueObject/ValueObject.cpp
b/lldb/source/ValueObject/ValueObject.cpp
index 8741cb7343166..aeee12a132630 100644
--- a/lldb/source/ValueObject/ValueObject.cpp
+++ b/lldb/source/ValueObject/ValueObject.cpp
@@ -2202,6 +2202,45 @@ void ValueObject::GetExpressionPath(Stream &s,
}
}
+// Return the alternate value (synthetic if the input object is non-synthetic
+// and otherwise) this is permitted by the expression path options.
+static ValueObjectSP GetAlternateValue(
+ ValueObject &valobj,
+ ValueObject::GetValueForExpressionPathOptions::SyntheticChildrenTraversal
+ synth_traversal) {
+ using SynthTraversal =
+
ValueObject::GetValueForExpressionPathOptions::SyntheticChildrenTraversal;
+
+ if (valobj.IsSynthetic()) {
+ if (synth_traversal == SynthTraversal::FromSynthetic ||
+ synth_traversal == SynthTraversal::Both)
+ return valobj.GetNonSyntheticValue();
+ } else {
+ if (synth_traversal == SynthTraversal::ToSynthetic ||
+ synth_traversal == SynthTraversal::Both)
+ return valobj.GetSyntheticValue();
+ }
+ return nullptr;
+}
+
+// Dereference the provided object or the alternate value, ir permitted by the
+// expression path options.
+static ValueObjectSP DereferenceValueOrAlternate(
+ ValueObject &valobj,
+ ValueObject::GetValueForExpressionPathOptions::SyntheticChildrenTraversal
+ synth_traversal,
+ Status &error) {
+ error.Clear();
+ ValueObjectSP result = valobj.Dereference(error);
+ if (!result || error.Fail()) {
+ if (ValueObjectSP alt_obj = GetAlternateValue(valobj, synth_traversal)) {
+ error.Clear();
+ result = alt_obj->Dereference(error);
+ }
+ }
+ return result;
+}
+
ValueObjectSP ValueObject::GetValueForExpressionPath(
llvm::StringRef expression, ExpressionPathScanEndReason *reason_to_stop,
ExpressionPathEndResultType *final_value_type,
@@ -2234,7 +2273,8 @@ ValueObjectSP ValueObject::GetValueForExpressionPath(
: dummy_final_task_on_target) ==
ValueObject::eExpressionPathAftermathDereference) {
Status error;
- ValueObjectSP final_value = ret_val->Dereference(error);
+ ValueObjectSP final_value = DereferenceValueOrAlternate(
+ *ret_val, options.m_synthetic_children_traversal, error);
if (error.Fail() || !final_value.get()) {
if (reason_to_stop)
*reason_to_stop =
@@ -2348,144 +2388,45 @@ ValueObjectSP
ValueObject::GetValueForExpressionPath_Impl(
temp_expression = temp_expression.drop_front(); // skip . or >
size_t next_sep_pos = temp_expression.find_first_of("-.[", 1);
- if (next_sep_pos == llvm::StringRef::npos) // if no other separator just
- // expand this last layer
- {
+ if (next_sep_pos == llvm::StringRef::npos) {
+ // if no other separator just expand this last layer
llvm::StringRef child_name = temp_expression;
ValueObjectSP child_valobj_sp =
- root->GetChildMemberWithName(child_name);
-
- if (child_valobj_sp.get()) // we know we are done, so just return
- {
- *reason_to_stop =
- ValueObject::eExpressionPathScanEndReasonEndOfString;
- *final_result = ValueObject::eExpressionPathEndResultTypePlain;
- return child_valobj_sp;
- } else {
- switch (options.m_synthetic_children_traversal) {
- case GetValueForExpressionPathOptions::SyntheticChildrenTraversal::
- None:
- break;
- case GetValueForExpressionPathOptions::SyntheticChildrenTraversal::
- FromSynthetic:
- if (root->IsSynthetic()) {
- child_valobj_sp = root->GetNonSyntheticValue();
- if (child_valobj_sp.get())
- child_valobj_sp =
- child_valobj_sp->GetChildMemberWithName(child_name);
- }
- break;
- case GetValueForExpressionPathOptions::SyntheticChildrenTraversal::
- ToSynthetic:
- if (!root->IsSynthetic()) {
- child_valobj_sp = root->GetSyntheticValue();
- if (child_valobj_sp.get())
- child_valobj_sp =
- child_valobj_sp->GetChildMemberWithName(child_name);
- }
- break;
- case GetValueForExpressionPathOptions::SyntheticChildrenTraversal::
- Both:
- if (root->IsSynthetic()) {
- child_valobj_sp = root->GetNonSyntheticValue();
- if (child_valobj_sp.get())
- child_valobj_sp =
- child_valobj_sp->GetChildMemberWithName(child_name);
- } else {
- child_valobj_sp = root->GetSyntheticValue();
- if (child_valobj_sp.get())
- child_valobj_sp =
- child_valobj_sp->GetChildMemberWithName(child_name);
- }
- break;
- }
+ root->GetChildMemberWithName(child_name);
+ if (!child_valobj_sp) {
+ if (ValueObjectSP altroot = GetAlternateValue(
+ *root, options.m_synthetic_children_traversal))
+ child_valobj_sp = altroot->GetChildMemberWithName(child_name);
}
-
- // if we are here and options.m_no_synthetic_children is true,
- // child_valobj_sp is going to be a NULL SP, so we hit the "else"
- // branch, and return an error
- if (child_valobj_sp.get()) // if it worked, just return
- {
+ if (child_valobj_sp) {
*reason_to_stop =
ValueObject::eExpressionPathScanEndReasonEndOfString;
*final_result = ValueObject::eExpressionPathEndResultTypePlain;
return child_valobj_sp;
- } else {
- *reason_to_stop =
- ValueObject::eExpressionPathScanEndReasonNoSuchChild;
- *final_result = ValueObject::eExpressionPathEndResultTypeInvalid;
- return nullptr;
}
- } else // other layers do expand
- {
- llvm::StringRef next_separator = temp_expression.substr(next_sep_pos);
- llvm::StringRef child_name = temp_expression.slice(0, next_sep_pos);
+ *reason_to_stop = ValueObject::eExpressionPathScanEndReasonNoSuchChild;
+ *final_result = ValueObject::eExpressionPathEndResultTypeInvalid;
+ return nullptr;
+ }
- ValueObjectSP child_valobj_sp =
- root->GetChildMemberWithName(child_name);
- if (child_valobj_sp.get()) // store the new root and move on
- {
- root = child_valobj_sp;
- remainder = next_separator;
- *final_result = ValueObject::eExpressionPathEndResultTypePlain;
- continue;
- } else {
- switch (options.m_synthetic_children_traversal) {
- case GetValueForExpressionPathOptions::SyntheticChildrenTraversal::
- None:
- break;
- case GetValueForExpressionPathOptions::SyntheticChildrenTraversal::
- FromSynthetic:
- if (root->IsSynthetic()) {
- child_valobj_sp = root->GetNonSyntheticValue();
- if (child_valobj_sp.get())
- child_valobj_sp =
- child_valobj_sp->GetChildMemberWithName(child_name);
- }
- break;
- case GetValueForExpressionPathOptions::SyntheticChildrenTraversal::
- ToSynthetic:
- if (!root->IsSynthetic()) {
- child_valobj_sp = root->GetSyntheticValue();
- if (child_valobj_sp.get())
- child_valobj_sp =
- child_valobj_sp->GetChildMemberWithName(child_name);
- }
- break;
- case GetValueForExpressionPathOptions::SyntheticChildrenTraversal::
- Both:
- if (root->IsSynthetic()) {
- child_valobj_sp = root->GetNonSyntheticValue();
- if (child_valobj_sp.get())
- child_valobj_sp =
- child_valobj_sp->GetChildMemberWithName(child_name);
- } else {
- child_valobj_sp = root->GetSyntheticValue();
- if (child_valobj_sp.get())
- child_valobj_sp =
- child_valobj_sp->GetChildMemberWithName(child_name);
- }
- break;
- }
- }
+ llvm::StringRef next_separator = temp_expression.substr(next_sep_pos);
+ llvm::StringRef child_name = temp_expression.slice(0, next_sep_pos);
- // if we are here and options.m_no_synthetic_children is true,
- // child_valobj_sp is going to be a NULL SP, so we hit the "else"
- // branch, and return an error
- if (child_valobj_sp.get()) // if it worked, move on
- {
- root = child_valobj_sp;
- remainder = next_separator;
- *final_result = ValueObject::eExpressionPathEndResultTypePlain;
- continue;
- } else {
- *reason_to_stop =
- ValueObject::eExpressionPathScanEndReasonNoSuchChild;
- *final_result = ValueObject::eExpressionPathEndResultTypeInvalid;
- return nullptr;
- }
+ ValueObjectSP child_valobj_sp = root->GetChildMemberWithName(child_name);
+ if (!child_valobj_sp) {
+ if (ValueObjectSP altroot = GetAlternateValue(
+ *root, options.m_synthetic_children_traversal))
+ child_valobj_sp = altroot->GetChildMemberWithName(child_name);
}
- break;
+ if (child_valobj_sp) {
+ root = child_valobj_sp;
+ remainder = next_separator;
+ *final_result = ValueObject::eExpressionPathEndResultTypePlain;
+ continue;
+ }
+ *reason_to_stop = ValueObject::eExpressionPathScanEndReasonNoSuchChild;
+ *final_result = ValueObject::eExpressionPathEndResultTypeInvalid;
+ return nullptr;
}
case '[': {
if (!root_compiler_type_info.Test(eTypeIsArray) &&
@@ -2601,7 +2542,8 @@ ValueObjectSP ValueObject::GetValueForExpressionPath_Impl(
// a bitfield
pointee_compiler_type_info.Test(eTypeIsScalar)) {
Status error;
- root = root->Dereference(error);
+ root = DereferenceValueOrAlternate(
+ *root, options.m_synthetic_children_traversal, error);
if (error.Fail() || !root) {
*reason_to_stop =
ValueObject::eExpressionPathScanEndReasonDereferencingFailed;
@@ -2746,7 +2688,8 @@ ValueObjectSP ValueObject::GetValueForExpressionPath_Impl(
ValueObject::eExpressionPathAftermathDereference &&
pointee_compiler_type_info.Test(eTypeIsScalar)) {
Status error;
- root = root->Dereference(error);
+ root = DereferenceValueOrAlternate(
+ *root, options.m_synthetic_children_traversal, error);
if (error.Fail() || !root) {
*reason_to_stop =
ValueObject::eExpressionPathScanEndReasonDereferencingFailed;
@@ -2916,9 +2859,6 @@ ValueObjectSP ValueObject::Dereference(Status &error) {
}
}
- } else if (HasSyntheticValue()) {
- m_deref_valobj =
- GetSyntheticValue()->GetChildMemberWithName("$$dereference$$").get();
} else if (IsSynthetic()) {
m_deref_valobj = GetChildMemberWithName("$$dereference$$").get();
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
