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

wallace created this revision. wallace added a reviewer: clayborg. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. wallace requested review of this revision. Herald added a subscriber: JDevlieghere. Depends on D89283 . The goal of this packe

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

DavidSpickett added inline comments. Comment at: lldb/docs/lldb-gdb-remote.txt:245 +// +// See lldb-enumerations for the list of available TraceType's. +// Saying `lldb-enumerations.h` might save someone some grepping time, also saying that it should be in deci

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

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

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

wallace updated this revision to Diff 302402. wallace edited the summary of this revision. wallace added a comment. Thanks for the feedback! Some changes: - I'm no longer showing the A in the documentation, as @labath pointed out that I was not using it, and, in fact, there's no need to use

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

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 type

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

wallace updated this revision to Diff 302439. wallace added a comment. After a sync up with Greg, I've followed his recommedations and did the following changes: - Kept the packet as a simple packet returning one single trace type. However, now the response is a json object {"pluginName": , "de

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

wallace updated this revision to Diff 302442. wallace added a comment. nit Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90490/new/ https://reviews.llvm.org/D90490 Files: lldb/docs/lldb-gdb-remote.txt lldb/include/lldb/Host/common/NativeProces

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

clayborg added a comment. See inlined comments. Just some questions on wether we are going to reuse the other existing "tTrace*" packets, or if we are going to make new ones. Comment at: lldb/docs/lldb-gdb-remote.txt:249 +//"pluginName": +//"description": +// }

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

wallace marked an inline comment as done. wallace added inline comments. Comment at: lldb/docs/lldb-gdb-remote.txt:249 +//"pluginName": +//"description": +// } clayborg wrote: > Can't IntelPT exist on a machine but not be enabled? If so I would suggest

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

wallace updated this revision to Diff 302468. wallace added a comment. - Renamed the packet to jLLDBTraceSupportedType following Greg's suggestion - Also, marked the old trace packets as deprecated, to discourage their usage. I doubt anyone is using them, as in March I discovered that the externa

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

labath added inline comments. Comment at: lldb/docs/lldb-gdb-remote.txt:247 +// +// { +//"pluginName": 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 technol

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

wallace updated this revision to Diff 302630. wallace added a comment. - Use name instead of pluginName and UnimplementedError, as @labath suggested - Still keep the packet returning one single trace technology. It'll be a long time until two technologies collide, but if that ever happens, we can

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

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"

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

wallace updated this revision to Diff 303031. wallace marked 10 inline comments as done. wallace added a comment. Herald added a subscriber: mgorny. address issues Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90490/new/ https://reviews.llvm.org/D9

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

clayborg added inline comments. Comment at: lldb/docs/lldb-gdb-remote.txt:248 +// { +//"name": +//"description": Should this specify that this name needs to match the name of the LLDB plug-in? Comment at: lldb/include/lldb/Utility/St

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

wallace updated this revision to Diff 303313. wallace marked 5 inline comments as done. wallace added a comment. Address all comments, specially improved the documentation requested by @clayborg. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90490/

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

clayborg added a comment. So down to just one question about the packet definition about if we should have an error code in JSON or if an error code even makes sense. Other than that this LGTM Comment at: lldb/docs/lldb-gdb-remote.txt:262 +send packet: jLLDBTraceSupportedType

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

labath added inline comments. Comment at: lldb/docs/lldb-gdb-remote.txt:262 +send packet: jLLDBTraceSupportedType +read packet: {"name": , "description", }/E + clayborg wrote: > I know the deprecated trace packets allows an error code to be returned, but > since

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

wallace updated this revision to Diff 304232. wallace added a comment. Added a test that accounts for a custom error message from the gdb-server. I decided to follow @labath's approach of using the existing error code format (;AAA). It works well and the GDBClient classes already have good su

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

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. LGTM with the error string stuff included so we can get a nice string error message. Pavel, do you have anything? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

labath accepted this revision. labath added a comment. I didn't check all the details, but seems ok to me. Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:3465-3468 + if (SendPacketAndWaitForResponse(escaped_packet.GetString(), response, +

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG21555fff4de8: [intel-pt][trace] Implement a "get supported trace type" packet (authored by wallace). Changed prior to commit: https://reviews.llvm

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

stella.stamenova added a comment. It looks like this broke the windows lldb buildbot: http://lab.llvm.org:8011/#/builders/83/builds/693 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90490/new/ https://reviews.llvm.org/D90490 _

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

wallace added a comment. @stella.stamenova I'm working on a fix right now. 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-comm