[Lldb-commits] [PATCH] D127001: [trace][intelpt] Support system-wide tracing [16] - Create threads automatically from context switch data in the post-mortem case

[walter erquinigo via Phabricator via lldb-commits]

wallace added inline comments.
Herald added a subscriber: Michael137.


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp:167-170
+          // A list of known threads for the given process. When context switch
+          // data is provided, LLDB will automatically create threads for the
+          // this process whenever it finds new threads when traversing the
+          // context switches.
----------------
jj10306 wrote:
> So should this be an empty list or will the key not exist at all in the case that context switches are provided?
this should be an empty list. I've improved this documentaiton a bit


================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp:212-215
+  "dontCreateThreadsFromContextSwitches"?: boolean,
+    // If this is true, then the automatic creation of threads from context switch
+    // data is disabled, and thus only the threads provided in the "processes.threads"
+    // section will be created.
----------------
jj10306 wrote:
> is this necessary? can't this be inferred by if the threads key isn't present/the list is empty?
> also, is this even being used rn? It appears to me that you always invoke `CreateThreadsFromContextSwitches()` if `session.cores` is present?
lol, i forgot to delete this


================
Comment at: lldb/test/API/commands/trace/intelpt-multi-core-trace/trace_missing_threads.json:31-35
+      "threads": [
+        {
+          "tid": 3497234
+        }
+      ]
----------------
jj10306 wrote:
> shouldn't this be gone or just an empty list depending on how we want to implement it?
i want to here here the hybrid situation of having some explicit threads here and others inferred


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127001