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

Tue Nov 3 02:03:22 PST 2020

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