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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

So I know Intel had gone and done some tracing support in an external kind of way before. This patch is preparing to use this functionality by asking for the supported trace types. I am not a fan of a global enumeration in lldb-enumerations.h controlling the tracing type and having to have GDB servers have to respond using these LLDB specific enumeration values. It would be nice if we can query the GDB server for the supported trace types and have them respond with a string for the tracing type. See my inlined comments for what was thinking. I also think that a GDB server should respond with a single kind of supported tracing, see inlined comments for more details.



================
Comment at: lldb/docs/lldb-gdb-remote.txt:245
+//
+//  See lldb::TraceType in lldb-enumerations.h for more information.
+//
----------------
I'm not a fan of using the enumeration in lldb-enumerations.h here if we can avoid it. See my comment below about giving a name to the tracing types.


================
Comment at: lldb/docs/lldb-gdb-remote.txt:247-248
+//
+//  The return packet is a JSON array of lldb::TraceType values represented in
+//  decimal.
+//
----------------
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 }
```


================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:395-399
+  /// \copydoc Process::GetSupportedTraceType()
+  virtual lldb::TraceType GetSupportedTraceType() {
+    return lldb::eTraceTypeNone;
+  }
+
----------------
So here we are returning only 1 tracing type? That is fine if we switch the reply to the jTraceSupportedTypes to be a single value, otherwise this would need to be an array. Not a fan of having to use enumerations from lldb-enumerations.h, so I would rather return a ConstString with the plug-in name if possible.


================
Comment at: lldb/include/lldb/Target/Process.h:2553
+  ///     gdb-server, an \a llvm::Error object is returned.
+  virtual llvm::Expected<lldb::TraceType> GetSupportedTraceType() {
+    return lldb::eTraceTypeNone;
----------------
Again only one is fine if we switch the jTraceSupportedTypes to return one value.


================
Comment at: lldb/include/lldb/lldb-enumerations.h:781
 
-  // Hardware Trace generated by the processor.
-  eTraceTypeProcessorTrace
+  eTraceTypeIntelProcessorTrace
 };
----------------
enumerations in this header file cannot change as they are part of the API for LLDB, so we can't rename this. You can add a comment letting people know about this, but you can't rename this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90490