[Lldb-commits] [PATCH] D89408: [trace] rename ThreadIntelPT to TraceThread

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D89408#2333502 <https://reviews.llvm.org/D89408#2333502>, @wallace wrote:

> Addressed the issues that @labath pointed out regarding the TraceProcess and TraceThread:
>
> - These classes were not together
> - TraceProcess was an optional plug-in, which would break the Trace parsing logic if plugged-out
>
> So I ended up moving TraceProcess and TraceThread next to Trace in LLDB core, guaranteeing that the "trace" process plug-in will always exist and now the source file structure looks better.

Yes, that looks better. My only quibble is about the naming. The prevailing lldb convention is to add a suffix to the subclass name, not prefix. So I think we should standardize on ProcessTrace & ThreadTrace and not the other way around.

> Btw, it doesn't seem to me a good idea to not have TraceProcess as a plug-in even inside LLDB core, as the main API for constructing processes in Target assumes that all processes are plug-ins. I'd rather keep one single simple API instead of having two or more.

Yeah, I don't think this is ideal, but it's not that bad either...



================
Comment at: lldb/include/lldb/Target/TraceSessionFileParser.h:18
 
+typedef std::shared_ptr<TraceThread> TraceThreadSP;
+
----------------
This would be better next to the definition of TraceThread class.


================
Comment at: lldb/include/lldb/Target/TraceSessionFileParser.h:87-101
+  /// Parse the fields common to all trace session schemas.
+  ///
+  /// \param[in] session
+  ///     The session json objects already deserialized.
+  ///
+  /// \return
+  ////    A list of \a ParsedProcess containing all threads and targets created
----------------
I don't understand the value of this function. Why couldn't the caller just call `ParseProcesses(session.processes)`? 

Are you expecting this to grow and access other subfields of the `session` object? In that case, maybe it would be better for all `JSONTraceSession<T>`s to extend a common (non-templated) base class (JSONTraceSettingsBase?) and then this function could take a reference to that?


================
Comment at: lldb/source/API/SystemInitializerFull.cpp:74
 
   // Now shutdown the common parts, in reverse order.
   SystemInitializerCommon::Terminate();
----------------
No terminate?


================
Comment at: lldb/source/Target/TraceProcess.cpp:40
+bool TraceProcess::CanDebug(TargetSP target_sp, bool plugin_specified_by_name) {
   return plugin_specified_by_name;
 }
----------------
Maybe this should be just `false`? I don't think we'd ever want to create this object in response to a normal "launch" request...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89408