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

Tue Nov 17 21:31:24 PST 2020

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

So I think I get the gist of what you were trying to go for with this patch. I stopped inline comments after I got the idea.

A few things:

We need to be able to enable tracing on all threads for a process and have lldb-server just "do the right thing" and enable tracing for all threads as they get created. If we have to stop the process each time a thread is created just to give LLDB the chance to enable tracing for that thread, this will be too slow and cumbersome. The ptrace implementation for unix already has to attach to each thread as it comes along by running some code in the NativeProcessProtocol subclass, so we should be able to start tracing any and all threads as needed.

NativeProcessProtocol should have generic plug-in like functions for starting and stopping tracing that each subclass will override. See inlined comments.

See inlined comments about the jLLDB packets and let me know what you think.

================
Comment at: lldb/docs/lldb-gdb-remote.txt:276
+//  {
+//    "type": <tracing technology name, e.g. intel-pt, arm-coresight>
+//    "tid": <thread id trace in decimal>
----------------
Do we need "type" here? Only one kind of trace is currently returned by jLLDBTraceSupportedType right now. I am ok with passing it down again in case we change to returning a list from jLLDBTraceSupportedType.

================
Comment at: lldb/docs/lldb-gdb-remote.txt:277
+//    "type": <tracing technology name, e.g. intel-pt, arm-coresight>
+//    "tid": <thread id trace in decimal>
+//    "params": {
----------------
Do we want this as optional? What if we want to trace all threads? I would be a real pain if we always have to detect new threads showing up and then sending a jLLDBTraceStart as this will really slow down the process for no good reason. So it would be nice if we can specify this as optional and if the "tid" is not specified, have the lldb-server start tracing any new threads as they come along. 

================
Comment at: lldb/docs/lldb-gdb-remote.txt:307
+//  {
+//    "type": <tracing technology name, e.g. intel-pt, arm-coresight>
+//    "tid": <thread id trace in decimal>
----------------
Do we need "type" here just like in jLLDBTraceStart?

================
Comment at: lldb/docs/lldb-gdb-remote.txt:308
+//    "type": <tracing technology name, e.g. intel-pt, arm-coresight>
+//    "tid": <thread id trace in decimal>
+//  }
----------------
Is this optional? Want if we want to stop tracing all threads?

================
Comment at: lldb/docs/lldb-gdb-remote.txt:323-332
+// SCHEMA
+//  The schema for the input is
+//
+//  {
+//    "type": <tracing technology name, e.g. intel-pt, arm-coresight>
+//    "query": <identifier for the data>
+//    "params": {
----------------
Would it be better for this command to just return all the data we need? Maybe a single packet like:

jLLDBTraceGetInfo

And it would return JSON that includes all available data like:
```
{
  "vendor": "intel" | "unknown",
  "family": <decimal>,
  "model": <decimal>,
  "stepping": <decimal>,
  "tids": [
    {
      "tid": <decimal>,
      "dataSize": <decimal>, // Size of all trace data for this thread in case it is huge, so we can download parts of it?
    }
  ]
 }
```

Then we have another packet to fetch the data for each thread with a packet like "jLLDBTraceGetData" whose schema for input would be "tid" and "offset" and "size". This would allow us to fetch partial data in case we need to interrupt the fetching since this data can be bigger. The return value for this function would be just like the "binary memory read" where the bytes are much more efficiently encoded as binary and not as HEX8 format.

================
Comment at: lldb/include/lldb/Core/PluginManager.h:336-337
+      ConstString name, const char *description,
+      TraceCreateInstanceForSessionFile create_callback_for_session_file,
+      TraceCreateInstanceForLiveProcess create_callback_for_live_process,
+      llvm::StringRef schema, TraceGetStartCommand get_start_command);
----------------
Why do we need these callbacks? Can we just keep the "TraceCreateInstance create_callback" and then add virtual functions to the API in Trace.h?

================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:320
+  ///     \a llvm::Error otherwise.
+  virtual llvm::Error StopIntelPTTrace(lldb::tid_t tid) {
+    return llvm::make_error<UnimplementedError>();
----------------
Shouldn't this be generic so it can work with any NativeProcessProtocol implementation? Think about doing this same stuff on ARM. We don't want to have functions in NativeProcessProtocol like "StopIntelPTrace" and "StopARMTrace" and "StopARM64Trace" that are all stubbed out. It would be better to have something generic that passes along structured data to each function where the type of the trace is specified in the structured data.

================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:341
+  ///     \a llvm::Error otherwise.
+  virtual llvm::Error StartIntelPTTrace(lldb::tid_t tid,
+                                        size_t buffer_size_in_kb,
----------------
Ditto above comment about making this generic.

================
Comment at: lldb/include/lldb/Target/Process.h:2566
+  ///     \a llvm::Error otherwise.
+  virtual llvm::Error StartTracingThread(const llvm::json::Value &options) {
+    return llvm::make_error<UnimplementedError>();
----------------
Should this be named "StartTracing" in case the options don't specify a thread ID?

================
Comment at: lldb/include/lldb/Target/Process.h:2582
+  ///     \a llvm::Error otherwise.
+  virtual llvm::Error StopTracingThread(lldb::tid_t tid,
+                                        llvm::StringRef trace_type) {
----------------
Seems like this should be named "StopTracing" and also take "const llvm::json::Value &options" kist ;oile the StartTracing call above so we can disable a single thread or all of them?

================
Comment at: lldb/include/lldb/Target/Trace.h:232
+  template <typename TParams>
+  llvm::Error StartTracingThread(Thread &thread, const TParams &params);
+
----------------
Shouldn't TParams just be JSON?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91679