[PATCH] D126015: [trace][intelpt] Support system-wide tracing [12] - Support multi-core trace load and save
Jakob Johnson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 3 13:05:21 PDT 2022
jj10306 added a comment.
feedback-v2
================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:183
+Expected<std::vector<uint8_t>>
+PerfEvent::ReadFlushedOutDataCyclicBuffer(size_t offset, size_t size) {
+ CollectionState previous_state = m_collection_state;
----------------
Do we need the offset/size? These arguments make the reading logic much more involved versus reading the whole buffer.
I understand that this is a more "general" method, but iiuc currently we only read the entire buffer. I guess in the future somebody could want the flexibility of providing an offset/size, but I don't see the need currently.
wdyt?
Same comment applies to the AUX buffer read method
================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:206-212
+ for (uint64_t i = actual_data_head + offset;
+ i < data_size && output.size() < size; i++)
+ output.push_back(data[i]);
+
+ // We need to find the starting position for the left part if the offset was
+ // too big
+ uint64_t left_part_start = actual_data_head + offset >= data_size
----------------
What happens if an offset larger then the size of the buffer is provided? should that offset be wrapped or not?
What happens if a size larger than the size of the buffer is provided? should we only copy up to buffer size bytes or should we do multiple copies of the buffer to satisfy the request?
As I mentioned above, imo the ambiguity/complexity these arguments introduce is not worth the value add (which currently is none since we are always copying the whole buffer).
If we do stick with these args, we should make it very clear what happens in the cases I mentioned above and/or put restrictions on the ranges of these values (ie they must always be less than the buffer size)
================
Comment at: lldb/source/Plugins/Trace/common/ThreadPostMortemTrace.h:47
/// The trace file of this thread.
- const FileSpec &GetTraceFile() const;
+ const llvm::Optional<FileSpec> &GetTraceFile() const;
----------------
why is this now optional?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:169
+ ///
+ /// \param[in] traces_proceses
+ /// The processes traced in the live session.
----------------
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:176-177
+ TraceIntelPT(JSONTraceSession &session,
+ llvm::ArrayRef<lldb::ProcessSP> traced_processes,
+ llvm::ArrayRef<lldb::ThreadPostMortemTraceSP> traced_threads);
----------------
do we need the threads explicitly or does `traced_processes` implicitly contain this info?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:53
+ return json::Object{{"tid", thread.tid},
+ {"traceBuffer", thread.trace_buffer}};
+}
----------------
if this is none, does this still create a key value pair?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:88-90
+ if (!o || !o.map("coreId", core_id) ||
+ !o.map("traceBuffer", core.trace_buffer) ||
+ !o.map("contextSwitchTrace", core.context_switch_trace))
----------------
nit: use what De Morgan taught us (;
Having to add a `!` for each new addition to the predicate is very error prone and would be a pain to debug if you accidentally forgot to add the negation.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:109-110
+ uint64_t family, model, stepping;
+ if (!o || !o.map("vendor", vendor) || !o.map("family", family) ||
+ !o.map("model", model) || !o.map("stepping", stepping))
+ return false;
----------------
same comment as above related to De Morgan's law
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:131-133
+ if (!o || !o.map("processes", session.processes) ||
+ !o.map("type", session.type) || !o.map("cores", session.cores) ||
+ !o.map("tscPerfZeroConversion", session.tsc_perf_zero_conversion))
----------------
same comment as above related to De Morgan's law
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h:32-40
+struct JSONThread {
+ int64_t tid;
+ llvm::Optional<std::string> trace_buffer;
};
-struct JSONTraceIntelPTTrace {
- std::string type;
- JSONTraceIntelPTCPUInfo cpuInfo;
+struct JSONProcess {
+ int64_t pid;
----------------
If the intention here is for system wide tracing to not provide threads while single thread tracing does then should we change this like so?
(i know you're currently working on the thread detection so potentially this will be addressed then?)
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp:31
+
+Error TraceIntelPTSessionFileParser::ParseModule(lldb::TargetSP &target_sp,
+ const JSONModule &module) {
----------------
take a reference to the underlying object directly?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp:33
+ const JSONModule &module) {
+ FileSpec system_file_spec(module.system_path);
+ NormalizePath(system_file_spec);
----------------
what happens if system_path is invalid? or is it already guaranteed to exist at this point (ie did we already check this somewhere?)?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp:44-45
+
+ if (module.uuid.hasValue())
+ module_spec.GetUUID().SetFromStringRef(*module.uuid);
+
----------------
curious, what is the debugger able to provide when the uuid is provided versus when it isn't?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp:71
+ThreadPostMortemTraceSP
+TraceIntelPTSessionFileParser::ParseThread(ProcessSP &process_sp,
+ const JSONThread &thread) {
----------------
take a reference to the underlying object directly?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp:227
- TraceSP trace_instance(new TraceIntelPT(cpu_info, threads));
+ TraceSP trace_instance(new TraceIntelPT(session, processes, threads));
for (const ParsedProcess &parsed_process : parsed_processes)
----------------
use make_shared instead?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h:55-57
lldb::TraceSP
- CreateTraceIntelPTInstance(const pt_cpu &cpu_info,
+ CreateTraceIntelPTInstance(JSONTraceSession &session,
std::vector<ParsedProcess> &parsed_processes);
----------------
docs?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h:64-69
+ lldb::ThreadPostMortemTraceSP ParseThread(lldb::ProcessSP &process_sp,
+ const JSONThread &thread);
+
+ llvm::Expected<ParsedProcess> ParseProcess(const JSONProcess &process);
+
+ llvm::Error ParseModule(lldb::TargetSP &target_sp, const JSONModule &module);
----------------
docs?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.h:90
const llvm::json::Value &m_trace_session_file;
+ std::string m_session_file_dir;
};
----------------
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp:33
-llvm::Error TraceIntelPTSessionSaver::SaveToDisk(TraceIntelPT &trace_ipt,
- FileSpec directory) {
- Process *live_process = trace_ipt.GetLiveProcess();
- if (live_process == nullptr)
+static llvm::Error WriteBytesToDisk(FileSpec &output_file,
+ ArrayRef<uint8_t> data) {
----------------
docs?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp:38
+ out_fs.write(reinterpret_cast<const char *>(&data[0]),
+ data.size() * sizeof(uint8_t));
+ out_fs.close();
----------------
to better defend against if the size of data's element's changes?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp:65
+ std::ofstream os(trace_path.GetPath());
+ os << std::string(formatv("{0:2}", trace_session_json));
+ os.close();
----------------
is the string construction necessary?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp:93
+ threads_dir.AppendPathComponent("threads");
+ FileSystem::Instance().Resolve(threads_dir);
+ sys::fs::create_directories(threads_dir.GetCString());
----------------
nit: what's the difference between this and using the `NormalizePath` method you added? I know that method is private but could that potentially be made public or global so we can use that here?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp:131-132
+ cores_dir.AppendPathComponent("cores");
+ FileSystem::Instance().Resolve(cores_dir);
+ sys::fs::create_directories(cores_dir.GetCString());
+
----------------
same comment as above related to potentially using the `ResolvePath` method?
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp:141
+ output_trace.AppendPathComponent(std::to_string(core_id) +
+ ".intelpt_trace");
+ json_core.trace_buffer = output_trace.GetPath();
----------------
nit: we need to discuss this offline to ensure we are following the same structure as currently I have
```
cpus/
<cpuId>/
ipt.data
context_switch.data
```
whereas this is a little different (ie using "cores" instead of "cpus" and not including the sub directory for each core id.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp:188
+/// \a /usr/lib/foo/libbar.so, then it will be copied to
+/// \a <directory>/modules/usr/lib/foo/libbar.so.
+///
----------------
<directory> is the directory that the trace.json file will live at, correct?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126015/new/
https://reviews.llvm.org/D126015
More information about the llvm-commits
mailing list