Author: Pavel Labath Date: 2024-01-24T07:12:52Z New Revision: 78b00c116be8b3b53ff13552e31eb305b11cb169
URL: https://github.com/llvm/llvm-project/commit/78b00c116be8b3b53ff13552e31eb305b11cb169 DIFF: https://github.com/llvm/llvm-project/commit/78b00c116be8b3b53ff13552e31eb305b11cb169.diff LOG: Revert "[lldb] Improve maintainability and readability for ValueObject methods (#75865)" This reverts commit d657519838e4b2310e13ec5ff52599e041860825 as it breaks two dozen tests. The breakages are related to variable path expression parsing and summary string parsing (possibly the same code). Added: Modified: lldb/source/Core/ValueObject.cpp Removed: ################################################################################ diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp index d58bf2ca763d9e2..9208047be36668d 100644 --- a/lldb/source/Core/ValueObject.cpp +++ b/lldb/source/Core/ValueObject.cpp @@ -1582,64 +1582,62 @@ bool ValueObject::IsUninitializedReference() { ValueObjectSP ValueObject::GetSyntheticArrayMember(size_t index, bool can_create) { - if (!IsPointerType() && !IsArrayType()) - return ValueObjectSP(); - - std::string index_str = llvm::formatv("[{0}]", index); - ConstString index_const_str(index_str); - // Check if we have already created a synthetic array member in this valid - // object. If we have we will re-use it. - if (auto existing_synthetic_child = GetSyntheticChild(index_const_str)) - return existing_synthetic_child; - - // We haven't made a synthetic array member for INDEX yet, so lets make - // one and cache it for any future reference. - ValueObject *synthetic_child = CreateChildAtIndex(0, true, index); - - if (!synthetic_child) - return ValueObjectSP(); - - // Cache the synthetic child's value because it's valid. - AddSyntheticChild(index_const_str, synthetic_child); - auto synthetic_child_sp = synthetic_child->GetSP(); - synthetic_child_sp->SetName(ConstString(index_str)); - synthetic_child_sp->m_flags.m_is_array_item_for_pointer = true; + ValueObjectSP synthetic_child_sp; + if (IsPointerType() || IsArrayType()) { + std::string index_str = llvm::formatv("[{0}]", index); + ConstString index_const_str(index_str); + // Check if we have already created a synthetic array member in this valid + // object. If we have we will re-use it. + synthetic_child_sp = GetSyntheticChild(index_const_str); + if (!synthetic_child_sp) { + ValueObject *synthetic_child; + // We haven't made a synthetic array member for INDEX yet, so lets make + // one and cache it for any future reference. + synthetic_child = CreateChildAtIndex(0, true, index); + + // Cache the value if we got one back... + if (synthetic_child) { + AddSyntheticChild(index_const_str, synthetic_child); + synthetic_child_sp = synthetic_child->GetSP(); + synthetic_child_sp->SetName(ConstString(index_str)); + synthetic_child_sp->m_flags.m_is_array_item_for_pointer = true; + } + } + } return synthetic_child_sp; } ValueObjectSP ValueObject::GetSyntheticBitFieldChild(uint32_t from, uint32_t to, bool can_create) { - if (!IsScalarType()) - return ValueObjectSP(); - - std::string index_str = llvm::formatv("[{0}-{1}]", from, to); - ConstString index_const_str(index_str); - - // Check if we have already created a synthetic array member in this valid - // object. If we have we will re-use it. - if (auto existing_synthetic_child = GetSyntheticChild(index_const_str)) - return existing_synthetic_child; - - uint32_t bit_field_size = to - from + 1; - uint32_t bit_field_offset = from; - if (GetDataExtractor().GetByteOrder() == eByteOrderBig) - bit_field_offset = - GetByteSize().value_or(0) * 8 - bit_field_size - bit_field_offset; - - // We haven't made a synthetic array member for INDEX yet, so lets make - // one and cache it for any future reference. - ValueObjectChild *synthetic_child = new ValueObjectChild( - *this, GetCompilerType(), index_const_str, GetByteSize().value_or(0), 0, - bit_field_size, bit_field_offset, false, false, eAddressTypeInvalid, 0); - - if (!synthetic_child) - return ValueObjectSP(); - - // Cache the synthetic child's value because it's valid. - AddSyntheticChild(index_const_str, synthetic_child); - auto synthetic_child_sp = synthetic_child->GetSP(); - synthetic_child_sp->SetName(ConstString(index_str)); - synthetic_child_sp->m_flags.m_is_bitfield_for_scalar = true; + ValueObjectSP synthetic_child_sp; + if (IsScalarType()) { + std::string index_str = llvm::formatv("[{0}-{1}]", from, to); + ConstString index_const_str(index_str); + // Check if we have already created a synthetic array member in this valid + // object. If we have we will re-use it. + synthetic_child_sp = GetSyntheticChild(index_const_str); + if (!synthetic_child_sp) { + uint32_t bit_field_size = to - from + 1; + uint32_t bit_field_offset = from; + if (GetDataExtractor().GetByteOrder() == eByteOrderBig) + bit_field_offset = + GetByteSize().value_or(0) * 8 - bit_field_size - bit_field_offset; + // We haven't made a synthetic array member for INDEX yet, so lets make + // one and cache it for any future reference. + ValueObjectChild *synthetic_child = new ValueObjectChild( + *this, GetCompilerType(), index_const_str, GetByteSize().value_or(0), + 0, bit_field_size, bit_field_offset, false, false, + eAddressTypeInvalid, 0); + + // Cache the value if we got one back... + if (synthetic_child) { + AddSyntheticChild(index_const_str, synthetic_child); + synthetic_child_sp = synthetic_child->GetSP(); + synthetic_child_sp->SetName(ConstString(index_str)); + synthetic_child_sp->m_flags.m_is_bitfield_for_scalar = true; + } + } + } return synthetic_child_sp; } @@ -1649,8 +1647,9 @@ ValueObjectSP ValueObject::GetSyntheticChildAtOffset( ValueObjectSP synthetic_child_sp; - if (name_const_str.IsEmpty()) + if (name_const_str.IsEmpty()) { name_const_str.SetString("@" + std::to_string(offset)); + } // Check if we have already created a synthetic array member in this valid // object. If we have we will re-use it. @@ -1660,13 +1659,13 @@ ValueObjectSP ValueObject::GetSyntheticChildAtOffset( return synthetic_child_sp; if (!can_create) - return ValueObjectSP(); + return {}; ExecutionContext exe_ctx(GetExecutionContextRef()); std::optional<uint64_t> size = type.GetByteSize(exe_ctx.GetBestExecutionContextScope()); if (!size) - return ValueObjectSP(); + return {}; ValueObjectChild *synthetic_child = new ValueObjectChild(*this, type, name_const_str, *size, offset, 0, 0, false, false, eAddressTypeInvalid, 0); @@ -1700,7 +1699,7 @@ ValueObjectSP ValueObject::GetSyntheticBase(uint32_t offset, return synthetic_child_sp; if (!can_create) - return ValueObjectSP(); + return {}; const bool is_base_class = true; @@ -1708,7 +1707,7 @@ ValueObjectSP ValueObject::GetSyntheticBase(uint32_t offset, std::optional<uint64_t> size = type.GetByteSize(exe_ctx.GetBestExecutionContextScope()); if (!size) - return ValueObjectSP(); + return {}; ValueObjectChild *synthetic_child = new ValueObjectChild(*this, type, name_const_str, *size, offset, 0, 0, is_base_class, false, eAddressTypeInvalid, 0); @@ -1737,30 +1736,30 @@ static const char *SkipLeadingExpressionPathSeparators(const char *expression) { ValueObjectSP ValueObject::GetSyntheticExpressionPathChild(const char *expression, bool can_create) { + ValueObjectSP synthetic_child_sp; ConstString name_const_string(expression); // Check if we have already created a synthetic array member in this valid // object. If we have we will re-use it. - if (auto existing_synthetic_child = GetSyntheticChild(name_const_string)) - return existing_synthetic_child; - - // We haven't made a synthetic array member for expression yet, so lets - // make one and cache it for any future reference. - auto path_options = GetValueForExpressionPathOptions(); - path_options.SetSyntheticChildrenTraversal( - GetValueForExpressionPathOptions::SyntheticChildrenTraversal::None); - auto synthetic_child = - GetValueForExpressionPath(expression, nullptr, nullptr, path_options); - - if (!synthetic_child) - return ValueObjectSP(); - - // Cache the synthetic child's value because it's valid. - // FIXME: this causes a "real" child to end up with its name changed to - // the contents of expression - AddSyntheticChild(name_const_string, synthetic_child.get()); - synthetic_child->SetName( - ConstString(SkipLeadingExpressionPathSeparators(expression))); - return synthetic_child; + synthetic_child_sp = GetSyntheticChild(name_const_string); + if (!synthetic_child_sp) { + // We haven't made a synthetic array member for expression yet, so lets + // make one and cache it for any future reference. + synthetic_child_sp = GetValueForExpressionPath( + expression, nullptr, nullptr, + GetValueForExpressionPathOptions().SetSyntheticChildrenTraversal( + GetValueForExpressionPathOptions::SyntheticChildrenTraversal:: + None)); + + // Cache the value if we got one back... + if (synthetic_child_sp.get()) { + // FIXME: this causes a "real" child to end up with its name changed to + // the contents of expression + AddSyntheticChild(name_const_string, synthetic_child_sp.get()); + synthetic_child_sp->SetName( + ConstString(SkipLeadingExpressionPathSeparators(expression))); + } + } + return synthetic_child_sp; } void ValueObject::CalculateSyntheticValue() { @@ -1957,55 +1956,66 @@ ValueObjectSP ValueObject::GetValueForExpressionPath( const GetValueForExpressionPathOptions &options, ExpressionPathAftermath *final_task_on_target) { - auto dummy_stop_reason = eExpressionPathScanEndReasonUnknown; - auto dummy_value_type = eExpressionPathEndResultTypeInvalid; - auto dummy_final_task = eExpressionPathAftermathNothing; - - auto proxy_stop_reason = reason_to_stop ? reason_to_stop : &dummy_stop_reason; - auto proxy_value_type = - final_value_type ? final_value_type : &dummy_value_type; - auto proxy_final_task = - final_task_on_target ? final_task_on_target : &dummy_final_task; - - auto ret_value = GetValueForExpressionPath_Impl(expression, proxy_stop_reason, - proxy_value_type, options, - proxy_final_task); - - // The caller knows nothing happened if `final_task_on_target` doesn't change. - if (!ret_value || (*proxy_value_type) != eExpressionPathEndResultTypePlain || - !final_task_on_target) - return ValueObjectSP(); - - ExpressionPathAftermath &final_task_on_target_ref = (*final_task_on_target); - ExpressionPathScanEndReason stop_reason_for_error; - Status error; - // The method can only dereference and take the address of plain objects. - switch (final_task_on_target_ref) { - case eExpressionPathAftermathNothing: - return ret_value; - - case eExpressionPathAftermathDereference: - ret_value = ret_value->Dereference(error); - stop_reason_for_error = eExpressionPathScanEndReasonDereferencingFailed; - break; - - case eExpressionPathAftermathTakeAddress: - ret_value = ret_value->AddressOf(error); - stop_reason_for_error = eExpressionPathScanEndReasonTakingAddressFailed; - break; - } - - if (ret_value && error.Success()) { - final_task_on_target_ref = eExpressionPathAftermathNothing; - return ret_value; + ExpressionPathScanEndReason dummy_reason_to_stop = + ValueObject::eExpressionPathScanEndReasonUnknown; + ExpressionPathEndResultType dummy_final_value_type = + ValueObject::eExpressionPathEndResultTypeInvalid; + ExpressionPathAftermath dummy_final_task_on_target = + ValueObject::eExpressionPathAftermathNothing; + + ValueObjectSP ret_val = GetValueForExpressionPath_Impl( + expression, reason_to_stop ? reason_to_stop : &dummy_reason_to_stop, + final_value_type ? final_value_type : &dummy_final_value_type, options, + final_task_on_target ? final_task_on_target + : &dummy_final_task_on_target); + + if (!final_task_on_target || + *final_task_on_target == ValueObject::eExpressionPathAftermathNothing) + return ret_val; + + if (ret_val.get() && + ((final_value_type ? *final_value_type : dummy_final_value_type) == + eExpressionPathEndResultTypePlain)) // I can only deref and takeaddress + // of plain objects + { + if ((final_task_on_target ? *final_task_on_target + : dummy_final_task_on_target) == + ValueObject::eExpressionPathAftermathDereference) { + Status error; + ValueObjectSP final_value = ret_val->Dereference(error); + if (error.Fail() || !final_value.get()) { + if (reason_to_stop) + *reason_to_stop = + ValueObject::eExpressionPathScanEndReasonDereferencingFailed; + if (final_value_type) + *final_value_type = ValueObject::eExpressionPathEndResultTypeInvalid; + return ValueObjectSP(); + } else { + if (final_task_on_target) + *final_task_on_target = ValueObject::eExpressionPathAftermathNothing; + return final_value; + } + } + if (*final_task_on_target == + ValueObject::eExpressionPathAftermathTakeAddress) { + Status error; + ValueObjectSP final_value = ret_val->AddressOf(error); + if (error.Fail() || !final_value.get()) { + if (reason_to_stop) + *reason_to_stop = + ValueObject::eExpressionPathScanEndReasonTakingAddressFailed; + if (final_value_type) + *final_value_type = ValueObject::eExpressionPathEndResultTypeInvalid; + return ValueObjectSP(); + } else { + if (final_task_on_target) + *final_task_on_target = ValueObject::eExpressionPathAftermathNothing; + return final_value; + } + } } - - if (reason_to_stop) - *reason_to_stop = stop_reason_for_error; - - if (final_value_type) - *final_value_type = eExpressionPathEndResultTypeInvalid; - return ValueObjectSP(); + return ret_val; // final_task_on_target will still have its original value, so + // you know I did not do it } ValueObjectSP ValueObject::GetValueForExpressionPath_Impl( @@ -2676,47 +2686,39 @@ ValueObjectSP ValueObject::AddressOf(Status &error) { const bool scalar_is_load_address = false; addr_t addr = GetAddressOf(scalar_is_load_address, &address_type); error.Clear(); + if (addr != LLDB_INVALID_ADDRESS && address_type != eAddressTypeHost) { + switch (address_type) { + case eAddressTypeInvalid: { + StreamString expr_path_strm; + GetExpressionPath(expr_path_strm); + error.SetErrorStringWithFormat("'%s' is not in memory", + expr_path_strm.GetData()); + } break; - StreamString expr_path_strm; - GetExpressionPath(expr_path_strm); - const char *expr_path_str = expr_path_strm.GetData(); - - ExecutionContext exe_ctx(GetExecutionContextRef()); - auto scope = exe_ctx.GetBestExecutionContextScope(); - - if (addr == LLDB_INVALID_ADDRESS) { + case eAddressTypeFile: + case eAddressTypeLoad: { + CompilerType compiler_type = GetCompilerType(); + if (compiler_type) { + std::string name(1, '&'); + name.append(m_name.AsCString("")); + ExecutionContext exe_ctx(GetExecutionContextRef()); + m_addr_of_valobj_sp = ValueObjectConstResult::Create( + exe_ctx.GetBestExecutionContextScope(), + compiler_type.GetPointerType(), ConstString(name.c_str()), addr, + eAddressTypeInvalid, m_data.GetAddressByteSize()); + } + } break; + default: + break; + } + } else { + StreamString expr_path_strm; + GetExpressionPath(expr_path_strm); error.SetErrorStringWithFormat("'%s' doesn't have a valid address", - expr_path_str); - return ValueObjectSP(); + expr_path_strm.GetData()); } - switch (address_type) { - case eAddressTypeInvalid: - error.SetErrorStringWithFormat("'%s' is not in memory", expr_path_str); - return ValueObjectSP(); - - case eAddressTypeHost: - error.SetErrorStringWithFormat("'%s' is in host process (LLDB) memory", - expr_path_str); - return ValueObjectSP(); - - case eAddressTypeFile: - case eAddressTypeLoad: { - CompilerType compiler_type = GetCompilerType(); - if (!compiler_type) { - error.SetErrorStringWithFormat("'%s' doesn't have a compiler type", - expr_path_str); - return ValueObjectSP(); - } - - std::string name(1, '&'); - name.append(m_name.AsCString("")); - m_addr_of_valobj_sp = ValueObjectConstResult::Create( - scope, compiler_type.GetPointerType(), ConstString(name.c_str()), addr, - eAddressTypeInvalid, m_data.GetAddressByteSize()); - return m_addr_of_valobj_sp; - } - } + return m_addr_of_valobj_sp; } ValueObjectSP ValueObject::DoCast(const CompilerType &compiler_type) { _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
