clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed.
================ Comment at: lldb/include/lldb/Target/TraceCursor.h:211-214 + /// A unique identifier for the instruction or error this cursor is + /// pointing to. Each Trace plug-in can decide the nature of these + /// identifiers and thus no assumptions can be made regarding its ordering + /// and sequentiality. ---------------- might be nice to clarify this definition a bit with info like: - does this need to be unique only within a single thread or does it need to be unique globally for any instruction in any thread within the process? - a bit of info on why this is needed and what it will be used for as this will help people know how to create it so they can make sure it is efficient. ================ Comment at: lldb/include/lldb/Target/TraceCursor.h:215 + /// and sequentiality. + virtual uint64_t GetId() const = 0; /// \} ---------------- ================ Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:44-47 + TraceInstructionDumper(lldb::TraceCursorUP &&cursor_up, bool forwards, + bool raw, bool show_tsc, llvm::Optional<uint64_t> id, + llvm::Optional<size_t> skip, Stream &s); ---------------- Since we are adding new options, and if this is going to go in the public API at some point, it might be nice to make a "TraceInstructionDumperOptions class which default constructs with good default values. This way when you need to add a new option, it won't change the API ================ Comment at: lldb/source/Commands/CommandObjectThread.cpp:2171-2177 m_count = kDefaultCount; - m_skip = 0; + m_skip = llvm::None; + m_id = llvm::None; m_raw = false; m_forwards = false; m_show_tsc = false; m_continue = false; ---------------- use TraceInstructionDumperOptions? ================ Comment at: lldb/source/Commands/CommandObjectThread.cpp:2187-2193 size_t m_count; - size_t m_skip; + llvm::Optional<size_t> m_skip; + llvm::Optional<uint64_t> m_id; bool m_raw; bool m_forwards; bool m_show_tsc; bool m_continue; ---------------- Use TraceInstructionDumperOptions? ================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:105 + +bool TraceCursorIntelPT::GoToId(uint64_t id) { + if (m_decoded_thread_sp->GetInstructions().size() <= id) ---------------- ================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:112 + +uint64_t TraceCursorIntelPT::GetId() const { return m_pos; } ---------------- ================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h:38 + bool GoToId(uint64_t id) override; + ---------------- ================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h:40 + + uint64_t GetId() const override; + ---------------- Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122254/new/ https://reviews.llvm.org/D122254 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits