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

[walter erquinigo via Phabricator via lldb-commits]

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


================
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.
----------------
jj10306 wrote:
> how would you have two different symbol info for the first and last? is last referring to the symbol of the next function?
the SymbolInfo class contains disassemblies of the instruction and source line information, so technically two different instructions will always give you two different SymbolInfo objects. 


================
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
----------------
jj10306 wrote:
> nit:is the Optional<UP> redundant? thoughts on just using the UP and mentioning nullptr indicates there is no nested call
I'll turn these structs into classes with stricter safety


================
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
----------------
jj10306 wrote:
> are these functions transferring ownership of these block pointers (ie are you responsible for freeing this mem)?
there's no ownership transfer. They just expose the underlying pointers.


================
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
----------------
jj10306 wrote:
> "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?
now really. This function is called only from a switch that operates on the previous instruction kind, not the current instruction kind


================
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.
----------------
jj10306 wrote:
> 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?
it's still true if you pop frames out from the stack, but if you return using a jmp, then anything could happen.


================
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.
----------------
jj10306 wrote:
> 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?
yes, and we won't solve this for the time being. We can leave this for next year. 


================
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]
+
----------------
jj10306 wrote:
> 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?
> 
> 
s/events/trace items

and yes. It was a hot loop in the same function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135917