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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Just some layering remarks. Other than that, I think this is fine.



================
Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:17
 #include "lldb/Host/MainLoop.h"
+#include "lldb/Target/Trace.h"
 #include "lldb/Utility/ArchSpec.h"
----------------
You shouldn't include `lldb/Target` from here. We already have Utility/TraceOptions.h. Maybe TraceTypeInfo could go there instead?


================
Comment at: lldb/include/lldb/Target/Process.h:2547-2548
 
+  /// See the information on the jLLDBTraceSupportedType packet in
+  /// lldb-gdb-remote.txt.
+  ///
----------------
This code shouldn't refer the the gdb packets. Theoretically we could have other process plugins supporting this.


================
Comment at: lldb/include/lldb/Target/Trace.h:184-185
 
+/// This struct represents a return value from the jLLDBTraceSupportedType
+/// packet in lldb-gdb-remote.txt
+struct TraceTypeInfo {
----------------
Neither should this.


================
Comment at: lldb/source/Plugins/Process/Linux/ProcessorTrace.cpp:60
+  StringRef buffer = intel_pt_type_text.get()->getBuffer();
+  if (buffer.empty() || buffer.trim().getAsInteger(10, intel_pt_type))
+    return createStringError(
----------------
the empty check is superfluous.


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

https://reviews.llvm.org/D90490