[Lldb-commits] [lldb] 8c7a1f8 - Revert "[lldb] Fix member access in GetExpressionPath"

Jonas Devlieghere via lldb-commits lldb-commits at lists.llvm.org
Thu Oct 20 15:22:05 PDT 2022


Author: Jonas Devlieghere
Date: 2022-10-20T15:21:59-07:00
New Revision: 8c7a1f87617067bc23c2e0733fe5e3202e1d6e81

URL: https://github.com/llvm/llvm-project/commit/8c7a1f87617067bc23c2e0733fe5e3202e1d6e81
DIFF: https://github.com/llvm/llvm-project/commit/8c7a1f87617067bc23c2e0733fe5e3202e1d6e81.diff

LOG: Revert "[lldb] Fix member access in GetExpressionPath"

This reverts commit 0205aa4a02570dfeda5807f66756ebdbb102744b because it
breaks TestArray.py:

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

I decided to revert instead of disable the test because it looks like a
legitimate issue with the patch.

Added: 
    

Modified: 
    lldb/source/Core/ValueObject.cpp
    lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
    lldb/test/API/functionalities/data-formatter/data-formatter-synth/TestDataFormatterSynth.py

Removed: 
    lldb/test/API/python_api/expression_path/Makefile
    lldb/test/API/python_api/expression_path/TestExpressionPath.py
    lldb/test/API/python_api/expression_path/main.cpp


################################################################################
diff  --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index 226a2c3f690f2..19d86bee40e1f 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -273,6 +273,8 @@ CompilerType ValueObject::MaybeCalculateCompleteType() {
   return compiler_type;
 }
 
+
+
 DataExtractor &ValueObject::GetDataExtractor() {
   UpdateValueIfNeeded(false);
   return m_data;
@@ -407,9 +409,8 @@ ValueObject::GetChildAtIndexPath(llvm::ArrayRef<size_t> idxs,
   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());
@@ -1184,10 +1185,9 @@ bool ValueObject::DumpPrintableRepresentation(
       {
         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) ||
+                                     (custom_format == eFormatCharArray));
         lldb_private::formatters::StringPrinter::
             ReadBufferAndDumpToStreamOptions options(*this);
         options.SetData(DataExtractor(
@@ -1552,7 +1552,8 @@ bool ValueObject::GetDeclaration(Declaration &decl) {
   return false;
 }
 
-void ValueObject::AddSyntheticChild(ConstString key, ValueObject *valobj) {
+void ValueObject::AddSyntheticChild(ConstString key,
+                                    ValueObject *valobj) {
   m_synthetic_children[key] = valobj;
 }
 
@@ -1923,96 +1924,64 @@ void ValueObject::GetExpressionPath(Stream &s,
     return;
   }
 
-  // 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();
-  };
-
-  ValueObject *parent = GetParent();
+  const bool is_deref_of_parent = IsDereferenceOfParent();
 
-  if (is_deref_of_non_reference(this) &&
+  if (is_deref_of_parent &&
       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.PutChar('*');
-    if (parent)
-      parent->GetExpressionPath(s, epformat);
-    return;
+    // a_ptr->memberName. the eHonorPointers mode is meant to produce strings
+    // in this latter format
+    s.PutCString("*(");
   }
 
-  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() && !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) {
-          print_ptr_access = true;
-        } else {
-          const uint32_t non_base_class_parent_type_info =
-              non_base_class_parent_compiler_type.GetTypeInfo();
-
-          if (non_base_class_parent_type_info & eTypeIsPointer) {
-            print_ptr_access = true;
-          } else if ((non_base_class_parent_type_info & eTypeHasChildren) &&
-                     !(non_base_class_parent_type_info & eTypeIsArray)) {
-            print_obj_access = true;
+  ValueObject *parent = GetParent();
+
+  if (parent)
+    parent->GetExpressionPath(s, epformat);
+
+  // 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());
+
+  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 (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('.');
+            }
           }
         }
       }
-      is_parent_deref_of_non_reference =
-          is_deref_of_non_reference(non_base_class_parent) &&
-          epformat == eGetExpressionPathFormatDereferencePointers;
-    }
-  }
 
-  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(')');
+      const char *name = GetName().GetCString();
+      if (name)
+        s.PutCString(name);
+    }
   }
 
-  if (print_obj_access)
-    s.PutChar('.');
-  if (print_ptr_access)
-    s.PutCString("->");
-
-  if (!IsBaseClass() && !is_deref_of_parent) {
-    const char *name = GetName().GetCString();
-    if (name)
-      s.PutCString(name);
+  if (is_deref_of_parent &&
+      epformat == eGetExpressionPathFormatDereferencePointers) {
+    s.PutChar(')');
   }
 }
 
@@ -3139,6 +3108,8 @@ bool ValueObject::CanProvideValue() {
   return (!type.IsValid()) || (0 != (type.GetTypeInfo() & eTypeHasValue));
 }
 
+
+
 ValueObjectSP ValueObject::Persist() {
   if (!UpdateValueIfNeeded())
     return nullptr;

diff  --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 3ebd42c996bdc..650f8a29d4063 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -6578,8 +6578,6 @@ CompilerType TypeSystemClang::GetChildCompilerTypeAtIndex(
             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) {

diff  --git a/lldb/test/API/functionalities/data-formatter/data-formatter-synth/TestDataFormatterSynth.py b/lldb/test/API/functionalities/data-formatter/data-formatter-synth/TestDataFormatterSynth.py
index b2fb677377426..7a2ea73f364e7 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-synth/TestDataFormatterSynth.py
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-synth/TestDataFormatterSynth.py
@@ -172,8 +172,8 @@ def cleanup():
         # 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',

diff  --git a/lldb/test/API/python_api/expression_path/Makefile b/lldb/test/API/python_api/expression_path/Makefile
deleted file mode 100644
index 99998b20bcb05..0000000000000
--- a/lldb/test/API/python_api/expression_path/Makefile
+++ /dev/null
@@ -1,3 +0,0 @@
-CXX_SOURCES := main.cpp
-
-include Makefile.rules

diff  --git a/lldb/test/API/python_api/expression_path/TestExpressionPath.py b/lldb/test/API/python_api/expression_path/TestExpressionPath.py
deleted file mode 100644
index bc81065f7bb1b..0000000000000
--- a/lldb/test/API/python_api/expression_path/TestExpressionPath.py
+++ /dev/null
@@ -1,119 +0,0 @@
-"""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

diff  --git a/lldb/test/API/python_api/expression_path/main.cpp b/lldb/test/API/python_api/expression_path/main.cpp
deleted file mode 100644
index 522022bd8125b..0000000000000
--- a/lldb/test/API/python_api/expression_path/main.cpp
+++ /dev/null
@@ -1,34 +0,0 @@
-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


        


More information about the lldb-commits mailing list