[lldb] Make ValueObject::Dereference less aggressive #137311
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesThe 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. Full diff: https://github.com/llvm/llvm-project/pull/137311.diff 2 Files Affected:
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();
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
kuilpd
reviewed
Apr 25, 2025
fix typo Co-authored-by: Ilia Kuklin <[email protected]>
If that's what they want, yes.
Inside lldb - no. It's of course possible there's python code out there which depends on this bug, but I'm saying that's what it is -- a bug. For other operations, you have to choose whether you want to access the synthetic or non-synthetic view of the value. For example, this is what you get with a vector variable: This makes |
kuilpd
approved these changes
Apr 28, 2025
jimingham
approved these changes
Apr 28, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/21223 Here is the relevant piece of the build log for the reference |
gizmondo
pushed a commit
to gizmondo/llvm-project
that referenced
this pull request
Apr 29, 2025
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. --------- Co-authored-by: Ilia Kuklin <[email protected]>
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
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. --------- Co-authored-by: Ilia Kuklin <[email protected]>
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
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. --------- Co-authored-by: Ilia Kuklin <[email protected]>
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
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. --------- Co-authored-by: Ilia Kuklin <[email protected]>
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
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. --------- Co-authored-by: Ilia Kuklin <[email protected]>
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
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. --------- Co-authored-by: Ilia Kuklin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.