clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.
================
Comment at: lldb/source/Target/Trace.cpp:163
+ /*inline_block_range*/ false, range) &&
+ range.ContainsFileAddress(address))
+ return prev_insn.sc;
----------------
AddressRange::ContainsFileAddress() can't be used here after looking at how it
is currently implemented. It will check if the sections are the same first and
return the correct answer if they are, otherwise it will just extract a file
addresses and compare them.
To do this right, we need to uncomment out the currently unused:
```
bool AddressRange::Contains (const Address &addr) const;
```
And we will need to fix and use it by making sure the modules match. If the
modules don't match, we don't want to extract two file addresses from two
different modules and then compare those, as two modules can both have a 0x1000
file address.
The current commented out implementation of AddressRange::Contains(...) is
wrong too. Fixing correctly looks like:
```
bool
AddressRange::Contains(const Address &addr) const
{
SectionSP rangeSectSP = GetBaseAddress().GetSection();
SectionSP addrSectSP = addr.GetSection();
if (rangeSectSP) {
if (!addrSectSP || rangeSectSP->GetModule() != addrSectSP->GetModule())
return false; // Modules do not match.
} else if (addrSectSP) {
return false; // Range has no module but "addr" does because addr has a
section
}
// Either the modules match, or both have no module, so it is ok to compare
// the file addresses in this case only.
return ContainsFileAddress(addr);
}
```
================
Comment at: lldb/source/Target/Trace.cpp:173
+ // instruction's disassembler when possible.
+ auto calculate_disass = [&](const Address &address, const SymbolContext &sc)
{
+ if (prev_insn.disassembler) {
----------------
Can or should we get the disassembler in calculate_symbol_context above after
calling address.CalculateSymbolContext(...)?
================
Comment at: lldb/source/Target/Trace.cpp:191
+ // We fallback to a single instruction disassembler
+ AddressRange range(address, arch.GetMaximumOpcodeByteSize() * 1);
+ DisassemblerSP disassembler = Disassembler::DisassembleRange(
----------------
Remove the " * 1" here
================
Comment at: lldb/source/Target/Trace.cpp:203
+ thread, position, direction,
+ [&](size_t index, Expected<lldb::addr_t> load_address) -> bool {
+ if (!load_address)
----------------
Can this function really be called without a valid load address?
================
Comment at: lldb/source/Target/Trace.cpp:210-211
+ insn.exe_ctx = exe_ctx;
+ target.GetSectionLoadList().ResolveLoadAddress(*load_address,
+ insn.address);
+ if (symbol_scope != 0)
----------------
Better to use:
```
bool Address::SetLoadAddress(lldb::addr_t load_addr, Target *target);
```
================
Comment at: lldb/source/Target/Trace.cpp:240-241
+ // symbol checks
+ if (!insn.sc.symbol && !prev_insn.sc.symbol)
return true;
+ else if (insn.sc.symbol != prev_insn.sc.symbol)
----------------
You can't just return if there is no symbol, you can have no symbol, but still
have a function from debug info if the symbol was a local symbol since it could
have been stripped from the executable at build time.
Just remove these two lines and the "else" below
================
Comment at: lldb/source/Target/Trace.cpp:252-260
+ if (IsLineEntryValid(insn.sc.line_entry) !=
+ IsLineEntryValid(prev_insn.sc.line_entry))
+ return false;
+ if (!IsLineEntryValid(insn.sc.line_entry) &&
+ !IsLineEntryValid(prev_insn.sc.line_entry))
+ return true;
+ else
----------------
================
Comment at: lldb/source/Target/Trace.cpp:310
size_t end_position, bool raw) {
- if (!IsTraced(thread)) {
+ Optional<size_t> instructions_count = GetInstructionCount(thread);
+ if (!instructions_count) {
----------------
Should this return a Expected<size_t>? In case there is an error we want to
show? Like maybe the trace buffer was truncated, or missing when it was
expected? Can we have a process that is traced, but by the time we fetch the
instructions, the circular buffer was filled with other data and even though
the thread was traced there is no data?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100740/new/
https://reviews.llvm.org/D100740
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
