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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits