[Lldb-commits] [PATCH] D90490: [intel-pt][trace] Implement a "get supported trace type" packet

[walter erquinigo via Phabricator via lldb-commits]

wallace marked an inline comment as done.
wallace added inline comments.


================
Comment at: lldb/docs/lldb-gdb-remote.txt:249
+//    "pluginName": <lldb trace plug-in name, e.g. intel-pt>
+//    "description": <optional description string for this technology>
+//  }
----------------
clayborg wrote:
> Can't IntelPT exist on a machine but not be enabled? If so I would suggest adding a few more key values:
> ```
> "enabled": <boolean>,
> "enableInstructions": <string>
> ```
> "enabled" would say if this tracing mechanism is currently installed and if it is enabled or not.
> "enableInstructions" could clarify what you would have to do to enable this tracing feature, like run a "sudo" command, or enable a kernel module and reboot, etc.
> 
> 
I was running some tests and checking the documentation on the kernel, and Intel PT is either available or not (from the OS perspective). There's no configuration to do, which leaves this case simple. 

However, there are other situations like what you describe. If you want to use Intel PT on a VM, then you need to enable the corresponding capabilities on your VM software. This case would benefit from the fields you propose. Right now I wouldn't attempt to solve that, so I prefer not to add new fields. However, in the future, when that's needed, we can add these fields to the packet as it's a flexible json object.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3462
+  StreamGDBRemote escaped_packet;
+  escaped_packet.PutCString("jTraceSupportedType");
+
----------------
clayborg wrote:
> So are we going to reuse all of the other "jTrace*" packets and possibly expand their usage? If so, then this name is good.  If we are going to make new packets for tracing then "jLLDBTraceSupportedType" might make more sense and all commands we would add would start with "jLLDBTrace".
I'm glad you mention this. I'm thinking about using a completely new set of packets and document the existing ones as deprecated.

The old packets include:

- jTraceMetaRead -> it's confusing and not really useful for the current implementation. Even the external intel-pt plugin doesn't make use of it. It exposes the data gotten when invoking the SYS_perf_event_open syscall, which is too perf+linux only. It can be useful for getting more information out of the kernel, like some clock tracing, but nothing useful as of now.
-  jTraceConfigRead -> instead of having it, jTraceStart could return that config once tracing has been enabled. That would avoid one round trip, as decoding can't happen anyway without this information.
-  jTraceStop -> it expects a trace ID (which is intel-pt only in the current implementation), and an optional thread id. It could be simpler by just asking a list of thread ids, or none and then stop the entire process.
-  jTraceStart -> it's expecting fields like metabuffersize, which don't make sense if we want to generalize this. It also returns a trace ID (which is intel-pt only). It could instead receive a trace-plugin name, a list of thread ids, a process id, and then a set of json params. Also, as mentioned above, it could return the jTraceConfig, making the api simpler.


Overall, I'd prefer to implement new packets in a very generic way. So I'll take your jLLDB prefix.

Btw, We could implement a generic data query packet

- jLLDBTraceQueryData -> which would receive a generic data query packet that would be handled by each trace type. It could return trace buffers, or the metadata from the jTraceMetaRead packet, or any plugin-specific data.

That would leave us with jLLDBTraceStart, jLLDBTraceStop, jLLDBTraceSupportedType and jLLDBTraceQueryData




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90490