[lldb] Fix member access in GetExpressionPath

[tonkosi]

Summary

This change fixes two issues in ValueObject::GetExpressionPath method:

1. Accessing members of struct references used to produce expression paths such as "str.&str.member" (instead of the expected "str.member"). This is fixed by assigning the flag tha the child value is a dereference when calling Dereference() on references and adjusting logic in expression path creation.
2. If the parent of member access is dereference, the produced expression path was "*(ptr).member". This is incorrect, since it dereferences the member instead of the pointer. This is fixed by wrapping dereference expression into parenthesis, resulting with "(*ptr).member".

[werat]

Nice fix! It would be nice to remove redundant parenthesis, e.g. (*(ptr)).member -> (*ptr).member, but I see this is non-trivial with the current logic.

[tonkosi]

Forgot to propagate "epformat" when calling GetExpressionPath

[clayborg]

Nice!

[mib]

Hi @tonkosi! I think your patch broke the macOS bot: https://green.lab.llvm.org/green/job/lldb-cmake/47258/console

Failed Tests (5):
  lldb-api :: commands/frame/diagnose/array/TestArray.py
  lldb-api :: commands/frame/diagnose/bad-reference/TestBadReference.py
  lldb-api :: commands/frame/diagnose/complicated-expression/TestComplicatedExpression.py
  lldb-api :: commands/frame/diagnose/dereference-argument/TestDiagnoseDereferenceArgument.py
  lldb-api :: commands/frame/diagnose/dereference-this/TestDiagnoseDereferenceThis.py

Let me know if you need help reproducing and fixing these issues, otherwise I'll have to revert your patch.

[werat]

In D132734#3827245, @mib wrote:

Let me know if you need help reproducing and fixing these issues, otherwise I'll have to revert your patch.

Sorry for the breaking the tests. We won't be able to look into this until next week, I've reverted the change for now.

[mib]

In D132734#3827361, @werat wrote:

In D132734#3827245, @mib wrote:

Let me know if you need help reproducing and fixing these issues, otherwise I'll have to revert your patch.

Sorry for the breaking the tests. We won't be able to look into this until next week, I've reverted the change for now.

Thank you @werat

[tonkosi]

Fixed the issue of printing unnecessary parenthesis, which broke API tests on macOS.

Improved use of parenthesis to produce outputs such as (*ptr).member instead of (*(ptr)).member.

[tonkosi]

Hi, I fixed the issue causing macOS tests to fail, but I don't have a macOS machine to verify it actually works. I'd be grateful if someone could check that part.

