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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Sorry for the late comments. Looks like it got checked in already, but will submit inline comments anyway.



================
Comment at: lldb/source/Expression/DWARFExpression.cpp:963
+static llvm::Optional<lldb::addr_t>
+ResolveAndLoadFileAddress(ExecutionContext *exe_ctx, lldb::ModuleSP module_sp,
+                          Status *error_ptr, const char *dw_op_type,
----------------
Pass the module_sp in as a reference to avoid the increment/decrement on the reference count?


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:963
+static llvm::Optional<lldb::addr_t>
+ResolveAndLoadFileAddress(ExecutionContext *exe_ctx, lldb::ModuleSP module_sp,
+                          Status *error_ptr, const char *dw_op_type,
----------------
clayborg wrote:
> Pass the module_sp in as a reference to avoid the increment/decrement on the reference count?
Maybe "ResolveLoadAddress" would be a better name?


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:971
+          "need module to resolve file address for %s", dw_op_type);
+    return {};
+  }
----------------
Do we prefer "return {}" over "return llvm::None"?


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:983
+  if (load_addr == LLDB_INVALID_ADDRESS &&
+      (check_sectionoffset && !so_addr.IsSectionOffset())) {
+    if (error_ptr)
----------------
Not sure if you need to do this as the call to "module_sp->ResolveFileAddress(...) will return false if the address wasn't resolved to a address that is section offset.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1006-1017
+  switch (size) {
+  case 1:
+    return addr_data.GetU8(&addr_data_offset);
+  case 2:
+    return addr_data.GetU16(&addr_data_offset);
+  case 4:
+    return addr_data.GetU32(&addr_data_offset);
----------------
DataExtractor already has a GetMaxU64() call you can use. It handles anything <= 8 bytes in size. Do we really want to handle the "default:" case above or the "else" clause in my suggested fix though?


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1102
                                           frame->CalculateTarget().get());
+
       break;
----------------
revert


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121408