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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D125509#3586928 <https://reviews.llvm.org/D125509#3586928>, @zequanwu wrote:

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

Ok, fair enough.

I noticed one issue (described inline) but other than that, I think this should be ok.



================
Comment at: lldb/include/lldb/Expression/DWARFExpressionList.h:29-31
+      : m_module_wp(), m_dwarf_cu(dwarf_cu) {
+    if (module_sp)
+      m_module_wp = module_sp;
----------------
I believe this is equivalent.


================
Comment at: lldb/include/lldb/Expression/DWARFExpressionList.h:103
+                    const DWARFExpression &rhs) const {
+      return true;
+    }
----------------
`false` would be more correct here (`(a<b) == false && (b<a) == false` implies `a==b`, but `a<b && b<a` is nonsense)


================
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,
----------------
zequanwu wrote:
> 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`.
Ok. If that's the only problem, then I think we can fix that by moving `ReadAddressFromDebugAddrSection` to the DWARFUnit class (if it does not contain something like that already). IIUC, this is equivalent to `getAddrOffsetSectionItem` in llvm DWARFUnit, so I'd give it a similar name.


================
Comment at: lldb/source/Expression/DWARFExpressionList.cpp:109
+    return false;
+  return expr->MatchesOperand(frame, operand);
+}
----------------
Could we make `MatchesOperand` const (and ditch the Mutable calls above)?


================
Comment at: lldb/source/Expression/DWARFExpressionList.cpp:179
+  if (m_exprs.GetSize() == 1) {
+    expr = m_exprs.Back()->data;
+  } else {
----------------
I don't think this is correct.

We should be able to distinguish between a location list that is always valid (i.e., one which is not a location list at all), and a location list which happens to contain a single valid range.

That could probably be achieved by making the range of the always-valid entry be (0, UINT_MAX), but maybe it would be better to have an explicit fallback/default entry for this (one that is used when no ranged entry applies). This is how the default location entries in DWARF5 are supposed to work, although I am not sure if any compiler actually makes use of them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125509