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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/Target/TraceCursor.h:162-166
+  /// - In terms of efficiency, moving the cursor to a given id should be as
+  /// fast
+  ///   as possible, but not necessarily O(1). That's why the recommended way to
+  ///   traverse sequential instructions is to use the \a TraceCursor::Next()
+  ///   method and only use \a TraceCursor::GoToId(id) sparingly.
----------------
fix wrap here. Might have been a clang format change


================
Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:50
+  ///     Additional options for configuring the dumping.
+  TraceInstructionDumper(lldb::TraceCursorUP &&cursor_up, Stream &s,
+                         const TraceInstructionDumperOptions &options);
----------------
Do we want the stream in the options?


================
Comment at: lldb/source/Commands/Options.td:1126-1128
+    Desc<"Continue dumping instructions right where the previous invocation of "
+    "this command was left, or from the beginning if this is the first "
+    "invocation. The --skip and --id arguments are discared if provided.">;
----------------
this option might not be needed. Each LLDB command line command can save information for how to continue a subsequent command. For example:

```(lldb) memory read $pc
0x100002ecf: 48 8d 7d f0 48 8d 35 a6 ff ff ff e8 51 00 00 00  H.}.H.5.....Q...
0x100002edf: c7 45 ec 01 00 00 00 8b 75 ec 48 8d 3d 1e 10 00  .E......u.H.=...
(lldb) 
0x100002eef: 00 31 c0 e8 f5 0e 00 00 e9 00 00 00 00 e9 00 00  .1..............
0x100002eff: 00 00 8b 45 ec 83 c0 01 89 45 ec e9 d7 ff ff ff  ...E.....E......
(lldb) 
0x100002f0f: 48 89 c1 89 d0 48 89 4d e0 89 45 dc 48 8d 7d f0  H....H.M..E.H.}.
0x100002f1f: e8 aa 0e 00 00 48 8b 7d e0 e8 7d 0e 00 00 0f 1f  .....H.}..}.....
```
Note that I just hit enter after the first read memory. Can we just take advantage of this feature instead of addind the "--continue" option?


================
Comment at: lldb/source/Target/TraceInstructionDumper.cpp:28
+    if (!m_cursor_up->GoToId(*m_options.id)) {
+      s.Printf("    invalid instruction id\n");
+      SetNoMoreData();
----------------
When there is no formatter, you can just use Stream::PutCString(...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122254