This is an archive of the discontinued LLVM Phabricator instance.

[trace][intelpt] Introduce instruction Ids
ClosedPublic

Authored by wallace on Mar 22 2022, 12:43 PM.

Details

Summary

In order to support quick arbitrary access to instructions in the trace, we need
each instruction to have an id. It could be an index or any other value that the
trace plugin defines.

This will be useful for reverse debugging or for creating callstacks, as each
frame will need an instruction id associated with them.

I've updated the thread trace dump instructions command accordingly. It now
prints the instruction id instead of relative offset. I've also added a new --id
argument that allows starting the dump from an arbitrary position.

Diff Detail

Event Timeline

wallace created this revision.Mar 22 2022, 12:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2022, 12:43 PM
wallace requested review of this revision.Mar 22 2022, 12:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2022, 12:43 PM
zrthxn accepted this revision.Mar 22 2022, 12:50 PM
This revision is now accepted and ready to land.Mar 22 2022, 12:50 PM
jj10306 requested changes to this revision.Mar 22 2022, 1:19 PM

Couple minor things

lldb/include/lldb/Target/TraceInstructionDumper.h
72
lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
60

Should this return an unsigned int? Currently, it appears the return value will always be >= 0

lldb/source/Target/TraceInstructionDumper.cpp
33

nit: Given that we have SeekType::End, why not rename SeekType::Set, to SeekType::Start? SeekType::Set was a little confusing to me the first time I read it.

This revision now requires changes to proceed.Mar 22 2022, 1:19 PM
wallace updated this revision to Diff 417389.Mar 22 2022, 1:43 PM
wallace marked 2 inline comments as done.

address comments

jj10306 accepted this revision.Mar 22 2022, 1:51 PM

nice work!

This revision is now accepted and ready to land.Mar 22 2022, 1:51 PM
clayborg requested changes to this revision.Mar 22 2022, 4:16 PM
clayborg added inline comments.
lldb/include/lldb/Target/TraceCursor.h
248–251

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.
252
lldb/include/lldb/Target/TraceInstructionDumper.h
55–58

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

lldb/source/Commands/CommandObjectThread.cpp
2170–2172

use TraceInstructionDumperOptions?

2182–2184

Use TraceInstructionDumperOptions?

lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
116
123
lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
38
40
This revision now requires changes to proceed.Mar 22 2022, 4:16 PM
wallace updated this revision to Diff 417827.Mar 23 2022, 10:18 PM
  • improve documentation
  • use lldb::user_id_t
  • add the new TraceInstructionDumperOptions struct
wallace marked 10 inline comments as done.Mar 23 2022, 10:30 PM
clayborg added inline comments.Mar 24 2022, 12:30 PM
lldb/include/lldb/Target/TraceCursor.h
162–166

fix wrap here. Might have been a clang format change

lldb/include/lldb/Target/TraceInstructionDumper.h
50

Do we want the stream in the options?

lldb/source/Commands/Options.td
1129–1130

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?

lldb/source/Target/TraceInstructionDumper.cpp
28

When there is no formatter, you can just use Stream::PutCString(...)

wallace updated this revision to Diff 418032.Mar 24 2022, 1:37 PM

fix some comments

wallace added inline comments.Mar 24 2022, 1:50 PM
lldb/include/lldb/Target/TraceInstructionDumper.h
50

we can't do it nicely, because we are adding a TraceInstructionDumper variable in CommandObjectTraceDumpInstructions::CommandOptions, where we don't have a stream available. I imagine this being the only variable that we can't put in the options, so it should be okay

wallace updated this revision to Diff 418361.Mar 25 2022, 6:14 PM
wallace marked 2 inline comments as done.

Some updates:

  • Modified thread trace dump instructions to accept one single thread instead of many. The reason is that, with the new --id argument, traversing multiple threads doesn't make sense anymore, as the id might only exist in one single thread. Besides that, it's really not very useful to analyze multiple threads in parallel by chunks. You normally want to analyze one of them at a time.
  • 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.
  • I can't pass the Stream to TraceInstructionDumperOptions because when that object is crea ted the Stream is not yet available.
clayborg added inline comments.Mar 28 2022, 11:51 AM
lldb/source/Commands/CommandObjectThread.cpp
2208

This " repeat" is pretty hacky here. If we can't get away without adding anything, I would prefer the --continue option we had before

2247–2249

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?

wallace updated this revision to Diff 418680.Mar 28 2022, 12:43 PM
  • Bring back the --continue command.
jj10306 accepted this revision.Apr 5 2022, 2:02 PM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 6 2022, 12:19 PM
This revision was automatically updated to reflect the committed changes.