This is an archive of the discontinued LLVM Phabricator instance.

[intel-pt] Add a basic implementation of the dump command
AbandonedPublic

Authored by wallace on Aug 26 2020, 6:54 PM.

Details

Summary

Depends on D85705.
The previous diff left the Dump command unimplemented, so I'm implementing it.
As we add more stuff into the intel-pt plugin, we'll dump more information. For now, showing the current settings seems okay.

The dump command will print the information of the currently selected target.

This diff is also important because it introduces Target as the owner of the Trace object. If a trace corresponds
to multiple targets, then all of then will share the same Trace.

Diff Detail

Event Timeline

wallace created this revision.Aug 26 2020, 6:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2020, 6:54 PM
wallace requested review of this revision.Aug 26 2020, 6:54 PM
JDevlieghere added inline comments.Aug 26 2020, 7:28 PM
lldb/include/lldb/Target/Target.h
1106

Who owns the trace? If there's a 1:1 relationship between a trace and a target, can we make the target its owner? I'm trying to avoid adding shared pointers if possible.

1361

Doxygen comment?

wallace added inline comments.Aug 26 2020, 10:39 PM
lldb/include/lldb/Target/Target.h
1106

Well, there's a 1 to many relationship. Many targets can own the same trace object, as a single trace can have data of several processes. On the other hand, there should no other object that could own a trace. I haven't found a better solution :(

1361

good catch

wallace added inline comments.Aug 26 2020, 10:52 PM
lldb/include/lldb/Target/Target.h
1106

What do you think about changing GetTrace() to returns a Trace & instead of TraceSP &, so that no other object can share the ownership?

clayborg requested changes to this revision.Aug 27 2020, 12:38 PM
clayborg added inline comments.
lldb/include/lldb/Target/Target.h
1106

So this is an instance of a Trace plug-in for a given set of data described by the JSON. Since trace files can contain multiple processes, this will need to be shared between multiple Target instances if the "trace load" decides to load all processes, not just one. So I think this should remain a TraceSP so that an instance of the trace plug-in can be shared.

1361

rename to "m_trace_sp" to indicate shared pointer here. See my previous comment for why we need a share pointer.

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
54–87

Since one Trace plug-in instance can contain trace data for one or more processes, would it make sense to have more arguments here? "bool verbose" which would enable dumping of settings? "lldb::pid_t pid" to dump the data for just one process? "lldb::tid_t tid" to dump the data for just one thread?

Or we might want to add functions to lldb_private::Target that can dump the trace data? Like maybe:

void Target::DumpTrace(Stream &strm, lldb::tid_t tid = LLDB_INVALID_THREAD_ID) {
  if (!m_trace_sp)
    strm.PutCString("error: no trace data in target");
  ThreadSP thread_sp;
  if (tid != LLDB_INVALID_THREAD_ID)
    thread_sp = m_process_sp->FindThreadByID(tid);
  m_trace_sp->Dump(strm, m_process_sp.get(), thread_sp.get());
}

Then the Trace::Dump would look like:

void Trace::Dump(Stream &strm, Process *process, Thread *thread, bool verbose);

If there is no process, dump it all trace data for all processes and all threads in each process.
If there is a process and no thread, dump all threads from that process.
If there is a process and thread, dump just the data for that thread

The "trace dump" command can then compose calls using this API for any arguments might receive?

Dump all processes all threads:

(lldb) trace dump

Dump all threads for the specified processes:

(lldb) trace dump --pid 123  --pid 234

Dump all one thread from one process:

(lldb) trace dump --pid 123 --tid 345

If the --tid is specified, then we can have only one "--pid" argument.

55–63

Settings should probably be only dumped if the "bool verbose" argument is true?

lldb/source/Target/Target.cpp
2969

Should we return a "Trace *" from this? If nullptr, then there is no trace data, else we have valid trace data? No one other than Target instances should be adding to the ref count. But I guess this is safer as if someone wants to use the returned Trace pointer, some other thread could free it and cause a crash. So maybe leaving as a TraceSP is a good idea...

lldb/test/API/commands/trace/TestTraceLoad.py
50

We should probably test --pid and --thread here if we decide the APIs.

This revision now requires changes to proceed.Aug 27 2020, 12:38 PM
labath added a subscriber: labath.Aug 28 2020, 2:42 AM

I think it would be appropriate to discuss the design of this feature on lldb-dev before going over the individual patches. One of the fundamental aspects of this patchset that I think should not be overlooked is that it essentially adds a new level of structure ( a "Target Group") lldb. One of the questions I'd have is whether this concept shouldn't be formalized somewhere instead of it existing just virtually as the group of threads that happen to share the same trace object.

I think it would be appropriate to discuss the design of this feature on lldb-dev before going over the individual patches. One of the fundamental aspects of this patchset that I think should not be overlooked is that it essentially adds a new level of structure ( a "Target Group") lldb. One of the questions I'd have is whether this concept shouldn't be formalized somewhere instead of it existing just virtually as the group of threads that happen to share the same trace object.

The idea was to iterate on the design of the trace feature using these patches and have the discussion here. The idea is one trace file can create N individual targets. Each target can be independent and the threads contained in each belong to each target. "trace dump" when no args are supplied it could maybe only dump the currently selected target to start with? I am not sure what the right semantics are, but that shouldn't stop us with proceeding with these patches IMHO. It will be easy to modify as we evolve if we do want to have target groups, but I don't think it is required for these patches. The target group is a much higher level design that can affect many things and could be used in this case, but isn't required. If a trace plug-in needs to iterate over all of the targets this can be achieved by using the debugger to grab all targets and iterate over them, so no real group is required.

So it would simplify things right now if we say that "trace dump" dumps the trace data for the currently selected target right now. That will map well with the stepping commands that will soon be added to allow forward and reverse traversal of the trace data.

wallace marked an inline comment as not done.Aug 28 2020, 11:26 AM

I agree with that Greg said.

So it would simplify things right now if we say that "trace dump" dumps the trace data for the currently selected target right now. That will map well with the stepping commands that will soon be added to allow forward and reverse traversal of the trace data.

I'll go ahead with this in order to simplify the code. We can revisit that later

wallace updated this revision to Diff 288751.Aug 28 2020, 7:24 PM

Addressed comments

  • Now the "dump" command specifically refers to the currently selected thread. That will be aligned with the future stepping commands we will be adding.
  • The format of the command is "trace dump [-t tid]"
  • The verbose option is used to dump the original JSON settings
  • For now, I'm printing the trace file of each thread. Eventually we'll have more information about each thread.
wallace marked 6 inline comments as done.Aug 28 2020, 7:25 PM
wallace updated this revision to Diff 289011.Aug 31 2020, 12:50 PM

Rebase. Had to add several const qualifiers to match the const of Dump, which is a good thing.

clayborg requested changes to this revision.Aug 31 2020, 3:11 PM

I think we should probably make a TraceDumpOptions class to contain all of the dumping options and use that in the Target.h, Trace.h and all plug-ins. We will no doubt expand the number of options in the future and add more. We current have "tid" and "verbose". See inlined comments for details!

lldb/include/lldb/Target/Target.h
1126–1127

So we are going to want probably more options for this in the future. We currently have "tid" and "verbose". We probably also need to specify the process. If we add more options in the future, we would need to add more arguments here. One way around this is to create a "TraceDumpOptions" structure and pass it in:

struct TraceDumpOptions {
  Stream *s;
  Process *process; // If NULL, dump all processes
  std::vector<lldb::tid_t> tids; // Thread IDs, if empty dump all threads
  bool verbose;
};

void DumpTrace(const TraceDumpOptions &trace_options);
1321–1326

Remove whitespace only changes.

lldb/include/lldb/Target/Trace.h
67–68

Maybe use "struct TraceDumpOptions" as mentioned before?

lldb/source/Commands/CommandObjectTrace.cpp
164–169

Should we be able to specify this more than once?

(lldb) trace dump --tid 123 --tid 234
178–179

replace with TraceDumpOptions struct:

TraceDumpOptions m_dump_options;

Then fill in this structure instead of m_verbose and m_tid.

196–197
target.DumpTrace(m_options.m_dump_options);

Again this will allow us to add new dumping options easily in the future.

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
21–23

Use TraceDumpOptions

lldb/source/Target/Target.cpp
2971

Use TraceDumpOptions. You will want to populate the "Process *process;" setting from the target's process and anything else in the dump settings that isn't filled in already that is target specific.

This revision now requires changes to proceed.Aug 31 2020, 3:11 PM
wallace updated this revision to Diff 289300.EditedSep 1 2020, 3:51 PM
  • Now using the TraceDumpOptions struct
  • Added support for multiple thread-ids in the dump command. Included a test for this
clayborg requested changes to this revision.Sep 2 2020, 5:27 PM

I think we should remove the "Stream *s;" from the TraceDumpOptions struct since we have a "Stream &s" as an argument already. We can't use a reference in the TraceDumpOptions without jumping through a lot of hoops.

lldb/include/lldb/Target/Target.h
1127

We don't need the stream option as an argument if the TraceDumpOptions has a stream.

lldb/include/lldb/Target/Trace.h
23

We don't need the stream if the dump method takes this as an argment.

23

inline init if we leave this in here:

Stream *s = nullptr;
24

inline init:

Process *process = nullptr;
26

inline init:

bool verbose = false;
58

Might be nice to leave Stream as an option here since we can use a reference and remove the "Stream *" from the TraceDumpOptions struct.

This revision now requires changes to proceed.Sep 2 2020, 5:27 PM
labath added a comment.Sep 3 2020, 6:39 AM

I think it would be appropriate to discuss the design of this feature on lldb-dev before going over the individual patches. One of the fundamental aspects of this patchset that I think should not be overlooked is that it essentially adds a new level of structure ( a "Target Group") lldb. One of the questions I'd have is whether this concept shouldn't be formalized somewhere instead of it existing just virtually as the group of threads that happen to share the same trace object.

The idea was to iterate on the design of the trace feature using these patches and have the discussion here. The idea is one trace file can create N individual targets. Each target can be independent and the threads contained in each belong to each target. "trace dump" when no args are supplied it could maybe only dump the currently selected target to start with? I am not sure what the right semantics are, but that shouldn't stop us with proceeding with these patches IMHO. It will be easy to modify as we evolve if we do want to have target groups, but I don't think it is required for these patches. The target group is a much higher level design that can affect many things and could be used in this case, but isn't required. If a trace plug-in needs to iterate over all of the targets this can be achieved by using the debugger to grab all targets and iterate over them, so no real group is required.

So it would simplify things right now if we say that "trace dump" dumps the trace data for the currently selected target right now. That will map well with the stepping commands that will soon be added to allow forward and reverse traversal of the trace data.

I agree with that Greg said.

I don't think a review like this is good for a broader discussion because it obscures the woods by lots of individual trees. Like, I could write a page about what I don't like about the Target::SetTrace method, but I don't think that's the most important thing to sort out right now. Before going into the details of how trace dump options should be passed around, I think we should have some understanding and agreement about the feature. Like a short introduction to the problem, mentioning what is in scope for the project, what isn't; what are the new user-facing commands/apis being added, new concepts; and finally a very brief overview of new important new lldb internal classes. It doesn't have to be as polished as the memory tagging RFC (that was probably the best rfc lldb has ever had, and I feel guilty for not responding to it), something simpler will also do. I'm sure the two of you have talked about this a lot, and already have something to say about all of the points above, but I don't know how many other people do. And without that its hard to evaluate whether any particular patch is good or not, and reduces the comments to "i think this should be a unique pointer".

Even if noone responds to the rfc, it's not a waste because anyone can go back to it for details when reviewing any particular patch.

Fair enough. I'll work on a document. Hopefully the feedback is going to be good :)

wallace updated this revision to Diff 290605.Sep 8 2020, 4:34 PM

Addresses Greg's comments.

As a side note, I'll work on a RFC for the LLDB mailing list to get feedback on the overall design of the feature, as @labath suggested. However, Greg and I want to get these patches in, as we need this functionality soon. We can easily make modifications whenever suggestions come, following an iterative approach to development.

wallace updated this revision to Diff 290607.Sep 8 2020, 4:38 PM
  • format
wallace updated this revision to Diff 295145.Sep 29 2020, 4:16 PM

Rebased and did some code cleanup. The output of the dump command looks like this

Settings directory:

/home/wallace/llvm-sand/external/llvm-project/lldb/test/API/commands/trace/intelpt-trace

Trace files:

pid: '1234', tid: '3842849', trace file: /home/wallace/llvm-sand/external/llvm-project/lldb/test/API/commands/trace/intelpt-trace/3842849.trace

Modules:

uuid: 6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A, load address: 0x0000000000400000, file: /home/wallace/llvm-sand/external/llvm-project/lldb/test/API/commands/trace/intelpt-trace/a.out
clayborg requested changes to this revision.Sep 30 2020, 5:51 PM

See inlined comments.

lldb/include/lldb/Target/Target.h
31

You don't need this as the only "Trace" in this file is the "TraceSP" which is a forward declaration. You will need to add it to the Target.cpp file though.

1211–1212

remove whitespace only changes

1321

remove whitespace only changes

1324

remove whitespace only changes

lldb/source/Commands/CommandObjectTrace.cpp
184

s/from/in/ in the text?

185

add the "eCommandRequiresTarget" at the end here to indicate this command requires a valid target. It will then never even get to the DoExecute(...) function if there isn't a valid target.

195–197

If we add eCommandRequiresTarget to the constructor, then we can just call GetSelectedTarget(). The dummy target won't do us any good. This also means we don't have to add a test for the target being valid for "trace dump" since the eCommandRequiresTarget is already tested!

lldb/source/Commands/Options.td
1182–1183

Can or should this be able to be specified more than once?

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
140–142

remove braces for single line if statement.

lldb/test/API/commands/trace/TestTraceDump.py
19

This might change to "invalid target" if we use eCommandRequiresTarget in the command object. If so, we will need to create a target with no trace data and still test that we can get this error.

43

I would either use all "-t" or all "--thread-id" here.

This revision now requires changes to proceed.Sep 30 2020, 5:51 PM
wallace marked 21 inline comments as done.Oct 1 2020, 12:54 PM
wallace added inline comments.
lldb/include/lldb/Target/Target.h
31

It's needed for accessing TraceDumpOptions

wallace updated this revision to Diff 295655.Oct 1 2020, 12:57 PM

comments addressed

clayborg accepted this revision.Oct 1 2020, 1:01 PM

Just one nit about forward declaring the TraceDumpOptions.

lldb/include/lldb/Target/Target.h
31

Add TraceDumpOptions to lldb-forward.h and then this won't be needed here. Anytime you only have pointers or references to objects in headers, we should use forward declarations.

This revision is now accepted and ready to land.Oct 1 2020, 1:01 PM
Harbormaster completed remote builds in B73705: Diff 295656.
clayborg requested changes to this revision.Oct 1 2020, 1:33 PM

Some active discussion on the Trace RFC right now that might change the outcome of this patch. Holding off accepting until we resolve.

This revision now requires changes to proceed.Oct 1 2020, 1:33 PM