[Lldb-commits] [PATCH] D100740: [trace] Dedup different source lines when dumping instructions + refactor

[Greg Clayton via Phabricator via lldb-commits]

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