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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/docs/lldb-gdb-remote.txt:245
+//
+//  See lldb-enumerations for the list of available TraceType's.
+//
----------------
DavidSpickett wrote:
> Saying `lldb-enumerations.h` might save someone some grepping time, also saying that it should be in decimal.
> 
> Can you clarify what TraceType means? Is it:
> The way the trace is generated/stored, like on chip vs off chip
> The thing you're tracging, e.g. instructions or power usage
> Specific trace versions e.g intel pt, arm coresight (pretty sure it's not this one)
> 
> If there's the possibility of one system having multiple types then the packet should at least have the format to hold them, even though we only read the first one.
> 
> Which your code already does since:
> 1,2;
> Would stop parsing at the ',' anyway, but worth noting if it's relevant.
The `j` in these trace packets was supposed to refer to the fact they are json-formatted. Using json for this is overkill, but that means I'm now torn between wanting to preserve the `j` consistency and the `jTrace` consistency. If this were returning a list of types, using json for that might not be totally unreasonable, and would let us avoid this dilemma :P.


================
Comment at: lldb/docs/lldb-gdb-remote.txt:254
+send packet: jTraceSupportedType
+read packet: <trace type>/E<error code>;AAAAAAAAA
+
----------------
DavidSpickett wrote:
> What is the AAAAA for?
That's the extended (textual) error message -- lldb and gdb have independently invented incompatible formats for that (I share a blame it that :/).

This style of presentation is consistent with the other jTrace packets, though I am not sure it's that helpful really -- we support the extended error on any response packet and we don't document that always. (Also the error message will only be sent if the client actually signals support for it.)

(In fact, when I look at the implementation, I see that it will _never_ send the textual error message.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90490