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

[David Blaikie via Phabricator via lldb-commits]

dblaikie added inline comments.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:72
+  }
+  llvm_unreachable("Fully covered switch!");
+}
----------------
I think usually the unreachable comment that's used in this sort of case is "Invalid LocationListFormat!" or similar, to give a bit more context about the situation.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:636
 
-  lldb::offset_t offset = 0;
-  lldb::addr_t base_address = m_loclist_addresses->cu_file_addr;
-  while (m_data.ValidOffset(offset)) {
-    // We need to figure out what the value is for the location.
-    addr_t lo_pc = LLDB_INVALID_ADDRESS;
-    addr_t hi_pc = LLDB_INVALID_ADDRESS;
-    if (!AddressRangeForLocationListEntry(m_dwarf_cu, m_data, &offset, lo_pc,
-                                          hi_pc))
-      break;
-
-    if (lo_pc == 0 && hi_pc == 0)
-      break;
-
-    if ((m_data.GetAddressByteSize() == 4 && (lo_pc == UINT32_MAX)) ||
-        (m_data.GetAddressByteSize() == 8 && (lo_pc == UINT64_MAX))) {
-      base_address = hi_pc;
-      continue;
-    }
-    RelocateLowHighPC(base_address, func_load_addr, lo_pc, hi_pc);
-
-    if (lo_pc <= addr && addr < hi_pc)
-      return true;
-
-    offset += m_data.GetU16(&offset);
-  }
-  return false;
-}
-
-bool DWARFExpression::GetLocation(addr_t func_load_addr, addr_t pc,
-                                  lldb::offset_t &offset,
-                                  lldb::offset_t &length) {
-  offset = 0;
-  if (!IsLocationList()) {
-    length = m_data.GetByteSize();
-    return true;
-  }
-
-  if (func_load_addr != LLDB_INVALID_ADDRESS && pc != LLDB_INVALID_ADDRESS) {
-    addr_t base_address = m_loclist_addresses->cu_file_addr;
-    while (m_data.ValidOffset(offset)) {
-      // We need to figure out what the value is for the location.
-      addr_t lo_pc = LLDB_INVALID_ADDRESS;
-      addr_t hi_pc = LLDB_INVALID_ADDRESS;
-      if (!AddressRangeForLocationListEntry(m_dwarf_cu, m_data, &offset, lo_pc,
-                                            hi_pc))
-        break;
-
-      if (lo_pc == 0 && hi_pc == 0)
-        break;
-
-      if ((m_data.GetAddressByteSize() == 4 && (lo_pc == UINT32_MAX)) ||
-          (m_data.GetAddressByteSize() == 8 && (lo_pc == UINT64_MAX))) {
-        base_address = hi_pc;
-        continue;
-      }
-
-      RelocateLowHighPC(base_address, func_load_addr, lo_pc, hi_pc);
-      length = m_data.GetU16(&offset);
-
-      if (length > 0 && lo_pc <= pc && pc < hi_pc)
-        return true;
-
-      offset += length;
-    }
-  }
-  offset = LLDB_INVALID_OFFSET;
-  length = 0;
-  return false;
+  return bool(GetLocationExpression(func_load_addr, addr));
 }
----------------
I'd probably write this as "... != None" rather than "bool(...)" - would make it clearer what the return type of GetLocationExpression is/what conversion is being done here.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2829
+    if (!loc)
+      LLDB_LOG_ERROR(log, loc.takeError(), "{0}");
+    if (loc->Range) {
----------------
Does LLDB_LOG_ERROR stop the program? If not, then the next line ("if (loc->Range)" will invoke UB when it tries to dereference the "loc" when it is unset.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71003