labath added inline comments.
================
Comment at: lldb/docs/lldb-gdb-remote.txt:247
+//
+// {
+// "pluginName": <lldb trace plug-in name, e.g. intel-pt>
----------------
Having lldb-server match the lldb trace plugin name seems a bit backward to me.
I think it'd be more natural if the server reports a name for the technology it
uses and then the client picks a plugin to go with that technology.
I think it's fine if the process of "picking a plugin" is implemented by
matching the GetPluginName() string to the value returned here, so I don't
think that the code needs to change in anyway (except maybe droping the word
"plugin" from the field name?). My quibble is about how this is framed in the
documentation -- I'd say this should be the source of truth and trace plugin
should reference it instead of the other way around. (One reason for that could
be that we can change the plugin matching logic in the client any time we want,
but changing the packet format brings up compatibility concerns.)
================
Comment at: lldb/docs/lldb-gdb-remote.txt:247-248
+//
+// The return packet is a JSON array of lldb::TraceType values represented in
+// decimal.
+//
----------------
clayborg wrote:
> So my main question is: should a lldb-server be able to return multiple
> tracing types? I would vote to always respond with the best tracing that is
> available and ensure that lldb-server can only enable on kind of trace. I
> know IntelPT can be supported along with some other format that only saves
> that few branches (ABL?), but if IntelPT is available, should we respond with
> only IntelPT being available? It would be also nice to have a name for each
> returned tracing type instead of just a number? Maybe making the response a
> dictionary of tracing integer type to name would be nice?:
> ```
> { 1: "intel-pt", 2: "abl" }
> ```
> I would rather we return just a single entry here if possible. If we can,
> this might be better as a dictionary response:
> ```
> { "name": "intel-pt", "trace-type": 1 }
> ```
If we're expecting that lldb-server will be able to capture multiple kinds of
traces (on the same processor architecture), then (strongly) believe
lldb-server should be able to return/enumerate all of them. I don't think
lldb-server should be in the business of deciding what the user wants to do
(like, he may want deliberately use the "inferior" technology so he can compare
the performance/features of the two trace types, or whatever). Defaulting to
the most advanced format is of course fine, but that could be done by listing
that format first (or giving them an explicit priority, etc.).
That said, there is still a big if on that statement -- tracing technologies
are complex beasts, so I'd be surprised if a single processor supports two
technologies with feature sets that overlap well enough that they may be thrown
into the same bag. However, I don't know much about processor traces and Greg's
comment seems to imply that they do exist.
(It's also worth noting that it'd be fairly easy to extend this packed to
(optionally) return a list of these dictionaries if that becomes needed.)
================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:398
+ virtual llvm::Expected<TraceTypeInfo> GetSupportedTraceType() {
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Tracing is not supported");
----------------
We now have an UnimplementedError class you can return here.
================
Comment at:
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3462
+ StreamGDBRemote escaped_packet;
+ escaped_packet.PutCString("jTraceSupportedType");
+
----------------
wallace wrote:
> 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
>
>
Given the state the old intel plugin, I wouldn't be surprised if it was
completely unused. As such I don't have a problem with new packets (if the old
ones don't fit). However, I also don't want to carry the excess compatibility
baggage for too long either. I'd say we should just delete all the old code as
soon your implementation reaches rough feature parity (which shouldn't take
long, as the old code didn't do much iirc).
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
