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

Greg Clayton via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Mar 29 15:00:46 PDT 2021

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.

Comment at: lldb/docs/lldb-gdb-remote.txt:246-251
+//   {
+//     "name": <string>,
+//         Tracing technology name, e.g. intel-pt, arm-coresight.
+//     "description": <string>,
+//         Description for this technology.
+//   }
Do we want to return a list of types here in case we have more than one option?

Comment at: lldb/docs/lldb-gdb-remote.txt:278-279
+//  This traces specific threads. This is a best effort request, which tries to
+//  trace as many threads as possible.
Is this comment out of date now? If we are doing thread tracing they should be just specific threads not right? Not "This is a best effort request, which tries to trace as many threads as possible".

Comment at: lldb/docs/lldb-gdb-remote.txt:311
+//         Trace buffer size per thread in bytes.
+//         It's rounded down to the closest non-zero multiple of 4KB (a page),
+//         with a minimum of 4KB.
Should we be rounding down or up? If someone specifies "1" for "threadBufferSize" it would round down to zero. Or maybe we specific this should be a multiple of 4K or we will fail?

Comment at: lldb/docs/lldb-gdb-remote.txt:426-433
+//  {
+//    "cpuInfo": {
+//      "vendor": "intel" | "unknown",
+//      "family": <decimal integer>,
+//      "model": <decimal integer>,
+//      "stepping": <decimal integer>,
+//    }
Should this "cpuInfo" be a process data type and sent as binary that we fetch with jLLDBTraceGetBinaryData? The JSON would look something like:
  "tracedThreads": [
    { "tid": 123, "data": [ { "kind": "thread-data", "size": 1024 } ] },
    { "tid": 234, "data": [ { "kind": "thread-data", "size": 1024 } ] }
  "process-data": [
    { "pid": 345, "data",  [ { "kind": "cpuInfo", "size": 16 } ] }

Comment at: lldb/include/lldb/lldb-enumerations.h:251
+  eStopReasonInstrumentation,
+  eStopReasonTracingError,
We should have a processor trace specific stop reason so we should rename this to something like "eStopReasonProcessorTrace". We will use this for many things:
- max memory for process is being exceeded (what this is being used
- trace buffers are full and need to be emptied (for future use with fixed buffers that stop the process when they are full)
- anything else trace related

Then you can encode things into the the StopInfo class. See StopInfo::CreateStopReasonWithSignal(...) and other functions that encode additional data for stop reasons.

Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:649
+  case eStopReasonTracingError:
+    return "tracing";
   case eStopReasonInstrumentation:
"process trace"?

Comment at: lldb/source/Target/Target.cpp:3060-3061
+  if (!m_trace_sp) {
+    llvm::Expected<TraceSupportedResponse> trace_type =
+        m_process_sp->TraceSupported();
+    if (!trace_type)
This will crash if there is no process, so maybe above if statement should be:

if (!m_trace_sp && m_process_sp)

Comment at: lldb/source/Target/Thread.cpp:1689
+  case eStopReasonTracingError:
+    return "tracing event";
"processor trace"?

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list