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

[walter erquinigo via Phabricator via lldb-commits]

wallace marked 6 inline comments as done.
wallace added inline comments.


================
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) {
----------------
clayborg wrote:
> Can or should we get the disassembler in calculate_symbol_context above after calling address.CalculateSymbolContext(...)? 
i think it's fine as it is, as we don't always need a disassembly. I want to use this TraverseInstructionsWithSymbolInfo method for creating the call graph, and I don't need the disassembly for that


================
Comment at: lldb/source/Target/Trace.cpp:203
+      thread, position, direction,
+      [&](size_t index, Expected<lldb::addr_t> load_address) -> bool {
+        if (!load_address)
----------------
clayborg wrote:
> Can this function really be called without a valid load address?
yes, whenever there are gaps in the trace


================
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) {
----------------
clayborg wrote:
> 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?
Any kind of errors are represented as failed instructions, which means that if the entire operation failed, there's one single instruction with an error message. If the thread is not traced at all, then this method return None. 
There can be errors while getting the data or errors while decoding individual instructions. That was a suggestion by Labath and I think this makes the code simpler, as there is only one way to handle errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100740