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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2201
+    // command
+    if (cmd.find(" repeat") == std::string::npos)
+      cmd += " repeat";
----------------
This " repeat" is pretty hacky here. If we can't get away without adding anything, I would prefer the --continue option we had before


================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2254-2256
+      m_options.m_dumper_options.skip = 1;
+      m_options.m_dumper_options.id = m_last_id;
     }
----------------
to make repeat work, you are filling in the dumper options, can't we just store a copy of the old thread options and update the "id" field?

You also mentioned:



> I couldn't find a simple way to create the repeat command directly from GetRepeatCommand because it's not easy to quickly find what the next instruction is going to be in the repeat command. In fact, finding the id of that future instruction requires to actually traverse the instructions up to that point, but the traversal only happens in DoExecute, which is invoked after GetRepeatCommand. As a simple workarou nd, I'm adding a " repeat" positional argument that won't be parsed by CommandOptions but will still indicate that a repeat is happening. I was able to remove the --continue flag from Options.td, thus reducing the amount of code needed to handle the repeat. Not only that, I was able to get rid of the map<tid, TraceInstructionDumper> that I had in the CommandObject that I was using to continue the iteration in the repeat commands. Now I'm using the last iterated id to computed where the next one will be, thus creating a new brand new dumper with each command making this class simpler.

You seem to have the right "id" to start at here with m_last_id? Why can't we just store this in an extra copy of TraceInstructionDumperOptions and have it create the right command?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122254