[Lldb-commits] [PATCH] D71003: [lldb/DWARF] Switch to llvm location list parser

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/Expression/DWARFExpression.h:260
 
-  bool GetLocation(lldb::addr_t func_load_addr, lldb::addr_t pc,
-                   lldb::offset_t &offset, lldb::offset_t &len);
+  void RelocateLowHighPC(lldb::addr_t load_function_start, lldb::addr_t &low_pc,
+                         lldb::addr_t &high_pc) const;
----------------
Should this be part of the DWARFExpression API? It feels like a static function might be sufficient. Maybe it could take a range directly?


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:53
+  if (data.ValidOffsetForDataOfSize(offset, index_size))
+    return data.GetMaxU64_unchecked(&offset, index_size);
+  return LLDB_INVALID_ADDRESS;
----------------
Why the unchecked?


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:158
+
+    llvm::MCRegisterInfo *MRI = nullptr;
+    if (abi)
----------------
``` llvm::MCRegisterInfo *MRI = abi ? &abi->GetMCRegisterInfo() : nullptr;```


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2799
+                                        addr_t &high_pc) const {
+  // This relocates low_pc and high_pc by adding the difference between the
+  // function file address, and the actual address it is loaded in memory.
----------------
Should this be a doxygen comment on the function itself?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71003