I also decided to further reduce number of parenthesis in output (e.g. (*(ptr)).member isn't ideal), but now I realize that the previous *(a.b.c) might be more clear than *a.b.c. Let me know if I should revert that part.

[JDevlieghere]

Hey, this patch still breaks TestArray.py. Could you please take another look?

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/47748/testReport/lldb-api/commands_frame_diagnose_array/TestArray_py/

The issue is:

a->c = <parent failed to evaluate: parent is NULL>

[hawkinsw]

lldb/source/Core/ValueObject.cpp

1947

I am *absolutely* not an expert here but I've spent several hours trying to debug why this incredibly thorough, well-written patch does not handle the case in TestArray.py. It would seem to me that this is the check that we forgot to bring over in to the new logic. I was attempting to be able to have an updated version of the patch by tonight, but didn't get further than just debugging. I will continue to work on it -- I am sure that you gurus have other, more important things to work on.

That said, I am just trying to help so if I am totally off base, please let me know!

Sincerely,
Will

[hawkinsw]

I updated this revision with the following change and I *think* that things are happy again:

diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index 226a2c3f690f..1f939c6fa2cd 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -2004,6 +2004,11 @@ void ValueObject::GetExpressionPath(Stream &s,
       s.PutChar(')');
   }
 
+  if (m_flags.m_is_array_item_for_pointer &&
+      epformat == eGetExpressionPathFormatHonorPointers) {
+    s.PutCString(m_name.GetStringRef());
+  }
+
   if (print_obj_access)
     s.PutChar('.');
   if (print_ptr_access)

Let me know if that is helpful.

Sincerely,
Will

[hawkinsw]

If you would like, I'd be happy to reopen this patch as a separate issue if that's the better way to handle it! Sorry for the SPAM!

Will

[JDevlieghere]

Thanks Will, let me apply this locally and see if that addresses the issue for which I reverted the original patch.

[tonkosi]

Thanks for the fix Will!

Just one question: cpp test file contains the expression a[10].c and the test complains about the expression path a->c.
If I understand correctly, with the latest fix, that would become a[10]->c, which is not the original expression (object access vs pointer access).
Was it like that before or did I mess up something else as well? (the test expects only the substring "a[10]" so I guess it's possible it was like that before)

lldb/source/Core/ValueObject.cpp

1947

Whoops, nice catch. I somehow deleted this part when refactoring the code and forgot about it.

Thanks for looking into this and fixing it, Will :D

[hawkinsw]

Mr. Sabolčec,

Thank you for the kind response -- I hope my work was helpful! It was lots of fun to dig in to this part of the codebase!

Sincerely,
Will

[werat]

In D132734#3887018, @JDevlieghere wrote:

Thanks Will, let me apply this locally and see if that addresses the issue for which I reverted the original patch.

Hy Jonas, sorry for bumping an old thread, but did you manage to check if the latest revision fixes the issue?

lldb/source/Core/ValueObject.cpp

if (llvm::Optional<CompilerType> complete_type =
       m_override_type = *complete_type;
       if (m_override_type.IsValid())
         return m_override_type;
     }
   }
   return compiler_type;
 }
 
-
-
 DataExtractor &ValueObject::GetDataExtractor() {
   UpdateValueIfNeeded(false);
   return m_data;
 }
 
 const Status &ValueObject::GetError() {
   UpdateValueIfNeeded(false);
   return m_error;
if (!root) {
       if (index_of_error)
         *index_of_error = idx;
       return root;
     }
   }
   return root;
 }
 
