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

[Zequan Wu via Phabricator via lldb-commits]

zequanwu added a comment.

> 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?

Then we need to duplicate the code that gets expression data and register kind before every call to `DWARFExpression::Evaluate`, making it less clean.

> 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.

Yes, the dwarf location list now is parsed at the time when parsing variable DIE. It used to be that a dwarf location list will be parsed every time when we try to access a dwarf expression inside.



================
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,
----------------
labath wrote:
> Could this go into the list class?
This function calls `ReadAddressFromDebugAddrSection` that is called multiple places in `DWARFExpression`. If we move it to `DWARFExpressionList`, we need to duplicate the `ReadAddressFromDebugAddrSection`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125509