[Lldb-commits] [PATCH] D91679: [trace][intel-pt] Implement trace start and trace stop

Tue Jan 26 16:28:55 PST 2021

clayborg added a comment.

> The start and stop operations have 3 variants for now:
>
> - specificThreads: operate on specific threads (e.g. thread trace start 2 3 9)
> - currentAndFutureThreads: operate of all threads including future ones (e.g. thread trace start all)
> - process: operate on a process as a single trace instance, instead of operating on each thread. Not supported, but left there to make the code more generic. Eventually I'll have to implement this anyway. (e.g. maybe? process trace start)

So how does "currentAndFutureThreads" differ from "process"?

It might be better to have "thread trace start" be able to specify one or more threads (but not all), and use "process trace start" to enable tracing for all threads in this patch?

================
Comment at: lldb/docs/lldb-gdb-remote.txt:278
+//     "tids": [<thread ids in decimal>],
+//     "variant": "specificThreads" | "currentAndFutureThreads" | "process",
+//     "params": {
----------------
Seems like we don't need the "variant" here. If we have "tids" then we are enabling tracing for only that list of threads, and if it is missing, then we enable it for all threads.

See my questions in the non-inlined comment section about the "process" variant.

================
Comment at: lldb/docs/lldb-gdb-remote.txt:298-299
+//
+//  If "variant" is "process", then the process will be traced as a single
+//  trace instance instead of tracing each individual thread.
+//
----------------
still unclear what this means. If we don't have it implemented, then we should probably leave it out for now.

================
Comment at: lldb/docs/lldb-gdb-remote.txt:310
+//  {
+//    "bufferSizeInKB": <trace buffer size in KB in decimal>
+//    "perfConfig": <custom low-level perf event intel-pt config in decimal>
----------------
Is this the trace buffer size that will be used for each thread? For all threads? Is this size divided up between the threads? What happens when new threads get created? Can we run out of memory in the kernel if a process creates too many threads if this buffer is for each thread? Need clarification on what this number means.

================
Comment at: lldb/docs/lldb-gdb-remote.txt:311
+//    "bufferSizeInKB": <trace buffer size in KB in decimal>
+//    "perfConfig": <custom low-level perf event intel-pt config in decimal>
+//  }
----------------
Any more details on what this is? Number? string? Why is it needed and what does it control/do?

================
Comment at: lldb/docs/lldb-gdb-remote.txt:340
+//    "tids": [<thread ids in decimal>],
+//    "variant": "specificThreads" | "currentAndFutureThreads" | "process",
+//  }
----------------
Same comment as above. Why do we need the variant? If there is a "tids" then we do only those threads, else we do all threads in a process?

================
Comment at: lldb/docs/lldb-gdb-remote.txt:403
+//    "type": <tracing technology name, e.g. intel-pt, arm-coresight>
+//    "label": <identifier for the data>
+//    "tid": <tid in decimal if the data is related to a thread, 0 otherwise>
----------------
Are these custom for each plug-in? Any details on what intel-pt can specify here? You supplied intel PT specific info for other settings above in the docs.

================
Comment at: lldb/include/lldb/Target/Trace.h:138
   ///     The current position of the thread's trace or \b 0 if empty.
-  virtual size_t GetCursorPosition(const Thread &thread) = 0;
+  virtual size_t GetCursorPosition(Thread &thread) = 0;

----------------
Why was "const" remove from the "Thread" here?

================
Comment at: lldb/include/lldb/Target/Trace.h:192
   virtual void TraverseInstructions(
-      const Thread &thread, size_t position, TraceDirection direction,
+      Thread &thread, size_t position, TraceDirection direction,
       std::function<bool(size_t index, llvm::Expected<lldb::addr_t> load_addr)>
----------------
Why no const?

================
Comment at: lldb/include/lldb/Utility/TraceOptions.h:124-139
+struct TraceIntelPTThreadState {
+  lldb::tid_t tid;
+  size_t buffer_size_in_bytes;
+};
+
+struct TraceIntelPTCPUConfig {
+  uint16_t family;
----------------
We really shouldn't have any Intel PT stuff in this header file. It should be in the TraceIntelPT.h/.cpp file. We don't want each plug-in added their definitions in this global TraceOptions.h file

================
Comment at: lldb/include/lldb/Utility/TraceOptions.h:171-174
+bool fromJSON(const Value &value,
+              lldb_private::TraceIntelPTStartPacketParams &packet, Path path);
+
+Value toJSON(const lldb_private::TraceIntelPTStartPacketParams &packet);
----------------
move to TraceIntelPT.h/.cpp files. We don't want each plug-in added their definitions in this global TraceOptions.h file

================
Comment at: lldb/include/lldb/Utility/TraceOptions.h:202-215
+bool fromJSON(const Value &value, lldb_private::TraceIntelPTThreadState &state,
+              Path path);
+
+Value toJSON(const lldb_private::TraceIntelPTThreadState &state);
+
+bool fromJSON(const Value &value,
+              lldb_private::TraceIntelPTCPUConfig &cpu_config, Path path);
----------------
move to TraceIntelPT.h/.cpp files. We don't want each plug-in added their definitions in this global TraceOptions.h file

================
Comment at: lldb/include/lldb/lldb-enumerations.h:772-783
+enum TraceOperationVariant {
+  /// Operate on specific existing threads, each one as an independent trace
+  /// instance.
+  eTraceOperationVariantSpecificThreads = 0,
+  /// Operate on all existing threads along with future threads, each one as an
+  /// independent trace instance.
+  eTraceOperationVariantCurrentAndFutureThreads,
----------------
This is the public API header file. Move to lldb/include/lldb/lldb-private-enumerations.h

================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2048-2049
             "Defaults to the current thread. Thread indices can be "
             "specified as arguments.\n Use the thread-index \"all\" to trace "
-            "all threads.",
+            "all threads including future threads.",
             "thread trace stop [<thread-index> <thread-index> ...]",
----------------
It would be nice to remove the "all" scenario here and just implement "process trace start".

================
Comment at: lldb/source/Commands/CommandObjectThreadUtil.cpp:172
+
+  if (num_args > 0 && ::strcmp(command.GetArgumentAtIndex(0), "all") == 0)
+    variant = eTraceOperationVariantCurrentAndFutureThreads;
----------------
remove "all" and create "process trace start"?

================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1680
+            thread_id,
+            m_intel_pt_trace_new_threads_params->params.buffer_size_in_kb,
+            m_intel_pt_trace_new_threads_params->params.perf_config)) {
----------------
Having a fixed size of memory for each thread can be dangerous. What if the user specifies a really large value, and then creates thousands of threads? What will happen to the system? Will if fail gracefully? Can you deadlock your computer?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91679