clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed.
================ Comment at: lldb/docs/lldb-gdb-remote.txt:246-251 +// { +// "name": <string>, +// Tracing technology name, e.g. intel-pt, arm-coresight. +// "description": <string>, +// Description for this technology. +// } ---------------- Do we want to return a list of types here in case we have more than one option? ================ Comment at: lldb/docs/lldb-gdb-remote.txt:278-279 +// THREAD TRACING +// This traces specific threads. This is a best effort request, which tries to +// trace as many threads as possible. +// ---------------- Is this comment out of date now? If we are doing thread tracing they should be just specific threads not right? Not "This is a best effort request, which tries to trace as many threads as possible". ================ Comment at: lldb/docs/lldb-gdb-remote.txt:311 +// Trace buffer size per thread in bytes. +// It's rounded down to the closest non-zero multiple of 4KB (a page), +// with a minimum of 4KB. ---------------- Should we be rounding down or up? If someone specifies "1" for "threadBufferSize" it would round down to zero. Or maybe we specific this should be a multiple of 4K or we will fail? ================ Comment at: lldb/docs/lldb-gdb-remote.txt:426-433 +// { +// "cpuInfo": { +// "vendor": "intel" | "unknown", +// "family": <decimal integer>, +// "model": <decimal integer>, +// "stepping": <decimal integer>, +// } ---------------- Should this "cpuInfo" be a process data type and sent as binary that we fetch with jLLDBTraceGetBinaryData? The JSON would look something like: ``` { "tracedThreads": [ { "tid": 123, "data": [ { "kind": "thread-data", "size": 1024 } ] }, { "tid": 234, "data": [ { "kind": "thread-data", "size": 1024 } ] } ], "process-data": [ { "pid": 345, "data", [ { "kind": "cpuInfo", "size": 16 } ] } ] } ``` ================ Comment at: lldb/include/lldb/lldb-enumerations.h:251 + eStopReasonInstrumentation, + eStopReasonTracingError, }; ---------------- We should have a processor trace specific stop reason so we should rename this to something like "eStopReasonProcessorTrace". We will use this for many things: - max memory for process is being exceeded (what this is being used - trace buffers are full and need to be emptied (for future use with fixed buffers that stop the process when they are full) - anything else trace related Then you can encode things into the the StopInfo class. See StopInfo::CreateStopReasonWithSignal(...) and other functions that encode additional data for stop reasons. ================ Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:649 + case eStopReasonTracingError: + return "tracing"; case eStopReasonInstrumentation: ---------------- "process trace"? ================ Comment at: lldb/source/Target/Target.cpp:3060-3061 + if (!m_trace_sp) { + llvm::Expected<TraceSupportedResponse> trace_type = + m_process_sp->TraceSupported(); + if (!trace_type) ---------------- This will crash if there is no process, so maybe above if statement should be: ``` if (!m_trace_sp && m_process_sp) ``` ================ Comment at: lldb/source/Target/Thread.cpp:1689 + case eStopReasonTracingError: + return "tracing event"; } ---------------- "processor trace"? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91679/new/ https://reviews.llvm.org/D91679 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits