[Lldb-commits] [PATCH] D90729: [trace][intel-pt] Scaffold the 'thread trace start | stop' commands

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

LGTM. A few inline nits, but nothing major. I only question if "RecordedProcess" is the right name for the base class for a core file/trace file, but don't have much of a better name. Maybe SerializedProcess? SavedProcess? Don't we usually have Pavel in on these reviews?



================
Comment at: lldb/include/lldb/Target/RecordedProcess.h:18
+/// Base class for all processes that don't represent a live process, such as
+/// coredumps or processes traced in the past.
+class RecordedProcess : public Process {
----------------
Might want to comment something like "common lldb_private::Process virtual functions overrides that are common between these kinds of processes can have default implementations in this class".


================
Comment at: lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp:209
   GetTarget().SetArchitecture(target_arch);
- 
+
   SetUnixSignals(UnixSignals::Create(GetArchitecture()));
----------------
undo space only change


================
Comment at: lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp:246-247
       if (exe_module_spec.GetFileSpec()) {
-        exe_module_sp = GetTarget().GetOrCreateModule(exe_module_spec, 
-                                                      true /* notify */);
+        exe_module_sp =
+            GetTarget().GetOrCreateModule(exe_module_spec, true /* notify */);
         if (exe_module_sp)
----------------
undo space only change


================
Comment at: lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp:766
 /// - NT_FILE - Files mapped into memory
-/// 
+///
 /// Additionally, for each thread in the process the core file will contain at
----------------
undo space only change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90729