From 8c7a1f87617067bc23c2e0733fe5e3202e1d6e81 Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Thu, 20 Oct 2022 15:20:50 -0700 Subject: [PATCH] Revert "[lldb] Fix member access in GetExpressionPath" This reverts commit 0205aa4a02570dfeda5807f66756ebdbb102744b because it breaks TestArray.py: a->c = I decided to revert instead of disable the test because it looks like a legitimate issue with the patch. --- lldb/source/Core/ValueObject.cpp | 141 ++++++++------------- .../Plugins/TypeSystem/Clang/TypeSystemClang.cpp | 2 - .../data-formatter-synth/TestDataFormatterSynth.py | 4 +- lldb/test/API/python_api/expression_path/Makefile | 3 - .../expression_path/TestExpressionPath.py | 119 ----------------- lldb/test/API/python_api/expression_path/main.cpp | 34 ----- 6 files changed, 58 insertions(+), 245 deletions(-) delete mode 100644 lldb/test/API/python_api/expression_path/Makefile delete mode 100644 lldb/test/API/python_api/expression_path/TestExpressionPath.py delete mode 100644 lldb/test/API/python_api/expression_path/main.cpp diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp index 226a2c3..19d86be 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 idxs, return root; } -lldb::ValueObjectSP -ValueObject::GetChildAtIndexPath(llvm::ArrayRef> idxs, - size_t *index_of_error) { +lldb::ValueObjectSP ValueObject::GetChildAtIndexPath( + llvm::ArrayRef> 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 read_string = - ReadPointedString(buffer_sp, error, 0, - (custom_format == eFormatVectorOfChar) || - (custom_format == eFormatCharArray)); + std::pair 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 3ebd42c..650f8a2 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 b2fb677..7a2ea73 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 @@ class SynthDataFormatterTestCase(TestBase): # 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 99998b2..0000000 --- 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 bc81065..0000000 --- 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 522022b..0000000 --- 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 -- 2.7.4