[lldb] Make ValueObject::Dereference less aggressive

labath · labath · commit fc40cf1387e3 · 2025-04-25T11:21:52.000+02:00
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-&gt;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-&gt;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.
diff --git 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
@@ -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();
   }
