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