From d657519838e4b2310e13ec5ff52599e041860825 Mon Sep 17 00:00:00 2001 From: Pete Lawrence Date: Tue, 23 Jan 2024 14:07:52 -1000 Subject: [lldb] Improve maintainability and readability for ValueObject methods (#75865) As I worked through changes to another PR (https://github.com/llvm/llvm-project/pull/74912), I couldn't help but rewrite a few methods for readability, maintainability, and possibly some behavior correctness too. 1. Exiting early instead of nested `if`-statements, which: - Reduces indentation levels for all subsequent lines - Treats missing pre-conditions similar to an error - Clearly indicates that the full length of the method is the "happy path". 2. Explicitly return empty Value Object shared pointers for those error (like) situations, which - Reduces the time it takes a maintainer to figure out what the method actually returns based on those conditions. 3. Converting a mix of `if` and `if`-`else`-statements around an enum into one `switch` statement, which: - Consolidates the former branching logic - Lets the compiler warn you of a (future) missing enum case - This one may actually change behavior slightly, because what was an early test for one enum case, now happens later on in the `switch`. 4. Consolidating near-identical, "copy-pasta" logic into one place, which: - Separates the common code to the diverging paths. - Highlights the differences between the code paths. rdar://119833526 --- lldb/source/Core/ValueObject.cpp | 330 +++++++++++++++++++-------------------- 1 file changed, 164 insertions(+), 166 deletions(-) diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp index 9208047..d58bf2c 100644 --- a/lldb/source/Core/ValueObject.cpp +++ b/lldb/source/Core/ValueObject.cpp @@ -1582,62 +1582,64 @@ bool ValueObject::IsUninitializedReference() { ValueObjectSP ValueObject::GetSyntheticArrayMember(size_t index, bool can_create) { - 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; - } - } - } + 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; return synthetic_child_sp; } ValueObjectSP ValueObject::GetSyntheticBitFieldChild(uint32_t from, uint32_t to, bool can_create) { - 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; - } - } - } + 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; return synthetic_child_sp; } @@ -1647,9 +1649,8 @@ 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. @@ -1659,13 +1660,13 @@ ValueObjectSP ValueObject::GetSyntheticChildAtOffset( return synthetic_child_sp; if (!can_create) - return {}; + return ValueObjectSP(); ExecutionContext exe_ctx(GetExecutionContextRef()); std::optional size = type.GetByteSize(exe_ctx.GetBestExecutionContextScope()); if (!size) - return {}; + return ValueObjectSP(); ValueObjectChild *synthetic_child = new ValueObjectChild(*this, type, name_const_str, *size, offset, 0, 0, false, false, eAddressTypeInvalid, 0); @@ -1699,7 +1700,7 @@ ValueObjectSP ValueObject::GetSyntheticBase(uint32_t offset, return synthetic_child_sp; if (!can_create) - return {}; + return ValueObjectSP(); const bool is_base_class = true; @@ -1707,7 +1708,7 @@ ValueObjectSP ValueObject::GetSyntheticBase(uint32_t offset, std::optional size = type.GetByteSize(exe_ctx.GetBestExecutionContextScope()); if (!size) - return {}; + return ValueObjectSP(); ValueObjectChild *synthetic_child = new ValueObjectChild(*this, type, name_const_str, *size, offset, 0, 0, is_base_class, false, eAddressTypeInvalid, 0); @@ -1736,30 +1737,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. - 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; + 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; } void ValueObject::CalculateSyntheticValue() { @@ -1956,66 +1957,55 @@ ValueObjectSP ValueObject::GetValueForExpressionPath( const GetValueForExpressionPathOptions &options, ExpressionPathAftermath *final_task_on_target) { - 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; - } - } + 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; } - return ret_val; // final_task_on_target will still have its original value, so - // you know I did not do it + + if (reason_to_stop) + *reason_to_stop = stop_reason_for_error; + + if (final_value_type) + *final_value_type = eExpressionPathEndResultTypeInvalid; + return ValueObjectSP(); } ValueObjectSP ValueObject::GetValueForExpressionPath_Impl( @@ -2686,39 +2676,47 @@ 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; - 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); + 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) { error.SetErrorStringWithFormat("'%s' doesn't have a valid address", - expr_path_strm.GetData()); + expr_path_str); + return ValueObjectSP(); } - return m_addr_of_valobj_sp; + 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; + } + } } ValueObjectSP ValueObject::DoCast(const CompilerType &compiler_type) { -- cgit v1.1