-lldb::ValueObjectSP ValueObject::GetChildAtIndexPath(
-  llvm::ArrayRef<std::pair<size_t, bool>> idxs, size_t *index_of_error) {
+lldb::ValueObjectSP
+ValueObject::GetChildAtIndexPath(llvm::ArrayRef<std::pair<size_t, bool>> idxs,
+                                 size_t *index_of_error) {
   if (idxs.size() == 0)
     return GetSP();
   ValueObjectSP root(GetSP());
   for (std::pair<size_t, bool> idx : idxs) {
     root = root->GetChildAtIndex(idx.first, idx.second);
     if (!root) {
       if (index_of_error)
         *index_of_error = idx.first;
if (flags.AnySet(eTypeIsArray | eTypeIsPointer) &&
       if (IsCStringContainer(true) &&
           (custom_format == eFormatCString ||
            custom_format == eFormatCharArray || custom_format == eFormatChar ||
            custom_format ==
                eFormatVectorOfChar)) // print char[] & char* directly
       {
         Status error;
         lldb::WritableDataBufferSP buffer_sp;
-        std::pair<size_t, bool> read_string = ReadPointedString(
-            buffer_sp, error, 0, (custom_format == eFormatVectorOfChar) ||
-                                     (custom_format == eFormatCharArray));
+        std::pair<size_t, bool> read_string =
+            ReadPointedString(buffer_sp, error, 0,
+                              (custom_format == eFormatVectorOfChar) ||
         lldb_private::formatters::StringPrinter::
             ReadBufferAndDumpToStreamOptions options(*this);
         options.SetData(DataExtractor(
             buffer_sp, lldb::eByteOrderInvalid,
             8)); // none of this matters for a string - pass some defaults
         options.SetStream(&s);
         options.SetPrefixToken(nullptr);
         options.SetQuote('"');
bool ValueObject::SetValueFromCString(const char *value_str, Status &error) {
   return true;
 }
 
 bool ValueObject::GetDeclaration(Declaration &decl) {
   decl.Clear();
   return false;
 }
 
-void ValueObject::AddSyntheticChild(ConstString key,
-                                    ValueObject *valobj) {
+void ValueObject::AddSyntheticChild(ConstString key, ValueObject *valobj) {
   m_synthetic_children[key] = valobj;
 }
 
 ValueObjectSP ValueObject::GetSyntheticChild(ConstString key) const {
   ValueObjectSP synthetic_child_sp;
   std::map<ConstString, ValueObject *>::const_iterator pos =
       m_synthetic_children.find(key);
   if (pos != m_synthetic_children.end())
if (CanProvideValue()) {
       s.Printf("((%s)%s)", GetTypeName().AsCString("void"),
                GetValueAsCString());
       return;
     }
 
     return;
   }
 
-  const bool is_deref_of_parent = IsDereferenceOfParent();
+  // Checks whether a value is dereference of a non-reference parent.
+  // This is used to determine whether to print a dereference operation (*).
+  auto is_deref_of_non_reference = [](ValueObject *value) {
+    if (value == nullptr)
+      return false;
+    ValueObject *value_parent = value->GetParent();
+    if (value_parent) {
+      CompilerType parent_compiler_type = value_parent->GetCompilerType();
+      if (parent_compiler_type) {
+        const uint32_t parent_type_info = parent_compiler_type.GetTypeInfo();
+        if (parent_type_info & eTypeIsReference)
+          return false;
+      }
+    }
+    return value->IsDereferenceOfParent();
+  };
 
-  if (is_deref_of_parent &&
+  ValueObject *parent = GetParent();
+
+  if (is_deref_of_non_reference(this) &&
       epformat == eGetExpressionPathFormatDereferencePointers) {
     // this is the original format of GetExpressionPath() producing code like
     // *(a_ptr).memberName, which is entirely fine, until you put this into
     // StackFrame::GetValueForVariableExpressionPath() which prefers to see
-    // a_ptr->memberName. the eHonorPointers mode is meant to produce strings
-    // in this latter format
-    s.PutCString("*(");
-  }
-
-  ValueObject *parent = GetParent();
-
+    // a_ptr->memberName. The eHonorPointers mode is meant to produce strings
+    // in this latter format.
+    s.PutChar('*');
     if (parent)
       parent->GetExpressionPath(s, epformat);
+    return;
+  }
 
-  // if we are a deref_of_parent just because we are synthetic array members
-  // made up to allow ptr[%d] syntax to work in variable printing, then add our
-  // name ([%d]) to the expression path
-  if (m_flags.m_is_array_item_for_pointer &&
-      epformat == eGetExpressionPathFormatHonorPointers)
-    s.PutCString(m_name.GetStringRef());
+  const bool is_deref_of_parent = IsDereferenceOfParent();
+  bool is_parent_deref_of_non_reference = false;
+  bool print_obj_access = false;
+  bool print_ptr_access = false;
 
-  if (!IsBaseClass()) {
-    if (!is_deref_of_parent) {
-      ValueObject *non_base_class_parent = GetNonBaseClassParent();
-      if (non_base_class_parent &&
-          !non_base_class_parent->GetName().IsEmpty()) {
-        CompilerType non_base_class_parent_compiler_type =
-            non_base_class_parent->GetCompilerType();
-        if (non_base_class_parent_compiler_type) {
-          if (parent && parent->IsDereferenceOfParent() &&
-              epformat == eGetExpressionPathFormatHonorPointers) {
-            s.PutCString("->");
-          } else {
-            const uint32_t non_base_class_parent_type_info =
-                non_base_class_parent_compiler_type.GetTypeInfo();
+  if (!IsBaseClass() && !is_deref_of_parent) {
+    if (non_base_class_parent && !non_base_class_parent->GetName().IsEmpty()) {
+          print_ptr_access = true;
 
-            if (non_base_class_parent_type_info & eTypeIsPointer) {
-              s.PutCString("->");
-            } else if ((non_base_class_parent_type_info & eTypeHasChildren) &&
-                       !(non_base_class_parent_type_info & eTypeIsArray)) {
-              s.PutChar('.');
-            }
-          }
-        }
-      }
+            print_ptr_access = true;
+            print_obj_access = true;
+          }
+      is_parent_deref_of_non_reference =
+          is_deref_of_non_reference(non_base_class_parent) &&
+          epformat == eGetExpressionPathFormatDereferencePointers;
 
-      const char *name = GetName().GetCString();
-      if (name)
-        s.PutCString(name);
-    }
-  }
+  if (parent) {
+    // The parent should be wrapped in parenthesis when doing a member access.
+    // This prevents forming incorrect expressions such as *(ptr).field,
+    // which dereferences the field instead of the ptr, and constructs the
+    // expression in the format (*(ptr)).field. To create expressions compatible
+    // with StackFrame::GetValueForVariableExpressionPath() and reduce amount of
+    // unnecessary parenthesis, this is done only when the parent has the
+    // dereference syntax *(parent).
+    const bool wrap_parent_in_parens = (print_obj_access || print_ptr_access) &&
+                                       is_parent_deref_of_non_reference;
+    if (wrap_parent_in_parens)
+      s.PutChar('(');
+    parent->GetExpressionPath(s, epformat);
+    if (wrap_parent_in_parens)
+      s.PutChar(')');
+  }
+
+  if (print_obj_access)
+    s.PutChar('.');
+  if (print_ptr_access)
+    s.PutCString("->");
+
+  if (!IsBaseClass() && !is_deref_of_parent) {
 
-  if (is_deref_of_parent &&
-      epformat == eGetExpressionPathFormatDereferencePointers) {
-    s.PutChar(')');
-  }
-}
-
 ValueObjectSP ValueObject::GetValueForExpressionPath(
     llvm::StringRef expression, ExpressionPathScanEndReason *reason_to_stop,
     ExpressionPathEndResultType *final_value_type,
     const GetValueForExpressionPathOptions &options,
     ExpressionPathAftermath *final_task_on_target) {
 
   ExpressionPathScanEndReason dummy_reason_to_stop =
       ValueObject::eExpressionPathScanEndReasonUnknown;

 bool ValueObject::CanProvideValue() {
   // we need to support invalid types as providers of values because some bare-
   // board debugging scenarios have no notion of types, but still manage to
   // have raw numeric values for things like registers. sigh.
   CompilerType type = GetCompilerType();
   return (!type.IsValid()) || (0 != (type.GetTypeInfo() & eTypeHasValue));
 }
 
-
-
 ValueObjectSP ValueObject::Persist() {
   if (!UpdateValueIfNeeded())
     return nullptr;
 
   TargetSP target_sp(GetTargetSP());
   if (!target_sp)
     return nullptr;
 

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

if (idx_is_valid) {
         bool tmp_child_is_deref_of_parent = false;
         return pointee_clang_type.GetChildCompilerTypeAtIndex(
             exe_ctx, idx, transparent_pointers, omit_empty_base_classes,
             ignore_array_bounds, child_name, child_byte_size, child_byte_offset,
             child_bitfield_bit_size, child_bitfield_bit_offset,
             child_is_base_class, tmp_child_is_deref_of_parent, valobj,
             language_flags);
       } else {
+        child_is_deref_of_parent = true;
+
         const char *parent_name =
             valobj ? valobj->GetName().GetCString() : nullptr;
         if (parent_name) {
           child_name.assign(1, '&');
           child_name += parent_name;
         }
 
         // We have a pointer to an simple type

lldb/test/API/functionalities/data-formatter/data-formatter-synth/TestDataFormatterSynth.py

def test_with_run_command(self):
         self.expect('frame variable plenty_of_stuff --synthetic-type no',
                     substrs=['some_values = 0x',
                              'array = 0x',
                              'array_size = 5'])
 
         # check flat printing with synthetic children
         self.expect('frame variable plenty_of_stuff --flat',
                     substrs=['plenty_of_stuff.bitfield = 17',
-                             '*(plenty_of_stuff.array) = 5',
-                             '*(plenty_of_stuff.array) = 3'])
+                             '*plenty_of_stuff.array = 5',
+                             '*plenty_of_stuff.array = 3'])
 
         # check that we do not lose location information for our children
         self.expect('frame variable plenty_of_stuff --location',
                     substrs=['0x',
                              ':   bitfield = 17'])
 
         # check we work across pointer boundaries
         self.expect('frame variable plenty_of_stuff.some_values --ptr-depth=1',

lldb/test/API/python_api/expression_path/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/python_api/expression_path/TestExpressionPath.py

+"""Test that SBFrame::GetExpressionPath construct valid expressions"""
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class SBValueGetExpressionPathTest(TestBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def path(self, value):
+        """Constructs the expression path given the SBValue"""
+        if not value:
+            return None
+        stream = lldb.SBStream()
+        if not value.GetExpressionPath(stream):
+            return None
+        return stream.GetData()
+
+    def test_expression_path(self):
+        """Test that SBFrame::GetExpressionPath construct valid expressions"""
+        self.build()
+        self.setTearDownCleanup()
+
+        exe = self.getBuildArtifact("a.out")
+
+        # Create the target
+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target, VALID_TARGET)
+
+        # Set the breakpoints
+        breakpoint = target.BreakpointCreateBySourceRegex(
+            'Set breakpoint here', lldb.SBFileSpec("main.cpp"))
+        self.assertTrue(breakpoint.GetNumLocations() > 0, VALID_BREAKPOINT)
+
+        # Launch the process, and do not stop at the entry point.
+        process = target.LaunchSimple(
+            None, None, self.get_process_working_directory())
+
+        self.assertTrue(process, PROCESS_IS_VALID)
+
+        # Frame #0 should be at our breakpoint.
+        threads = lldbutil.get_threads_stopped_at_breakpoint(
+            process, breakpoint)
+
+        self.assertEquals(len(threads), 1)
+        self.thread = threads[0]
+        self.frame = self.thread.frames[0]
+        self.assertTrue(self.frame, "Frame 0 is valid.")
+
+        # Find "b" variables in frame
+        b = self.frame.FindVariable("b")
+        bp = self.frame.FindVariable("b_ptr")
+        br = self.frame.FindVariable("b_ref")
+        bpr = self.frame.FindVariable("b_ptr_ref")
+        # Check expression paths
+        self.assertEqual(self.path(b), "b")
+        self.assertEqual(self.path(bp), "b_ptr")
+        self.assertEqual(self.path(br), "b_ref")
+        self.assertEqual(self.path(bpr), "b_ptr_ref")
+
+        # Dereference "b" pointers
+        bp_deref = bp.Dereference()
+        bpr_deref = bpr.Dereference()  # a pointer
+        bpr_deref2 = bpr_deref.Dereference()  # two Dereference() calls to get object
+        # Check expression paths
+        self.assertEqual(self.path(bp_deref), "*b_ptr")
+        self.assertEqual(self.path(bpr_deref), "b_ptr_ref")
+        self.assertEqual(self.path(bpr_deref2), "*b_ptr_ref")
+
+        # Access "b" members and check expression paths
+        self.assertEqual(self.path(b.GetChildMemberWithName("x")), "b.x")
+        self.assertEqual(self.path(bp.GetChildMemberWithName("x")), "b_ptr->x")
+        self.assertEqual(self.path(br.GetChildMemberWithName("x")), "b_ref.x")
+        self.assertEqual(self.path(bp_deref.GetChildMemberWithName("x")), "(*b_ptr).x")
+        self.assertEqual(self.path(bpr_deref.GetChildMemberWithName("x")), "b_ptr_ref->x")
+        self.assertEqual(self.path(bpr_deref2.GetChildMemberWithName("x")), "(*b_ptr_ref).x")
+        # TODO: Uncomment once accessing members on pointer references is supported.
+        # self.assertEqual(self.path(bpr.GetChildMemberWithName("x")), "b_ptr_ref->x")
+
+        # Try few expressions with multiple member access
+        bp_ar_x = bp.GetChildMemberWithName("a_ref").GetChildMemberWithName("x")
+        br_ar_y = br.GetChildMemberWithName("a_ref").GetChildMemberWithName("y")
+        self.assertEqual(self.path(bp_ar_x), "b_ptr->a_ref.x")
+        self.assertEqual(self.path(br_ar_y), "b_ref.a_ref.y")
+        bpr_deref_apr_deref = bpr_deref.GetChildMemberWithName("a_ptr_ref").Dereference()
+        bpr_deref_apr_deref2 = bpr_deref_apr_deref.Dereference()
+        self.assertEqual(self.path(bpr_deref_apr_deref), "b_ptr_ref->a_ptr_ref")
+        self.assertEqual(self.path(bpr_deref_apr_deref2), "*b_ptr_ref->a_ptr_ref")
+        bpr_deref_apr_deref_x = bpr_deref_apr_deref.GetChildMemberWithName("x")
+        bpr_deref_apr_deref2_x = bpr_deref_apr_deref2.GetChildMemberWithName("x")
+        self.assertEqual(self.path(bpr_deref_apr_deref_x), "b_ptr_ref->a_ptr_ref->x")
+        self.assertEqual(self.path(bpr_deref_apr_deref2_x), "(*b_ptr_ref->a_ptr_ref).x")
+
+        # Find "c" variables in frame
+        c = self.frame.FindVariable("c")
+        cp = self.frame.FindVariable("c_ptr")
+        cr = self.frame.FindVariable("c_ref")
+        cpr = self.frame.FindVariable("c_ptr_ref")
+        # Dereference pointers
+        cp_deref = cp.Dereference()
+        cpr_deref = cpr.Dereference()  # a pointer
+        cpr_deref2 = cpr_deref.Dereference()  # two Dereference() calls to get object
+        # Check expression paths
+        self.assertEqual(self.path(cp_deref), "*c_ptr")
+        self.assertEqual(self.path(cpr_deref), "c_ptr_ref")
+        self.assertEqual(self.path(cpr_deref2), "*c_ptr_ref")
+
+        # Access members on "c" variables and check expression paths
+        self.assertEqual(self.path(c.GetChildMemberWithName("x")), "c.x")
+        self.assertEqual(self.path(cp.GetChildMemberWithName("x")), "c_ptr->x")
+        self.assertEqual(self.path(cr.GetChildMemberWithName("x")), "c_ref.x")
+        self.assertEqual(self.path(cp_deref.GetChildMemberWithName("x")), "(*c_ptr).x")
+        self.assertEqual(self.path(cpr_deref.GetChildMemberWithName("x")), "c_ptr_ref->x")
+        self.assertEqual(self.path(cpr_deref2.GetChildMemberWithName("x")), "(*c_ptr_ref).x")
+        # TODO: Uncomment once accessing members on pointer references is supported.
+        # self.assertEqual(self.path(cpr.GetChildMemberWithName("x")), "c_ptr_ref->x")
 No newline at end of file

lldb/test/API/python_api/expression_path/main.cpp

+struct StructA {
+  int x;
+  int y;
+};
+
+struct StructB {
+  int x;
+  StructA &a_ref;
+  StructA *&a_ptr_ref;
+};
+
+struct StructC : public StructB {
+  int y;
+
+  StructC(int x, StructA &a_ref, StructA *&a_ref_ptr, int y)
+      : StructB{x, a_ref, a_ref_ptr}, y(y) {}
+};
+
+int main() {
+  StructA a{1, 2};
+  StructA *a_ptr = &a;
+
+  StructB b{3, a, a_ptr};
+  StructB *b_ptr = &b;
+  StructB &b_ref = b;
+  StructB *&b_ptr_ref = b_ptr;
+
+  StructC c(4, a, a_ptr, 5);
+  StructC *c_ptr = &c;
+  StructC &c_ref = c;
+  StructC *&c_ptr_ref = c_ptr;
+
+  return 0; // Set breakpoint here
+}
 No newline at end of file