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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
