[Lldb-commits] [PATCH] D135917: [lldb][trace] Add a basic function call dump [2] - Implement the reconstruction algorithm

[Jakob Johnson via Phabricator via lldb-commits]

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

this is awesome - the tests do a great job documenting the code and weird edge cases so thanks for that.
just some minor nits and questions, looks good overall



================
Comment at: lldb/include/lldb/Target/TraceDumper.h:86
+  ///   A traced segment is a maximal list of consecutive traced instructions
+  ///   that belong to the same call. A traced segment will end in three
+  ///   possible ways:
----------------
nit


================
Comment at: lldb/include/lldb/Target/TraceDumper.h:213-214
+      /// The symbol information of the delimiting instructions.
+      SymbolInfo first_symbol_info;
+      SymbolInfo last_symbol_info;
+      /// A nested call starting at the end of this segment.
----------------
how would you have two different symbol info for the first and last? is last referring to the symbol of the next function?


================
Comment at: lldb/include/lldb/Target/TraceDumper.h:216
+      /// A nested call starting at the end of this segment.
+      llvm::Optional<FunctionCallUP> nested_call;
+      /// Owning call
----------------
nit:is the Optional<UP> redundant? thoughts on just using the UP and mentioning nullptr indicates there is no nested call


================
Comment at: lldb/source/Target/TraceDumper.cpp:92-94
+  Block *inline_block_a =
+      insn.sc.block ? insn.sc.block->GetContainingInlinedBlock() : nullptr;
+  Block *inline_block_b = prev_insn.sc.block
----------------
are these functions transferring ownership of these block pointers (ie are you responsible for freeing this mem)?


================
Comment at: lldb/source/Target/TraceDumper.cpp:584
+
+/// Given an instruction that happens after a return, find the ancestor function
+/// call that owns it. If this ancestor doesn't exist, create a new ancestor and
----------------
"that happens after a return"
from looking at how this function's called from `AppendInstructionToFunctionCallForest` this appears to be the return instruction, not the instruction after a return. is this correct?


================
Comment at: lldb/source/Target/TraceDumper.cpp:617-619
+      // Note: If this is not robust enough, we should actually check if we
+      // returning to the instruction that follows the last instruction from
+      // that call, as that's the behavior of CALL instructions.
----------------
this would only be the case for well behaved CALL/RET, right? in the case of modified stack return or JMP, this wouldn't necessarily be true?


================
Comment at: lldb/source/Target/TraceDumper.cpp:783-785
+      // TODO: In case of a CPU change, we create a new root because we haven't
+      // investigated yet if a call tree can safely continue or if interrupts
+      // could have polluted the original call tree.
----------------
wouldn't this be a very common case where a thread is in the middle of executing a function, gets context switched out of CPU X, then after some time gets context switched into CPU Y and continues executing the function?


================
Comment at: lldb/test/API/commands/trace/TestTraceDumpFunctionCalls.py:31-32
+
+[call tree #0]
+m.out`foo() + 65 at multi_thread.cpp:12:21 to 12:21  [4, 19524]
+
----------------
this is saying that events 4 - 19524 were all executed by foo() and there weren't any other function calls?  am I understanding this output correctly?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135917