This is an archive of the discontinued LLVM Phabricator instance.

[intel pt] Refactor parsing
ClosedPublic

Authored by wallace on Oct 5 2020, 11:20 AM.

Details

Summary

With the feedback I was getting in different diffs and RFC discussion, I realized that splitting the parsing logic into two classes was not easy to deal with. I do see value in doing that, but I'd rather leave that as a refactor after most of the intel-pt logic is in place. Thus, I'm merging the common parser into the intel pt one, having thus only one that is fully aware of Intel PT during parsing and object creation.

Additionally, now the parser is used to create a Trace instance as output, as @labath suggested, and I was able to remove methods like GetParser(). This makes the code more modular.

Besides, based on the feedback in https://reviews.llvm.org/D88769, I'm creating a ThreadIntelPT class that will be able to orchestrate decoding of its own trace and can handle the stop events correctly.

This leaves the TraceIntelPT class as an initialization class that glues together different components. Right now it can initialize a trace session from a json file, and in the future will be able to initialize a trace session from a live process.

Besides, I'm renaming SettingsParser to SessionParser, which I think is a better name, as the json object represents a trace session of possibly many processes.

With the current set of targets, we have the following

  • Trace: main interface for dealing with trace sessions
  • TraceIntelPT: plugin Trace for dealing with intel pt sessions
  • TraceIntelPTSessionParser: a parser of a json trace session file that can create a corresponding TraceIntelPT instance along with Targets, ProcessTraces (to be created in https://reviews.llvm.org/D88769), and ThreadIntelPT threads.
  • ProcessTrace: (to be created in https://reviews.llvm.org/D88769) can handle the correct state of the traces as the user traverses the trace. I don't think there'll be a need an intel-pt specific implementation of this class.
  • ThreadIntelPT: a thread implementation that can handle the decoding of its own trace file, along with keeping track of the current position the user is looking at when doing reverse debugging.

Diff Detail

Event Timeline

wallace created this revision.Oct 5 2020, 11:20 AM
Herald added a project: Restricted Project. · View Herald Transcript
wallace requested review of this revision.Oct 5 2020, 11:20 AM
wallace edited the summary of this revision. (Show Details)Oct 5 2020, 11:20 AM
wallace edited the summary of this revision. (Show Details)Oct 5 2020, 12:02 PM
clayborg requested changes to this revision.Oct 5 2020, 2:34 PM
clayborg added inline comments.
lldb/include/lldb/Target/Trace.h
77–80

Is this the top level object from the JSON file? I think the old name of "settings" was better?

95–96

Maybe some more comments here saying something about the fact that there are common settings required by all plug-ins and where to find this info, and also where this plug-in specific schema must appear within the JSON trace file? Maybe also mention that each trace plug-in can defined their own needed settings and that this information will appear in "bla bla" command if you type it so you can see the information required by each plug-in?

lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.h
52

Can we have different "pt_cpu" values for each thread? Or are we just storing this here because we don't control the Process class that we will use when doing IntelPT when we have a live process? Do we need this in the thread? Could we not just fetch the trace from the target and cast it to TraceIntelPT and call an accessor if we need the pt_cpu?

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
43

I still think "settings" is a better word for this argument.

61

"const llvm::json::Value &" instead of a pointer? This can't ever be NULL and it would be nice to not each plug-in think it might need to check for NULL?

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionParser.cpp
31–50 ↗(On Diff #296240)

This common information should still be parsed by code in Trace.cpp.

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionParser.h
21–42 ↗(On Diff #296240)

Seems like we should still parse this in Trace.cpp. Any common information shouldn't be duplicated or represented differently in different trace plug-ins IMHO.

lldb/source/Target/Trace.cpp
58–60

Why have the create_callback take an of these arguments if they are always null?

This revision now requires changes to proceed.Oct 5 2020, 2:34 PM
labath added inline comments.Oct 6 2020, 6:49 AM
lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.cpp
21

just =default, or even don't declare it all (as explained in the other thread, these declarations are completely redundant).

lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.h
10–11

Yep, all our header guards have been renamed now. Please don't reintroduce the old style.

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionParser.cpp
167 ↗(On Diff #296240)

std::make_shared

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionParser.h
21–59 ↗(On Diff #296240)

How much of this stuff needs to be in the header? If parsing happens completely within this class, I'd expect that these classes (and the relevant fromJSON methods) could be in the cpp file (in an anon namespace).

lldb/source/Target/Trace.cpp
58–60

It seems this is used to implement "trace schema". I think that a better way to implement this functionality would be to have each plugin pass its schema (as a string) when it registers itself (in TraceXXX::Initialize`). Then we can have an entry point like StringRef Trace::GetSchema(StringRef plugin_name) which will return that, and there won't be a need to create a polymorphic object just for the sake of calling it GetSchema on it (you can still have one if you need to get the schema for an existing object for any reason).

wallace updated this revision to Diff 296562.EditedOct 6 2020, 4:25 PM
wallace marked 10 inline comments as done.

Address issues based on this diff's comments and from conversations with Greg.

  • I'm still keeping the TraceSessionFile name instead of TraceSettings, as later there'll be actual trace settings (e.g. size of an Intel PT trace buffer, whether to trace in a ring or non-ring buffer, etc.).
  • As Greg asked, I'm bringing back the top-level TraceSessionFileParser class, which each plug-in should override to perform parsing. This time the implementation is much cleaner and the implementation class can decide what to reuse from the base class. This will help ensuring that all Trace plug-ins consistently use the same common schema and parsing logic.
  • I moved the fromJSON implementations to cpp files.
  • Improved comments overall.
  • Now, as Pavel suggested, I'm storing the schema in the PluginInstance, which allows fetching schemas without needing to instantiate any class. With this I was able able to simplify and remove several constructors.
  • Fixed the header guards
  • Some small fixes here are there, following the comments in the diff.
wallace marked 3 inline comments as done.Oct 6 2020, 4:27 PM
labath added a comment.Oct 8 2020, 8:41 AM

I like this. A lot of comments, but they're all cosmetic.

lldb/include/lldb/Target/Trace.h
96–97

delete completely. As this is an abstract class, making the constructor protected does not do anything.

lldb/include/lldb/Target/TraceSessionFileParser.h
10

I think it wants this to be LLDB_TARGET_TRACESESSIONPARSER_H

108

delete. "using namespace" in a header file is a big no-no.

110

delete inline

lldb/source/Commands/CommandObjectTrace.cpp
279

error = schemaOrErr.takeError()

lldb/source/Core/PluginManager.cpp
1044

Return StringRef

lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.cpp
24–26
lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.h
15

btw, (most of) new plugins tend to place themselves in a sub-namespace of lldb_private. That avoids prefixing everything with lldb_private::

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
25

drop data() and have this function take a StringRef ?

51

This could probably also return an Expected<TraceSP>

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
64

const auto &, or potentially even const TargetSP & due to this.

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp
19

You're using namespace llvm, but then still prefixing a lot of the stuff with llvm::. Is that because llvm::json:: is a mouthful? namespace json = llvm::json, maybe?

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h
81

This demonstrates why the previous "using namespace" is so bad. Now the entirety of lldb is accessible from within the llvm::json namespace. :D

lldb/source/Target/TraceSessionFileParser.cpp
99

End this with a newline ?

clayborg requested changes to this revision.Oct 8 2020, 1:34 PM
clayborg added inline comments.
lldb/include/lldb/Core/PluginManager.h
342

This is fine if we have a case where we want the scheme for a given plug-in. We should probably also add a call that takes a "size_t index" as an argument that is the index of the plug-ins. When the index is too big, returns NULL. This will allow us to iterate over all of the installed plug-ins and print out the schemas for each one?

lldb/include/lldb/Target/Trace.h
97

Do we also want a non static version of this to be able to get the schema for the current Trace plugin?

lldb/include/lldb/Target/TraceSessionFileParser.h
108

indeed! I will trickly through to any .cpp file that includes it

lldb/source/Commands/CommandObjectTrace.cpp
243–244

Make <plug-in> optional so we can list all trace plug-in schemas?

lldb/source/Plugins/Trace/intel-pt/ThreadIntelPT.h
15

I second this, add a plug-in namespace!

This revision now requires changes to proceed.Oct 8 2020, 1:34 PM
wallace marked 19 inline comments as done.Oct 8 2020, 3:26 PM
wallace updated this revision to Diff 297075.Oct 8 2020, 4:26 PM
  • Added “trace schema all”, following the pattern from "trace <command> all", and added a test for this.
  • Created a non-static version of GetSchema in Trace.h
  • Cannot delete Trace() {}, as there’s a compilation error. Could be because some constructors have been marked as deleted, so the compiler wants explicit declarations.
  • Created the lldb_private::trace_intel_pt namespace
  • Deleted all unnecessary namespace and “using namespace” usages.
  • Using StringRef more ubiquitously now and other small fixes requested by Pavel
labath accepted this revision.Oct 9 2020, 1:40 AM

Looks good.

  • Cannot delete Trace() {}, as there’s a compilation error. Could be because some constructors have been marked as deleted, so the compiler wants explicit declarations.

Right. I was going to say you can delete those too as they are already deleted on the base class. Except, it turns out, PluginInterface did not have them deleted. Well.. after 0610a25a85 it has. :)

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
19

I think LLDB_PLUGIN_DEFINE should work fine. The _ADV version is for (mostly legacy) cases where the plugin name does not match the class name.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 9 2020, 5:32 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
lldb/source/Target/TraceSessionFileParser.cpp