shafik added inline comments.
================ Comment at: lldb/source/Expression/DWARFExpression.cpp:948 +llvm::Optional<lldb::addr_t> +ResolveAndLoadFileAddress(ExecutionContext *exe_ctx, lldb::ModuleSP module_sp, + Status *error_ptr, const char *dw_op_type, ---------------- aprantl wrote: > Should this also be in the anonymous namespace? Good catch although `static` is probably more appropriate: https://llvm.org/docs/CodingStandards.html#anonymous-namespaces ================ Comment at: lldb/source/Expression/DWARFExpression.cpp:1141 Address so_addr; - if (!module_sp->ResolveFileAddress(file_addr, so_addr)) { - if (error_ptr) - error_ptr->SetErrorString( - "failed to resolve file address in module"); + auto load_Addr_optional = ResolveAndLoadFileAddress( + exe_ctx, module_sp, error_ptr, "DW_OP_deref", file_addr, so_addr); ---------------- aprantl wrote: > aprantl wrote: > > why is `Addr` captialized? > we sometimes call these `maybe_load_addr` or `load_addr_or_err` not sure if > that's better :-) `load_Addr` was what it was before, I didn't even notice it. ================ Comment at: lldb/source/Expression/DWARFExpression.cpp:1282 + + const bool force_live_memory = true; + if (exe_ctx->GetTargetRef().ReadMemory(so_addr, &addr_bytes, size, ---------------- aprantl wrote: > Why do we need to force live memory? Good catch, we don't. ================ Comment at: lldb/source/Expression/DWARFExpression.cpp:1299 + } + } else if (load_Addr == LLDB_INVALID_ADDRESS) { + if (error_ptr) ---------------- aprantl wrote: > same here, but maybe we can just sink this into the helper. Makes sense, it was a little tricky but I think with these changes it is neater. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121408/new/ https://reviews.llvm.org/D121408 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
