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