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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
