jj10306 requested changes to this revision. jj10306 added a comment. This revision now requires changes to proceed. Herald added a subscriber: Michael137.
Looks great overall, thanks for making these improvements - just a couple minor things ================ Comment at: lldb/include/lldb/Target/Trace.h:524 + /// process data refreshers. + struct Storage { + /// Portmortem processes traced by this object if doing non-live tracing. ---------------- nice ================ Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:188 + + bool was_enabled = m_enabled; if (Error err = DisableWithIoctl()) ---------------- why this intermediate `was_enabled` variable? ================ Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:248 + + bool was_enabled = m_enabled; if (Error err = DisableWithIoctl()) ---------------- same question as above ================ Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:347-349 + attr.exclude_kernel = 1; + attr.sample_id_all = 1; + attr.exclude_hv = 1; ---------------- it may not make a difference since this is a context switch event, but should these values be set based on the parent's perf event? ================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:319 + +TraceIntelPT::Storage &TraceIntelPT::GetUpdatedStorage() { RefreshLiveProcessState(); ---------------- this is much cleaner now (: ================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:205 + /// These decoders are used for the non-per-core case + std::map<lldb::tid_t, std::unique_ptr<ThreadDecoder>> thread_decoders; + /// Helper variable used to track long running operations for telemetry. ---------------- use llvm::DenseMap instead? ================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h:33-38 + uint64_t tid; llvm::Optional<std::string> trace_buffer; }; struct JSONProcess { + uint64_t pid; ---------------- pid is really a int32_t in the kernel. should these be int32_t or pid_t/tid_t instead? ================ Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.h:65 - TraceIntelPT &m_trace; - std::set<lldb::core_id_t> m_cores; + TraceIntelPT *m_trace; std::set<lldb::tid_t> m_tids; ---------------- why are we storing a pointer now if the constructor is still passing in a ref? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127456/new/ https://reviews.llvm.org/D127456 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits