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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

This is nice, I have a few mostly stylistic comments inline.



================
Comment at: lldb/source/Expression/DWARFExpression.cpp:947
 
+llvm::Optional<lldb::addr_t>
+ResolveAndLoadFileAddress(ExecutionContext *exe_ctx, lldb::ModuleSP module_sp,
----------------
Maybe add a Doxygen comment explaining what this function does?


================
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,
----------------
Should this also be in the anonymous namespace?


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:964
+
+  addr_t load_Addr = so_addr.GetLoadAddress(exe_ctx->GetTargetPtr());
+
----------------
`return so_addr.GetLoadAddress(exe_ctx->GetTargetPtr();`
or `load_addr`


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:977
+    return addr_data.GetU8(&addr_data_offset);
+    break;
+  case 2:
----------------
the `break` is redundant after the `return`.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:990
+  }
+  llvm_unreachable("Fell out of a switch with a default, shouldn't happen!");
+}
----------------
This shouldn't be necessary, or does clang warn about the function not having a return on this otherwise?


================
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);
----------------
why is `Addr` captialized?


================
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:
> why is `Addr` captialized?
we sometimes call these `maybe_load_addr` or `load_addr_or_err` not sure if that's better :-)


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1149
+
         if (load_Addr == LLDB_INVALID_ADDRESS) {
           if (error_ptr)
----------------
should we sink this check into `ResolveAndLoadFileAddress`, too?


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1278
+
+        if (load_Addr == LLDB_INVALID_ADDRESS && so_addr.IsSectionOffset()) {
+          uint8_t addr_bytes[size];
----------------
Comment what this case handles? It's unintuitive that we go into a case where `load_Addr == LLDB_INVALID_ADDRESS`


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1282
+
+          const bool force_live_memory = true;
+          if (exe_ctx->GetTargetRef().ReadMemory(so_addr, &addr_bytes, size,
----------------
Why do we need to force live memory?


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1293
+          } else {
+            if (error_ptr)
+              error_ptr->SetErrorStringWithFormat(
----------------
It would be nice to invert the if and have an early exit instead.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1299
+          }
+        } else if (load_Addr == LLDB_INVALID_ADDRESS) {
+          if (error_ptr)
----------------
same here, but maybe we can just sink this into the helper.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121408/new/

https://reviews.llvm.org/D121408