[Lldb-commits] [PATCH] D121408: Fixing DWARFExpression handling of ValueType::FileAddress case for DW_OP_deref_size

[Shafik Yaghmour via Phabricator via lldb-commits]

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