[Lldb-commits] [PATCH] D125509: [LLDB][NFC] Decouple dwarf location table from DWARFExpression.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.
Herald added a subscriber: Michael137.

Overall, I like this. The main thing I am wondering is if it wouldn't be better to (instead of essentially duplicating the DWARFExpression interface in the DWARFExpressionList class) somehow split the process of finding the appropriate expression (for the given PC value or whatever), and the actual evaluation. This would be particularly nice if the arguments can be split in such a way that those that are used for locating the expression are not repeated in the "evaluate" call. Have you considered this?

Also, if I understand correctly, one of the side effects of this patch that the DWARF location list is now parsed more eagerly (when the containing object (e.g. variable) is constructed rather than during evaluation time). I think that's probably fine, but it's definitely worth calling out.



================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2581
 
-static DataExtractor ToDataExtractor(const llvm::DWARFLocationExpression &loc,
-                                     ByteOrder byte_order, uint32_t addr_size) {
-  auto buffer_sp =
-      std::make_shared<DataBufferHeap>(loc.Expr.data(), loc.Expr.size());
-  return DataExtractor(buffer_sp, byte_order, addr_size);
-}
-
-bool DWARFExpression::DumpLocations(Stream *s, lldb::DescriptionLevel level,
-                                    addr_t load_function_start, addr_t addr,
-                                    ABI *abi) {
-  if (!IsLocationList()) {
-    DumpLocation(s, m_data, level, abi);
-    return true;
-  }
-  bool dump_all = addr == LLDB_INVALID_ADDRESS;
-  llvm::ListSeparator separator;
-  auto callback = [&](llvm::DWARFLocationExpression loc) -> bool {
-    if (loc.Range &&
-        (dump_all || (loc.Range->LowPC <= addr && addr < loc.Range->HighPC))) {
-      uint32_t addr_size = m_data.GetAddressByteSize();
-      DataExtractor data = ToDataExtractor(loc, m_data.GetByteOrder(),
-                                           m_data.GetAddressByteSize());
-      s->AsRawOstream() << separator;
-      s->PutCString("[");
-      s->AsRawOstream() << llvm::format_hex(loc.Range->LowPC,
-                                            2 + 2 * addr_size);
-      s->PutCString(", ");
-      s->AsRawOstream() << llvm::format_hex(loc.Range->HighPC,
-                                            2 + 2 * addr_size);
-      s->PutCString(") -> ");
-      DumpLocation(s, data, level, abi);
-      return dump_all;
-    }
-    return true;
-  };
-  if (!GetLocationExpressions(load_function_start, callback))
-    return false;
-  return true;
-}
-
-bool DWARFExpression::GetLocationExpressions(
-    addr_t load_function_start,
-    llvm::function_ref<bool(llvm::DWARFLocationExpression)> callback) const {
-  if (load_function_start == LLDB_INVALID_ADDRESS)
-    return false;
-
-  Log *log = GetLog(LLDBLog::Expressions);
-
+bool DWARFExpression::ParseDWARFLocationList(
+    const DWARFUnit *dwarf_cu, const DataExtractor &data,
----------------
Could this go into the list class?


================
Comment at: lldb/source/Expression/DWARFExpressionList.cpp:111
+
+void DWARFExpressionList::ForAllLocations(
+    const std::function<bool(const Entry *)> &fn) const {
----------------
What would it take to make this class iterable? (i.e. have some begin/end methods instead of this foreach callback.


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc.s:43
 # CHECK:     Variable{{.*}}, name = "x1", {{.*}}, scope = parameter
-# CHECK:     Variable{{.*}}, name = "x2", {{.*}}, scope = parameter, location = 0x00000000: error: unexpected end of data
 # CHECK:     Variable{{.*}}, name = "x3", {{.*}}, scope = parameter, location =
----------------
This is probably fine, but it would be nice to at least log the errors somewhere (if we don't do it already).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125509