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

[Pavel Labath via Phabricator via lldb-commits]

labath 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;
----------------
JDevlieghere wrote:
> Should this be part of the DWARFExpression API? It feels like a static function might be sufficient. Maybe it could take a range directly?
It's a private member function, so it's not really a part of any API. However, given that now it's called from only a single place, it's not actually useful. It served a purpose in the preceding patch (I'm going to need a stamp on that too BTW), since there it had like five callers, but now that this patch centralizes them, I can just inline it.


================
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;
----------------
JDevlieghere wrote:
> Why the unchecked?
Because I've already checked that the offset is valid (that's the difference between the checked and unchecked versions, though I'm not really sure if we need them).


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:57
+
+static std::unique_ptr<llvm::DWARFLocationTable>
+GetLocationTable(DWARFExpression::LocationListFormat format, const DataExtractor &data) {
----------------
aprantl wrote:
> Doxygen comment?
I've added /something/, though I think the purpose is mostly obvious here.

Btw, this part is going to get refactored further, since the current design doesn't really permit mixing DWARF 4&5 location lists within a single object file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71003