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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

See my inlined comments, but it would be great to be able to just trace a few threads if any "tids" are specified in the "jLLDBTraceStart" packet, and all threads using a per thread buffer and call that "process". We would still use a per thread buffer for "process" tracing, basically "currentAndFutureThreads" will become the equivalent of "process" tracing. Just have the Intel PT trace plug-in take care of it. We can avoid any use of the one buffer for the entire process issues you mentioned that way, and it effectively does what the user wants. Adding a max memory settings to the custom settings in "jLLDBTraceStart" for Intel PT sounds like a good idea as well.



================
Comment at: lldb/docs/lldb-gdb-remote.txt:278
+//     "tids": [<thread ids in decimal>],
+//     "variant": "specificThreads" | "currentAndFutureThreads" | "process",
+//     "params": {
----------------
I would be happy to just call "currentAndFutureThreads" to be the same thing as "process" here. No one cares how it is done, and this ends up tracing all threads anyway right? Can't we just make it happen? A user doesn't care if it uses the same trace buffer for all threads, just as long as all threads can be traced.


================
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>
----------------
maybe better named as "threadBufferSize"? Does it have to be in KB? Does it actually have to be a multiple of kernel page sizes at all? The description isn't clear that this is a per thread buffer, so a better description would be great that explains this.


================
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>
+//  }
----------------
more description on this please?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1690
+            thread_id,
+            m_intel_pt_trace_new_threads_params->params.buffer_size_in_kb,
+            m_intel_pt_trace_new_threads_params->params.perf_config)) {
----------------
max memory setting would be nice and should maybe be specified at trace start?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91679