llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) <details> <summary>Changes</summary> Change the interface of DWARFExpression::Evaluate and DWARFExpressionList::Evaluate to return an llvm::Expected instead of a boolean. This eliminates the Status output parameter and improves error handling. --- Patch is 78.24 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/94420.diff 12 Files Affected: - (modified) lldb/include/lldb/Expression/DWARFExpression.h (+6-7) - (modified) lldb/include/lldb/Expression/DWARFExpressionList.h (+6-4) - (modified) lldb/source/Core/ValueObjectVariable.cpp (+6-2) - (modified) lldb/source/Expression/DWARFExpression.cpp (+286-461) - (modified) lldb/source/Expression/DWARFExpressionList.cpp (+10-18) - (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+12-8) - (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+9-7) - (modified) lldb/source/Symbol/Function.cpp (+9-8) - (modified) lldb/source/Target/RegisterContextUnwind.cpp (+12-11) - (modified) lldb/source/Target/StackFrame.cpp (+7-12) - (modified) lldb/unittests/Expression/DWARFExpressionTest.cpp (+38-40) - (modified) llvm/include/llvm/Support/Error.h (+5) ``````````diff diff --git a/lldb/include/lldb/Expression/DWARFExpression.h b/lldb/include/lldb/Expression/DWARFExpression.h index 1d85308d1caa7..e85ba464dea6b 100644 --- a/lldb/include/lldb/Expression/DWARFExpression.h +++ b/lldb/include/lldb/Expression/DWARFExpression.h @@ -132,13 +132,12 @@ class DWARFExpression { /// \return /// True on success; false otherwise. If error_ptr is non-NULL, /// details of the failure are provided through it. - static bool Evaluate(ExecutionContext *exe_ctx, RegisterContext *reg_ctx, - lldb::ModuleSP module_sp, const DataExtractor &opcodes, - const plugin::dwarf::DWARFUnit *dwarf_cu, - const lldb::RegisterKind reg_set, - const Value *initial_value_ptr, - const Value *object_address_ptr, Value &result, - Status *error_ptr); + static llvm::Expected<Value> + Evaluate(ExecutionContext *exe_ctx, RegisterContext *reg_ctx, + lldb::ModuleSP module_sp, const DataExtractor &opcodes, + const plugin::dwarf::DWARFUnit *dwarf_cu, + const lldb::RegisterKind reg_set, const Value *initial_value_ptr, + const Value *object_address_ptr); static bool ParseDWARFLocationList(const plugin::dwarf::DWARFUnit *dwarf_cu, const DataExtractor &data, diff --git a/lldb/include/lldb/Expression/DWARFExpressionList.h b/lldb/include/lldb/Expression/DWARFExpressionList.h index c2218ad4af0a7..f711a1cbe9bbd 100644 --- a/lldb/include/lldb/Expression/DWARFExpressionList.h +++ b/lldb/include/lldb/Expression/DWARFExpressionList.h @@ -9,6 +9,7 @@ #ifndef LLDB_EXPRESSION_DWARFEXPRESSIONLIST_H #define LLDB_EXPRESSION_DWARFEXPRESSIONLIST_H +#include "lldb/Core/Value.h" #include "lldb/Expression/DWARFExpression.h" #include "lldb/Utility/RangeMap.h" #include "lldb/lldb-private.h" @@ -113,10 +114,11 @@ class DWARFExpressionList { void SetModule(const lldb::ModuleSP &module) { m_module_wp = module; } - bool Evaluate(ExecutionContext *exe_ctx, RegisterContext *reg_ctx, - lldb::addr_t func_load_addr, const Value *initial_value_ptr, - const Value *object_address_ptr, Value &result, - Status *error_ptr) const; + llvm::Expected<Value> Evaluate(ExecutionContext *exe_ctx, + RegisterContext *reg_ctx, + lldb::addr_t func_load_addr, + const Value *initial_value_ptr, + const Value *object_address_ptr) const; private: // RangeDataVector requires a comparator for DWARFExpression, but it doesn't diff --git a/lldb/source/Core/ValueObjectVariable.cpp b/lldb/source/Core/ValueObjectVariable.cpp index 67d71c90a959d..51eb11d3a189e 100644 --- a/lldb/source/Core/ValueObjectVariable.cpp +++ b/lldb/source/Core/ValueObjectVariable.cpp @@ -164,8 +164,11 @@ bool ValueObjectVariable::UpdateValue() { target); } Value old_value(m_value); - if (expr_list.Evaluate(&exe_ctx, nullptr, loclist_base_load_addr, nullptr, - nullptr, m_value, &m_error)) { + llvm::Expected<Value> maybe_value = expr_list.Evaluate( + &exe_ctx, nullptr, loclist_base_load_addr, nullptr, nullptr); + + if (maybe_value) { + m_value = *maybe_value; m_resolved_value = m_value; m_value.SetContext(Value::ContextType::Variable, variable); @@ -246,6 +249,7 @@ bool ValueObjectVariable::UpdateValue() { SetValueIsValid(m_error.Success()); } else { + m_error = maybe_value.takeError(); // could not find location, won't allow editing m_resolved_value.SetContext(Value::ContextType::Invalid, nullptr); } diff --git a/lldb/source/Expression/DWARFExpression.cpp b/lldb/source/Expression/DWARFExpression.cpp index 7473bb8255d0a..5c7ab49410efe 100644 --- a/lldb/source/Expression/DWARFExpression.cpp +++ b/lldb/source/Expression/DWARFExpression.cpp @@ -541,12 +541,12 @@ bool DWARFExpression::LinkThreadLocalStorage( return true; } -static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack, - ExecutionContext *exe_ctx, - RegisterContext *reg_ctx, - const DataExtractor &opcodes, - lldb::offset_t &opcode_offset, - Status *error_ptr, Log *log) { +static llvm::Error Evaluate_DW_OP_entry_value(std::vector<Value> &stack, + ExecutionContext *exe_ctx, + RegisterContext *reg_ctx, + const DataExtractor &opcodes, + lldb::offset_t &opcode_offset, + Log *log) { // DW_OP_entry_value(sub-expr) describes the location a variable had upon // function entry: this variable location is presumed to be optimized out at // the current PC value. The caller of the function may have call site @@ -593,16 +593,15 @@ static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack, // 1. Find the function which pushed the current frame onto the stack. if ((!exe_ctx || !exe_ctx->HasTargetScope()) || !reg_ctx) { - LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no exe/reg context"); - return false; + return llvm::createStringError( + "Evaluate_DW_OP_entry_value: no exe/reg context"); } StackFrame *current_frame = exe_ctx->GetFramePtr(); Thread *thread = exe_ctx->GetThreadPtr(); - if (!current_frame || !thread) { - LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no current frame/thread"); - return false; - } + if (!current_frame || !thread) + return llvm::createStringError( + "Evaluate_DW_OP_entry_value: no current frame/thread"); Target &target = exe_ctx->GetTargetRef(); StackFrameSP parent_frame = nullptr; @@ -634,25 +633,23 @@ static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack, break; } if (!parent_frame || !parent_frame->GetRegisterContext()) { - LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no parent frame with reg ctx"); - return false; + return llvm::createStringError( + "Evaluate_DW_OP_entry_value: no parent frame with reg ctx"); } Function *parent_func = parent_frame->GetSymbolContext(eSymbolContextFunction).function; - if (!parent_func) { - LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no parent function"); - return false; - } + if (!parent_func) + return llvm::createStringError( + "Evaluate_DW_OP_entry_value: no parent function"); // 2. Find the call edge in the parent function responsible for creating the // current activation. Function *current_func = current_frame->GetSymbolContext(eSymbolContextFunction).function; - if (!current_func) { - LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no current function"); - return false; - } + if (!current_func) + return llvm::createStringError( + "Evaluate_DW_OP_entry_value: no current function"); CallEdge *call_edge = nullptr; ModuleList &modlist = target.GetImages(); @@ -663,17 +660,16 @@ static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack, // produced by an ambiguous tail call. In this case, refuse to proceed. call_edge = parent_func->GetCallEdgeForReturnAddress(return_pc, target); if (!call_edge) { - LLDB_LOG(log, - "Evaluate_DW_OP_entry_value: no call edge for retn-pc = {0:x} " - "in parent frame {1}", - return_pc, parent_func->GetName()); - return false; + return llvm::createStringError(llvm::formatv( + "Evaluate_DW_OP_entry_value: no call edge for retn-pc = {0:x} " + "in parent frame {1}", + return_pc, parent_func->GetName())); } Function *callee_func = call_edge->GetCallee(modlist, parent_exe_ctx); if (callee_func != current_func) { - LLDB_LOG(log, "Evaluate_DW_OP_entry_value: ambiguous call sequence, " - "can't find real parent frame"); - return false; + return llvm::createStringError( + "Evaluate_DW_OP_entry_value: ambiguous call sequence, " + "can't find real parent frame"); } } else { // The StackFrameList solver machinery has deduced that an unambiguous tail @@ -686,21 +682,19 @@ static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack, } } } - if (!call_edge) { - LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no unambiguous edge from parent " - "to current function"); - return false; - } + if (!call_edge) + return llvm::createStringError( + "Evaluate_DW_OP_entry_value: no unambiguous edge from parent " + "to current function"); // 3. Attempt to locate the DW_OP_entry_value expression in the set of // available call site parameters. If found, evaluate the corresponding // parameter in the context of the parent frame. const uint32_t subexpr_len = opcodes.GetULEB128(&opcode_offset); const void *subexpr_data = opcodes.GetData(&opcode_offset, subexpr_len); - if (!subexpr_data) { - LLDB_LOG(log, "Evaluate_DW_OP_entry_value: subexpr could not be read"); - return false; - } + if (!subexpr_data) + return llvm::createStringError( + "Evaluate_DW_OP_entry_value: subexpr could not be read"); const CallSiteParameter *matched_param = nullptr; for (const CallSiteParameter ¶m : call_edge->GetCallSiteParameters()) { @@ -726,28 +720,27 @@ static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack, break; } } - if (!matched_param) { - LLDB_LOG(log, - "Evaluate_DW_OP_entry_value: no matching call site param found"); - return false; - } + if (!matched_param) + return llvm::createStringError( + "Evaluate_DW_OP_entry_value: no matching call site param found"); // TODO: Add support for DW_OP_push_object_address within a DW_OP_entry_value // subexpresion whenever llvm does. - Value result; const DWARFExpressionList ¶m_expr = matched_param->LocationInCaller; - if (!param_expr.Evaluate(&parent_exe_ctx, - parent_frame->GetRegisterContext().get(), - LLDB_INVALID_ADDRESS, - /*initial_value_ptr=*/nullptr, - /*object_address_ptr=*/nullptr, result, error_ptr)) { + + llvm::Expected<Value> maybe_result = param_expr.Evaluate( + &parent_exe_ctx, parent_frame->GetRegisterContext().get(), + LLDB_INVALID_ADDRESS, + /*initial_value_ptr=*/nullptr, + /*object_address_ptr=*/nullptr); + if (!maybe_result) { LLDB_LOG(log, "Evaluate_DW_OP_entry_value: call site param evaluation failed"); - return false; + return maybe_result.takeError(); } - stack.push_back(result); - return true; + stack.push_back(*maybe_result); + return llvm::Error::success(); } namespace { @@ -862,19 +855,15 @@ static Scalar DerefSizeExtractDataHelper(uint8_t *addr_bytes, return addr_data.GetAddress(&addr_data_offset); } -bool DWARFExpression::Evaluate( +llvm::Expected<Value> DWARFExpression::Evaluate( ExecutionContext *exe_ctx, RegisterContext *reg_ctx, lldb::ModuleSP module_sp, const DataExtractor &opcodes, const DWARFUnit *dwarf_cu, const lldb::RegisterKind reg_kind, - const Value *initial_value_ptr, const Value *object_address_ptr, - Value &result, Status *error_ptr) { + const Value *initial_value_ptr, const Value *object_address_ptr) { - if (opcodes.GetByteSize() == 0) { - if (error_ptr) - error_ptr->SetErrorString( - "no location, value may have been optimized out"); - return false; - } + if (opcodes.GetByteSize() == 0) + return llvm::createStringError( + "no location, value may have been optimized out"); std::vector<Value> stack; Process *process = nullptr; @@ -994,11 +983,9 @@ bool DWARFExpression::Evaluate( // retrieved from the dereferenced address is the size of an address on the // target machine. case DW_OP_deref: { - if (stack.empty()) { - if (error_ptr) - error_ptr->SetErrorString("Expression stack empty for DW_OP_deref."); - return false; - } + if (stack.empty()) + return llvm::createStringError( + "Expression stack empty for DW_OP_deref."); Value::ValueType value_type = stack.back().GetValueType(); switch (value_type) { case Value::ValueType::HostAddress: { @@ -1013,11 +1000,12 @@ bool DWARFExpression::Evaluate( LLDB_INVALID_ADDRESS); Address so_addr; + Status load_err; auto maybe_load_addr = ResolveLoadAddress( - exe_ctx, module_sp, error_ptr, "DW_OP_deref", file_addr, so_addr); + exe_ctx, module_sp, &load_err, "DW_OP_deref", file_addr, so_addr); if (!maybe_load_addr) - return false; + return load_err.ToError(); stack.back().GetScalar() = *maybe_load_addr; // Fall through to load address promotion code below. @@ -1041,30 +1029,22 @@ bool DWARFExpression::Evaluate( stack.back().GetScalar() = pointer_value; stack.back().ClearContext(); } else { - if (error_ptr) - error_ptr->SetErrorStringWithFormat( - "Failed to dereference pointer from 0x%" PRIx64 - " for DW_OP_deref: %s\n", - pointer_addr, error.AsCString()); - return false; + return llvm::createStringError( + "Failed to dereference pointer from 0x%" PRIx64 + " for DW_OP_deref: %s\n", + pointer_addr, error.AsCString()); } } else { - if (error_ptr) - error_ptr->SetErrorString("NULL process for DW_OP_deref.\n"); - return false; + return llvm::createStringError("NULL process for DW_OP_deref.\n"); } } else { - if (error_ptr) - error_ptr->SetErrorString( - "NULL execution context for DW_OP_deref.\n"); - return false; + return llvm::createStringError( + "NULL execution context for DW_OP_deref.\n"); } break; case Value::ValueType::Invalid: - if (error_ptr) - error_ptr->SetErrorString("Invalid value type for DW_OP_deref.\n"); - return false; + return llvm::createStringError("Invalid value type for DW_OP_deref.\n"); } } break; @@ -1083,18 +1063,13 @@ bool DWARFExpression::Evaluate( // expression stack. case DW_OP_deref_size: { if (stack.empty()) { - if (error_ptr) - error_ptr->SetErrorString( - "Expression stack empty for DW_OP_deref_size."); - return false; + return llvm::createStringError( + "Expression stack empty for DW_OP_deref_size."); } uint8_t size = opcodes.GetU8(&offset); if (size > 8) { - if (error_ptr) - error_ptr->SetErrorStringWithFormat( - "Invalid address size for DW_OP_deref_size: %d\n", - size); - return false; + return llvm::createStringError( + "Invalid address size for DW_OP_deref_size: %d\n", size); } Value::ValueType value_type = stack.back().GetValueType(); switch (value_type) { @@ -1142,13 +1117,14 @@ bool DWARFExpression::Evaluate( auto file_addr = stack.back().GetScalar().ULongLong(LLDB_INVALID_ADDRESS); Address so_addr; + Status resolve_err; auto maybe_load_addr = - ResolveLoadAddress(exe_ctx, module_sp, error_ptr, - "DW_OP_deref_size", file_addr, so_addr, - /*check_sectionoffset=*/true); + ResolveLoadAddress(exe_ctx, module_sp, &resolve_err, + "DW_OP_deref_size", file_addr, so_addr, + /*check_sectionoffset=*/true); if (!maybe_load_addr) - return false; + return resolve_err.ToError(); addr_t load_addr = *maybe_load_addr; @@ -1166,12 +1142,10 @@ bool DWARFExpression::Evaluate( stack.back().ClearContext(); break; } else { - if (error_ptr) - error_ptr->SetErrorStringWithFormat( - "Failed to dereference pointer for DW_OP_deref_size: " - "%s\n", - error.AsCString()); - return false; + return llvm::createStringError( + "Failed to dereference pointer for DW_OP_deref_size: " + "%s\n", + error.AsCString()); } } stack.back().GetScalar() = load_addr; @@ -1195,30 +1169,25 @@ bool DWARFExpression::Evaluate( process->GetByteOrder(), size); stack.back().ClearContext(); } else { - if (error_ptr) - error_ptr->SetErrorStringWithFormat( - "Failed to dereference pointer from 0x%" PRIx64 - " for DW_OP_deref: %s\n", - pointer_addr, error.AsCString()); - return false; + return llvm::createStringError( + "Failed to dereference pointer from 0x%" PRIx64 + " for DW_OP_deref: %s\n", + pointer_addr, error.AsCString()); } } else { - if (error_ptr) - error_ptr->SetErrorString("NULL process for DW_OP_deref_size.\n"); - return false; + + return llvm::createStringError( + "NULL process for DW_OP_deref_size.\n"); } } else { - if (error_ptr) - error_ptr->SetErrorString( - "NULL execution context for DW_OP_deref_size.\n"); - return false; + return llvm::createStringError( + "NULL execution context for DW_OP_deref_size.\n"); } break; case Value::ValueType::Invalid: - if (error_ptr) - error_ptr->SetErrorString("Invalid value for DW_OP_deref_size.\n"); - return false; + + return llvm::createStringError("Invalid value for DW_OP_deref_size.\n"); } } break; @@ -1239,9 +1208,9 @@ bool DWARFExpression::Evaluate( // extended to the size of an address on the target machine before being // pushed on the expression stack. case DW_OP_xderef_size: - if (error_ptr) - error_ptr->SetErrorString("Unimplemented opcode: DW_OP_xderef_size."); - return false; + + return llvm::createStringError( + "Unimplemented opcode: DW_OP_xderef_size."); // OPCODE: DW_OP_xderef // OPERANDS: none // DESCRIPTION: Provides an extended dereference mechanism. The entry at @@ -1253,9 +1222,7 @@ bool DWARFExpression::Evaluate( // retrieved from the dereferenced address is the size of an address on the // target machine. case DW_OP_xderef: - if (error_ptr) - error_ptr->SetErrorString("Unimplemented opcode: DW_OP_xderef."); - return false; + return llvm::createStringError("Unimplemented opcode: DW_OP_xderef."); // All DW_OP_constXXX opcodes have a single operand as noted below: // @@ -1308,9 +1275,7 @@ bool DWARFExpression::Evaluate( // DESCRIPTION: duplicates the value at the top of the stack case DW_OP_dup: if (stack.empty()) { - if (error_ptr) - error_ptr->SetErrorString("Expression stack empty for DW_OP_dup."); - return false; + return llvm::createStringError("Expression stack empty for DW_OP_dup."); } else stack.push_back(stack.back()); break; @@ -1320,9 +1285,8 @@ bool DWARFExpression::Evaluate( // DESCRIPTION: pops the value at the top of the stack case DW_OP_drop: if (stack.empty()) { - if (error_ptr) - error_ptr->SetErrorString("Expression stack empty for DW... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/94420 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
