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

[Greg Clayton via Phabricator via lldb-commits]

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
+// THREAD TRACING
+//  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"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91679