[Lldb-commits] [PATCH] D122254: [trace][intelpt] Introduce instruction Ids

[Greg Clayton via Phabricator via lldb-commits]

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