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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

See inlined comments. Just some questions on wether we are going to reuse the other existing "tTrace*" packets, or if we are going to make new ones.



================
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>
+//  }
----------------
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.




================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3462
+  StreamGDBRemote escaped_packet;
+  escaped_packet.PutCString("jTraceSupportedType");
+
----------------
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".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90490