[Lldb-commits] [PATCH] D124640: [trace][intelpt] Support system-wide tracing [2] - Add a dummy --per-core-tracing option

Mon May 2 08:20:09 PDT 2022

jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.

Before doing a complete review can you provide clarity on the decision to only support perCore for process wide (see my inline comment with my thoughts/questions)?  Understanding this will potentially provide the answers to many other questions I was going to leave, so figured I'd ask it ahead of time (:

================
Comment at: lldb/docs/lldb-gdb-remote.txt:369
+//
+//     /* process tracing only */
 //     "processBufferSizeLimit": <decimal integer>,
----------------
Why do we only want this option for process tracing?
Per cpu tracing collects all trace data agnostic to a user specified process/thread, so why should this only be exposed for process wide? I think it makes more sense to decouple the `perCoreTracing` option from process/threads specific options entirely so it is its own option all together and cannot be used in conjunction with process/thread options.
If there is reason to not go down that route, we then should also add support for `perCoreTracing` with the  thread tracing option, not just the process tracing option as I feel it doesn't make since to only expose this for process tracing since it's doing the same thing behind the scenes.

================
Comment at: lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp:66
     OptionParsingStarting(ExecutionContext *execution_context) {
-  m_thread_buffer_size = kDefaultThreadBufferSize;
+  m_trace_buffer_size = kDefaultTraceBufferSize;
   m_enable_tsc = kDefaultEnableTscValue;
----------------
So now m_trace_buffer_size is the size of each trace buffer, regardless of the buffer is for a single thread or a single core?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124640