[Lldb-commits] [PATCH] D120595: [trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces

jj10306 created this revision. jj10306 added reviewers: wallace, davidca. jj10306 requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Begin implementing Tsc to timestamp conversion for IntelPT traces: - Add TscConverter and stub out first impl

[Lldb-commits] [PATCH] D120595: [trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces

jj10306 added inline comments. Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:51 + int64_t m_time_shift; + int64_t m_time_zero; +}; What is the suggested way to serialize a u64 into JSON? I thought of two approaches: 1. Store two separate

[Lldb-commits] [PATCH] D120595: [WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces

jj10306 marked 21 inline comments as done. jj10306 added inline comments. Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:51 + int64_t m_time_shift; + int64_t m_time_zero; +}; wallace wrote: > jj10306 wrote: > > What is the suggested way to

[Lldb-commits] [PATCH] D120595: [WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces

jj10306 updated this revision to Diff 411862. jj10306 marked 3 inline comments as done. jj10306 edited the summary of this revision. jj10306 added a comment. Address comments, still need to handle uint64_t JSON serialization correctly. I plan to proceed with decoder and schema changes now that in

[Lldb-commits] [PATCH] D120595: [WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces

jj10306 marked 41 inline comments as done. jj10306 added a comment. Addressed comments locally, waiting to post update until the couple minor discussions with @wallace and @davidca in the comments are complete. Comment at: lldb/docs/lldb-gdb-remote.txt:469 +// +//tsc_rate:

[Lldb-commits] [PATCH] D120595: [WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces

jj10306 updated this revision to Diff 413985. jj10306 marked 2 inline comments as done. jj10306 edited the summary of this revision. jj10306 added a comment. Addressed most of the previous diff's comments (other than the ones from @wallace and @davidca that were explicitly mentioned in my previou

[Lldb-commits] [PATCH] D120595: [WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces

jj10306 marked 10 inline comments as done. jj10306 added inline comments. Comment at: lldb/include/lldb/Target/TraceCursor.h:193 + // TODO: add docs and rename once naming convention is agreed upon + virtual llvm::Optional GetNanos() = 0; + wallace wrote: > Wha

[Lldb-commits] [PATCH] D120595: [WIP][trace][intelpt] Add TSC to Nanosecond conversion for IntelPT traces

jj10306 added a comment. Due to the size and expanding scope of this diff, I'm going to split this into several smaller diffs. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120595/new/ https://reviews.llvm.org/D120595 ___ lldb-commits mailin

[Lldb-commits] [PATCH] D121711: [trace][intelpt] Rename IntelPTManager and change TraceCursor::GetTimestampCounter API to general trace counter API

jj10306 created this revision. jj10306 added reviewers: wallace, davidca. Herald added a subscriber: mgorny. Herald added a project: All. jj10306 requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Minor refactor and renaming: - Rename `IntelP

[Lldb-commits] [PATCH] D128484: [NFC][lldb][trace] Rename trace session to trace bundle

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. lgtm - thanks for doing this renaming 🙂 Comment at: lldb/include/lldb/Core/PluginManager.h:346 - static TraceCreateInstanceForSessionFile + static TraceCreateInstanceFo

[Lldb-commits] [PATCH] D128477: [trace] Add a flag to the decoder to output the instruction type

jj10306 requested changes to this revision. jj10306 added a comment. Thanks for working on this 🙂 Looks good overall, just left some minor comments. Comment at: lldb/include/lldb/Target/TraceCursor.h:64 /// Low level traversal: -/// Unlike the \a TraceCursor::Next() API, whi

[Lldb-commits] [PATCH] D128543: [trace] Improve the TraceCursor iteration API

jj10306 requested changes to this revision. jj10306 added a comment. This revision now requires changes to proceed. will take a complete look over the weekend, but wanted to point out the conflict with @persona0220's diff asap Comment at: lldb/include/lldb/Target/TraceCursor.h

[Lldb-commits] [PATCH] D128543: [trace] Improve the TraceCursor iteration API

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. lgtm, thanks for making the cursor traversal much cleaner Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:228-231 - // We insert a fake error signaling an

[Lldb-commits] [PATCH] D128576: [trace] Make events first class items in the trace cursor and rework errors

jj10306 requested changes to this revision. jj10306 added a comment. This revision now requires changes to proceed. review- part 1 Comment at: lldb/include/lldb/Target/Trace.h:171 + /// information failed to load, an \a llvm::Error is returned. + virtual llvm::Expected +

[Lldb-commits] [PATCH] D128576: [trace] Make events first class items in the trace cursor and rework errors

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. This is awesome work - the code is much more understandable, so thanks for doing this! lgtm overall, just left a couple final minor comments. I think we can continue to iterate/improve this

[Lldb-commits] [PATCH] D129340: [trace][intel pt] Create a CPU change event and expose it in the dumper

jj10306 requested changes to this revision. jj10306 added a comment. This revision now requires changes to proceed. looks good overall, just a couple minor suggestions and questions Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:288 +decoded_thread.Notif

[Lldb-commits] [PATCH] D129257: [trace][intel pt] Add a cgroup filter

jj10306 requested changes to this revision. jj10306 added a comment. This revision now requires changes to proceed. looks good overall, just a couple questions from my end Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:87-90 +std::string slice = line.sub

[Lldb-commits] [PATCH] D129239: [trace] Add an option to save a compact trace bundle

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. lgtm, two minor comments Comment at: lldb/source/Plugins/Trace/intel-pt/PerfContextSwitchDecoder.cpp:318 +should_copy = true; +} else if (perf_record.IsErrorRec

[Lldb-commits] [PATCH] D129613: [trace][intel pt] Add a nice parser for the trace size

jj10306 accepted this revision. jj10306 added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Plugins/Trace/intel-pt/CommandObjectTraceStartIntelPT.cpp:174 + + std::map multipliers = {{"mib", 1024 * 1024}, +

[Lldb-commits] [PATCH] D129249: [trace][intel pt] Measure the time it takes to decode a thread in per-cpu mode

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129249/new/ https://reviews.llvm.org/D129249 _

[Lldb-commits] [PATCH] D129340: [trace][intel pt] Create a CPU change event and expose it in the dumper

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. thanks for following up on those questions, lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D129340/new/ https://reviews.llvm.org/D129340 _

[Lldb-commits] [PATCH] D129257: [trace][intel pt] Add a cgroup filter

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. thanks for answering those questions, lgtm Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:87-90 +std::string slice = line.substr(line.find_first_of(

[Lldb-commits] [PATCH] D130054: [trace][intel pt] Introduce wall clock time for each trace item

jj10306 requested changes to this revision. jj10306 added inline comments. This revision now requires changes to proceed. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:183 -DecodedThread::TscRange::TscRange(std::map::const_iterator it, +DecodedThread::TscRan

[Lldb-commits] [PATCH] D130054: [trace][intel pt] Introduce wall clock time for each trace item

jj10306 added a comment. AFAICT this doesn't handle the TSCs we get for CPU change events through the context switch traces, is that correct? This will be very useful to have for visualization purposes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[Lldb-commits] [PATCH] D130054: [trace][intel pt] Introduce wall clock time for each trace item

jj10306 requested changes to this revision. jj10306 added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:288 +decoded_thread.NotifyTsc(execution.thread_execution.GetLowestKnownTSC()); decoded_thread.NotifyCPU(execution.thread_executio

[Lldb-commits] [PATCH] D130580: Add string conversion for InstructionControlFlowKind enum

jj10306 created this revision. jj10306 added reviewers: persona0220, wallace. Herald added a project: All. jj10306 requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Refactor the string conversion of the `lldb::InstructionControlFlowKind` enum

[Lldb-commits] [PATCH] D130580: Add string conversion for InstructionControlFlowKind enum

jj10306 updated this revision to Diff 447727. jj10306 added a comment. remove accidentily included change to trace dumper Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130580/new/ https://reviews.llvm.org/D130580 Files: lldb/include/lldb/Core/Di

[Lldb-commits] [PATCH] D130580: Add string conversion for InstructionControlFlowKind enum

jj10306 added inline comments. Comment at: lldb/source/Target/TraceDumper.cpp:221 &item.symbol_info->exe_ctx))); + +m_j.attribute( ignore this accidentally included change, will delete this and update the diff Repository: rG

[Lldb-commits] [PATCH] D130580: Refactor string conversion for InstructionControlFlowKind enum

jj10306 added inline comments. Comment at: lldb/source/Core/Disassembler.cpp:930 + } +} + Thoughts on adding a default case with a string that indicates that no string conversion was implemented for the provided variant? This would potentially make it easier fo

[Lldb-commits] [PATCH] D130580: Refactor string conversion for InstructionControlFlowKind enum

jj10306 updated this revision to Diff 447802. jj10306 added a comment. Rebase, fix merge conflicts with D130320 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130580/new/ https://reviews.llvm.org/D130580 Files:

[Lldb-commits] [PATCH] D130580: Refactor string conversion for InstructionControlFlowKind enum

jj10306 updated this revision to Diff 447804. jj10306 marked 3 inline comments as done. jj10306 added a comment. Remove unnecessary comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130580/new/ https://reviews.llvm.org/D130580 Files: lldb/in

[Lldb-commits] [PATCH] D130580: Refactor string conversion for InstructionControlFlowKind enum

This revision was automatically updated to reflect the committed changes. Closed by commit rGd36ae4952d70: Add string conversion for InstructionControlFlowKind enum (authored by jj10306). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130580/new/ htt

[Lldb-commits] [PATCH] D130607: [trace] Add instruction control flow kind to JSON trace dumper's output

jj10306 created this revision. jj10306 added reviewers: wallace, persona0220. Herald added a project: All. jj10306 requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. D128477 adds a '-k' flag which displays ea

[Lldb-commits] [PATCH] D130607: [trace] Add instruction control flow kind to JSON trace dumper's output

This revision was automatically updated to reflect the committed changes. Closed by commit rGdde3cf2e83d2: [trace] Add instruction control flow kind to JSON trace dumper's output (authored by jj10306). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D13

[Lldb-commits] [PATCH] D130924: [trace][intelpt] Update TraceIntelPTBundleSaver.cpp to accommodate FileSpec API changes

jj10306 created this revision. jj10306 added reviewers: clayborg, wallace, persona0220. Herald added a project: All. jj10306 requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. D130309 introduced changes to th

[Lldb-commits] [PATCH] D130925: [trace] Replace TraceCursorUP with TraceCursorSP

jj10306 created this revision. jj10306 added reviewers: wallace, persona0220. Herald added a project: All. jj10306 requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. The use of `std::unique_ptr` with `TraceCursor` adds unnecessary complexity t

[Lldb-commits] [PATCH] D130924: [NFC][trace] Update TraceIntelPTBundleSaver.cpp to accommodate FileSpec API changes

This revision was automatically updated to reflect the committed changes. Closed by commit rG9bab358e3922: [trace][intelpt] Update TraceIntelPTBundleSaver.cpp to accommodate FileSpec API… (authored by jj10306). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

jj10306 created this revision. jj10306 added reviewers: wallace, persona0220. Herald added a subscriber: mgorny. Herald added a project: All. jj10306 requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Add bindings for the `TraceCursor` to allo

[Lldb-commits] [PATCH] D130925: [trace] Replace TraceCursorUP with TraceCursorSP

This revision was automatically updated to reflect the committed changes. Closed by commit rG3bec33b16db1: [trace] Replace TraceCursorUP with TraceCursorSP (authored by jj10306). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130925/new/ https://revi

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

jj10306 marked 13 inline comments as done. jj10306 added inline comments. Comment at: lldb/include/lldb/API/SBTraceCursor.h:166 + /// The specific kind of event the cursor is pointing at, or \b + /// TraceEvent::eTraceEventNone if the cursor not pointing to an event. +

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

jj10306 updated this revision to Diff 449289. jj10306 marked 4 inline comments as done. jj10306 added a comment. 1. address comments 2. rebase 3. minor changes to `DecodedThread::GetInstructionLoadAddress` and `DecodedThread::GetErrorByIndex` Repository: rG LLVM Github Monorepo CHANGES SINCE

[Lldb-commits] [PATCH] D130805: [trace][intel pt] Support a new kernel section in LLDB’s trace bundle schema

jj10306 requested changes to this revision. jj10306 added a comment. Nice work! qq: Do we plan to add this kernel tracing support for live tracing as well? Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:83 JSONTraceBundleDescription &bundle_description, Ar

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

jj10306 added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:201 lldb::TraceEvent DecodedThread::GetEventByIndex(int item_index) const { + // This currently returns an undefined value when the item isn't an event. return m_item_data[item_in

[Lldb-commits] [PATCH] D131005: [LLDB] Add SBInstruction::GetControlFlowKind()

jj10306 created this revision. jj10306 added reviewers: wallace, persona0220. Herald added a project: All. jj10306 requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. D128477 adds the control flow kind for `I

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

jj10306 updated this revision to Diff 449461. jj10306 marked 4 inline comments as done. jj10306 added a comment. Revert changes to DecodedThread trace item getter API's Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130930/new/ https://reviews.llvm.

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

jj10306 updated this revision to Diff 449462. jj10306 added a comment. nit Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130930/new/ https://reviews.llvm.org/D130930 Files: lldb/bindings/interface/SBTrace.i lldb/bindings/interface/SBTraceCurso

[Lldb-commits] [PATCH] D131005: [LLDB] Add SBInstruction::GetControlFlowKind()

This revision was automatically updated to reflect the committed changes. jj10306 marked an inline comment as done. Closed by commit rG6cbc6e9a6d5f: [LLDB] Add SBInstruction::GetControlFlowKind() (authored by jj10306). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[Lldb-commits] [PATCH] D130930: [trace] Add SBTraceCursor bindings

This revision was automatically updated to reflect the committed changes. Closed by commit rGf9b4ea0ce9ef: [trace] Add SBTraceCursor bindings (authored by jj10306). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130930/new/ https://reviews.llvm.org/D

[Lldb-commits] [PATCH] D131630: [trace][intel pt] Fix per-psb packet decoding

jj10306 requested changes to this revision. jj10306 added inline comments. This revision now requires changes to proceed. Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:164-166 + /// \param[in] decoder + /// A decoder configured to start and end within the

[Lldb-commits] [PATCH] D131630: [trace][intel pt] Fix per-psb packet decoding

jj10306 accepted this revision. jj10306 added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:440 +Expected> lldb_private::trace_intel_pt::SplitTraceInContinuousExecutions( +TraceIntelPT &trac

[Lldb-commits] [PATCH] D121711: [trace][intelpt] Rename IntelPTManager and change TraceCursor::GetTimestampCounter API to general trace counter API

jj10306 updated this revision to Diff 415918. jj10306 added a comment. Fix minor comment issues CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121711/new/ https://reviews.llvm.org/D121711 Files: lldb/include/lldb/Target/TraceCursor.h lldb/include/lldb/lldb-enumerations.h lldb/sour

[Lldb-commits] [PATCH] D121711: [trace][intelpt] Rename IntelPTManager and change TraceCursor::GetTimestampCounter API to general trace counter API

jj10306 updated this revision to Diff 415960. jj10306 added a comment. rebase CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121711/new/ https://reviews.llvm.org/D121711 Files: lldb/include/lldb/Target/TraceCursor.h lldb/include/lldb/lldb-enumerations.h lldb/source/Plugins/Process

[Lldb-commits] [PATCH] D121711: [trace][intelpt] Rename IntelPTManager and change TraceCursor::GetTimestampCounter API to general trace counter API

This revision was automatically updated to reflect the committed changes. Closed by commit rG22077627ae20: Minor refactor and renaming: (authored by jj10306). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121711/new/ https://reviews.llvm.org/D121711

[Lldb-commits] [PATCH] D122023: [trace][intelpt] fix some test failures

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122023/new/ https://reviews.llvm.org/D122023 _

[Lldb-commits] [PATCH] D121734: [trace][intelpt] Add thin wrapper for perf_event_open API

This revision was automatically updated to reflect the committed changes. Closed by commit rGe6c84f82b875: Add thin wrapper for perf_event_open API (authored by jj10306). Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[Lldb-commits] [PATCH] D122176: [trace] clear any existing tracing sessions before relaunching the binary

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. nice catch, lgtm! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122176/new/ https://reviews.llvm.org/D122176

[Lldb-commits] [PATCH] D122254: [trace][intelpt] Introduce instruction Ids

jj10306 requested changes to this revision. jj10306 added a comment. This revision now requires changes to proceed. Couple minor things Comment at: lldb/include/lldb/Target/TraceInstructionDumper.h:66 private: + /// Indicate the dumper that no more data is available in the tr

[Lldb-commits] [PATCH] D122254: [trace][intelpt] Introduce instruction Ids

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. nice work! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122254/new/ https://reviews.llvm.org/D122254 ___

[Lldb-commits] [PATCH] D122293: [wip][intelpt] Refactoring instruction decoding for flexibility

jj10306 requested changes to this revision. jj10306 added inline comments. This revision now requires changes to proceed. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:172 std::vector m_instructions; + std::vector m_errors; + wallace wrote: >

[Lldb-commits] [PATCH] D122293: [wip][intelpt] Refactoring instruction decoding for flexibility

jj10306 added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:152 + /// The errors of the trace. + std::unordered_map GetErrors() const; + Return a reference here to avoid potential expensive copy when returning. Something els

[Lldb-commits] [PATCH] D122246: [trace][intelpt] Server side changes for TSC to wall time conversion

This revision was automatically updated to reflect the committed changes. Closed by commit rG9b79187c96a3: [trace][intelpt] Server side changes for TSC to wall time conversion (authored by jj10306). Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Repository: rG LLVM Githu

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

jj10306 added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:41 m_pt_insn.iclass = ptic_error; + m_is_error = true; } Is this boolean necessary? In the case of an error, the other two fields also indicate an error so this

[Lldb-commits] [PATCH] D122293: [intelpt] Refactoring instruction decoding for flexibility

jj10306 added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp:77 bool TraceCursorIntelPT::IsError() { return m_decoded_thread_sp->GetInstructions()[m_pos].IsError(); } zrthxn wrote: > jj10306 wrote: > > nit: should we u

[Lldb-commits] [PATCH] D122603: [intelpt] Refactor timestamps out of IntelPTInstruction

jj10306 added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:145 + private: +friend class DecodedThread; + nit: No need to friend the enclosing class since C++11 - https://en.cppreference.com/w/cpp/language/nested_types Re

[Lldb-commits] [PATCH] D122867: [trace][intel pt] Handle better tsc in the decoder

jj10306 added a comment. The changes to the decoder look sound, just left two questions related to it and a couple other nits. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp:120 + +void DecodedThread::ReportTscError(int libipt_error_code) { m_tsc_errors++; }

[Lldb-commits] [PATCH] D122867: [trace][intel pt] Handle better tsc in the decoder

jj10306 added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:236-238 + /// Notify this class that the last added instruction or error has + /// an associated TSC. + void ReportTscForLastInstruction(uint64_t tsc); nit: When I in

[Lldb-commits] [PATCH] D122867: [trace][intel pt] Handle better tsc in the decoder

jj10306 accepted this revision. jj10306 added a comment. Looks good Comment at: lldb/source/Plugins/Trace/intel-pt/IntelPTDecoder.cpp:248 - DecodeInstructions(*decoder, decoded_thread_sp); + DecodeInstructions(*decoder, *decoded_thread_sp.get()); pt_insn_free_decoder(dec

[Lldb-commits] [PATCH] D123106: [trace][intel pt] Create a class for the libipt decoder wrapper

jj10306 added a comment. Overall looks good, just a couple cosmetic things and comments about the plan for multi-buffer, single thread decoding Comment at: lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp:41 + +/// Class that decodes a raw buffer for a single thread using

[Lldb-commits] [PATCH] D123281: [trace][intel pt] Create a common accessor for live and postmortem data

jj10306 added inline comments. Comment at: lldb/include/lldb/Target/Trace.h:265 + lldb::tid_t tid, llvm::StringRef kind, + std::function data)> callback); + typedef the callback to be cleaner and make the intention more clear? Comment

[Lldb-commits] [PATCH] D123281: [trace][intel pt] Create a common accessor for live and postmortem data

jj10306 accepted this revision. jj10306 added inline comments. This revision is now accepted and ready to land. Comment at: lldb/include/lldb/Target/Trace.h:386-396 + llvm::DenseMap> m_live_thread_data; + /// data kind -> size std::unordered_map m_live_process_data;

[Lldb-commits] [PATCH] D123358: [trace][intelpt] Remove code smell when printing the raw trace size

jj10306 added a comment. Looks good. Any reason not to have `GetRawTraceSize()` at the `Trace` interface level instead of being specific to `TraceIntelPT`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123358/new/ https://reviews.llvm.org/D123358

[Lldb-commits] [PATCH] D123357: [trace][intelpt] Add task timer classes

jj10306 added inline comments. Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:243 + /// Total amount of time spent decoding. + std::chrono::milliseconds m_total_decoding_time{0}; }; nit: Do you need an explicit initialization? ===

[Lldb-commits] [PATCH] D123982: [trace][intel pt] Support events

jj10306 added inline comments. Comment at: lldb/include/lldb/lldb-enumerations.h:1151 +// Events that might happen during a trace session. +FLAGS_ENUM(TraceEvents){ +// Tracing was paused. If instructions were executed after pausing What are some other events

[Lldb-commits] [PATCH] D123984: [trace][intel pt] Add a memory usage test

jj10306 added a comment. What is the intention of this test? To catch changes that may increase the memory by a lot? I'm concerned this could be very flaky given that it's dependent on external factors (compiler output, context switches, etc). Now that we have this test, is there any value in ha

[Lldb-commits] [PATCH] D124064: [NFC][trace] simplify the instruction dumper

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. two very minor comments, but looks good - I'm sure you already have, but be sure to run the dumper unittest test to ensure the output didn't change unexpectedly as a result some minor format

[Lldb-commits] [PATCH] D123982: [trace][intel pt] Support events

jj10306 added inline comments. Comment at: lldb/include/lldb/Target/TraceCursor.h:265 +class TraceEventsUtils { +public: nit: maybe this will change in the future to where this will have data and instance members, but if not, consider just using a namespace he

[Lldb-commits] [PATCH] D123982: [trace][intel pt] Support events

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. Thanks for making those changes - lgtm! Comment at: lldb/source/Plugins/Trace/intel-pt/DecodedThread.h:258 /// first one. Otherwise, this map will be empty. std::map

[Lldb-commits] [PATCH] D124573: [trace][intelpt] Support system-wide tracing [1] - Add a method for accessing the list of logical core ids

jj10306 added inline comments. Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:22 +// List of data kinds used by jLLDBGetState and jLLDBGetBinaryData. +struct IntelPTDataKinds { + static const char *kProcFsCpuInfo; why not use an enum here?

[Lldb-commits] [PATCH] D124573: [trace][intelpt] Support system-wide tracing [1] - Add a method for accessing the list of logical core ids

jj10306 accepted this revision. jj10306 added inline comments. Comment at: lldb/unittests/Process/Linux/PerfTests.cpp:53 +TEST(Perf, HardcodedLogicalCoreIDs) { + Expected> core_ids = Not directly related to this diff, but I just realized the proc fs parsing l

[Lldb-commits] [PATCH] D124640: [trace][intelpt] Support system-wide tracing [2] - Add a dummy --per-core-tracing option

jj10306 requested changes to this revision. jj10306 added a comment. This revision now requires changes to proceed. Before doing a complete review can you provide clarity on the decision to only support perCore for process wide (see my inline comment with my thoughts/questions)? Understanding t

[Lldb-commits] [PATCH] D124648: [trace][intelpt] Support system-wide tracing [3] - Refactor IntelPTThreadTrace

jj10306 requested changes to this revision. jj10306 added inline comments. This revision now requires changes to proceed. Comment at: lldb/docs/lldb-gdb-remote.txt:498 // Binary data kinds: -//- threadTraceBuffer: trace buffer for a thread. +//- traceBuffer: trace buffe

[Lldb-commits] [PATCH] D124573: [trace][intelpt] Support system-wide tracing [1] - Add a method for accessing the list of logical core ids

jj10306 added a comment. Much cleaner now, thanks for separating out the procfs logic 🙂 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124573/new/ https://reviews.llvm.org/D124573 ___ lldb-commits mailing

[Lldb-commits] [PATCH] D124648: [trace][intelpt] Support system-wide tracing [3] - Refactor IntelPTThreadTrace

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. I'm assuming the changes to make the single buffer trace class generic for threads and cpu buffers is done in the next diff so stamping this, feel free to update with those changes if that i

[Lldb-commits] [PATCH] D124858: [trace][intelpt] Support system-wide tracing [4] - Support per core tracing on lldb-server

jj10306 added inline comments. Comment at: lldb/docs/lldb-gdb-remote.txt:372-374 +// This limit applies to the sum of the sizes of all trace and core +// buffers for the current process, excluding the ones started with +// "thread tracing". ---

[Lldb-commits] [PATCH] D124858: [trace][intelpt] Support system-wide tracing [4] - Support per core tracing on lldb-server

jj10306 added inline comments. Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:228-234 + return m_per_thread_process_trace_up->TraceStart(tid); } Error IntelPTCollector::OnThreadDestroyed(lldb::tid_t tid) { - if (IsProcessTracingEnabled() && m_process_tra

[Lldb-commits] [PATCH] D124962: [trace][intelpt] Support system-wide tracing [5] - Disable/enable per-core tracing based on the process state

jj10306 added inline comments. Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp:234 +IntelPTSingleBufferTrace::GetTraceBuffer(size_t offset, size_t size, + bool flush) { std::vector data(size, 0); --

[Lldb-commits] [PATCH] D124858: [trace][intelpt] Support system-wide tracing [4] - Support per core tracing on lldb-server

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. couple minor things, but looks good overall Comment at: lldb/docs/lldb-gdb-remote.txt:465-474 // "tid": , // "binaryData": [ //{ // "kind": ,

[Lldb-commits] [PATCH] D124858: [trace][intelpt] Support system-wide tracing [4] - Support per core tracing on lldb-server

jj10306 added inline comments. Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:152-156 + IntelPTPerThreadProcessTraceUP m_per_thread_process_trace_up; + /// Cores traced due to per-core "process tracing". Only one active + /// "process tracing" instance is all

[Lldb-commits] [PATCH] D125047: [trace][intelpt] Support system-wide tracing [6] - Break IntelPTCollector into smaller files and minor refactor

jj10306 accepted this revision. jj10306 added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Utility/TraceGDBRemotePackets.cpp:136 {"tid", packet.tid}, -{"size", packet.size}}); +

[Lldb-commits] [PATCH] D124962: [trace][intelpt] Support system-wide tracing [5] - Disable/enable per-core tracing based on the process state

jj10306 accepted this revision. jj10306 added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp:211 +Error IntelPTSingleBufferTrace::SetCollectionState( +TraceCollectionState new_stat

[Lldb-commits] [PATCH] D125503: [trace][intelpt] Support system-wide tracing [7] - Create a base IntelPTProcessTrace class

jj10306 requested changes to this revision. jj10306 added inline comments. This revision now requires changes to proceed. Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.h:73 + /// The target process. NativeProcessProtocol &m_process; /// Threads traced due

[Lldb-commits] [PATCH] D125850: [trace][intelpt] Support system-wide tracing [8] - Improve the single buffer perf_event configuration

jj10306 requested changes to this revision. jj10306 added inline comments. This revision now requires changes to proceed. Comment at: lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp:186 attr.exclude_kernel = 1; - attr.sample_type = PERF_SAMPLE_TIME; - attr.sam

[Lldb-commits] [PATCH] D125897: [trace][intelpt] Support system-wide tracing [9] - Collect and return context switch traces

jj10306 requested changes to this revision. jj10306 added a comment. This revision now requires changes to proceed. Submitting my comments so far, will continue my review in a bit Comment at: lldb/include/lldb/Utility/TraceIntelPTGDBRemotePackets.h:26-28 static const char *k

[Lldb-commits] [PATCH] D125897: [trace][intelpt] Support system-wide tracing [9] - Collect and return context switch traces

jj10306 added inline comments. Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:122-129 void IntelPTMultiCoreTrace::ForEachCore( std::function callback) { for (auto &it : m_traces_per_core) -callback(it.first, *it.second); +callbac

[Lldb-commits] [PATCH] D126267: [trace][intelpt] Support system-wide tracing [13] - Add context switch decoding

jj10306 requested changes to this revision. jj10306 added inline comments. This revision now requires changes to proceed. Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.cpp:22-42 +struct PerfContextSwitchRecord { + struct perf_event_header header; +

[Lldb-commits] [PATCH] D125932: [trace][intelpt] Support system-wide tracing [10] - Return warnings and tsc information from lldb-server.

jj10306 added inline comments. Comment at: lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp:40 +IntelPTCollector::FetchPerfTscConversionParameters() { + if (!m_cached_tsc_conversion) { +if (Expected tsc_conversion = Don't we want to always reload the p

[Lldb-commits] [PATCH] D125943: [trace][intelpt] Support system-wide tracing [11] - Read warnings and perf conversion in the client

jj10306 accepted this revision. jj10306 added a comment. This revision is now accepted and ready to land. Herald added a subscriber: jsji. lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125943/new/ https://reviews.llvm.org/D125943

[Lldb-commits] [PATCH] D126015: [trace][intelpt] Support system-wide tracing [12] - Support multi-core trace load and save

jj10306 requested changes to this revision. jj10306 added a comment. This revision now requires changes to proceed. feedback-v1 Comment at: lldb/include/lldb/Target/Trace.h:520 + /// core id -> data kind -> size + llvm::DenseMap> + m_live_core_data; Woul

[Lldb-commits] [PATCH] D126015: [trace][intelpt] Support system-wide tracing [12] - Support multi-core trace load and save

jj10306 added a comment. feedback-v2 Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:183 +Expected> +PerfEvent::ReadFlushedOutDataCyclicBuffer(size_t offset, size_t size) { + CollectionState previous_state = m_collection_state; Do we need the offset/siz

[Lldb-commits] [PATCH] D126015: [trace][intelpt] Support system-wide tracing [12] - Support multi-core trace load and save

jj10306 added a comment. feeback-v3 - completed review Comment at: lldb/docs/use/intel_pt.rst:168 :: { +"type": "intel-pt", Consider adding a section on the perfTscConversion parameters while we're editing this file as I don't currently see that in thi

  1   2   >