[PATCH] D126015: [trace][intelpt] Support system-wide tracing [12] - Support multi-core trace load and save
walter erquinigo via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 14 00:14:58 PDT 2022
wallace added inline comments.
================
Comment at: lldb/include/lldb/Target/Trace.h:520
+ /// core id -> data kind -> size
+ llvm::DenseMap<lldb::core_id_t, std::unordered_map<std::string, size_t>>
+ m_live_core_data;
----------------
jj10306 wrote:
> Would this work instead? I noticed that the several other maps around this code also use unordered_map and string, maybe we could update those too at some point.
great idea! I'll use ConstString instead of string. That will work. ConstString is like StringRef but uses a shared pool for storage.
================
Comment at: lldb/include/lldb/Target/Trace.h:538
+ /// core id -> data kind -> file
+ llvm::DenseMap<lldb::core_id_t, std::unordered_map<std::string, FileSpec>>
+ m_postmortem_core_data;
----------------
jj10306 wrote:
> same as comment above related to using LLVM types rather than std types wherever possible.
+1
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:193
-Expected<std::vector<uint8_t>>
-IntelPTMultiCoreTrace::GetBinaryData(const TraceGetBinaryDataRequest &request) {
- return createStringError(inconvertibleErrorCode(), "Unimplemented");
+Expected<Optional<std::vector<uint8_t>>>
+IntelPTMultiCoreTrace::TryGetBinaryData(
----------------
jj10306 wrote:
> `Expected<Optional<...>>` feels weirddddd. Are both of these "layers" needed?
> I noticed this in a couple places but I'm just leaving a single comment here.
I think they are fine because in this case the collector is looking for who might own the data being fetched, and each object that implements TryGetBinaryData can return 3 possible outputs:
- None -> it doesn't have the data
- the sought value
- an error -> it should have the data but something went wrong
so this is fine.
================
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;
----------------
jj10306 wrote:
> 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
After giving many thoughts to this, I agree with you. If we ever need to stream the data, we can implement it then, but it's not needed now.
================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:191
+ * with respect to their definition of head pointer. In the case
+ * of Aux data buffer the head always wraps around the aux buffer
+ * and we don't need to care about it, whereas the data_head keeps
----------------
jj10306 wrote:
> Be careful with the "strong" language here. The AUX head only wraps around when the AUX buffer is mmaped with read-only, if it is mmapped with write perms, then the AUX head behaves like the data head (it doesn't auto wrap)
>
> See this snipped of kernel code, the overwrite flag is set based on the mmap PROT and that overwrite flag is passed to the intelpt code.
> https://github.com/torvalds/linux/blob/df0cc57e/kernel/events/ring_buffer.c#L670-L733
+1
================
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
----------------
jj10306 wrote:
> 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)
yep, i got rid of all of this
================
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;
----------------
jj10306 wrote:
> why is this now optional?
because now trace buffers are optional for threads. The trace buffer might be at the core level
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h:169
+ ///
+ /// \param[in] traces_proceses
+ /// The processes traced in the live session.
----------------
jj10306 wrote:
>
+1
================
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);
----------------
jj10306 wrote:
> do we need the threads explicitly or does `traced_processes` implicitly contain this info?
we don't really need, but it's useful because this way we don't need to cast ThreadSP to ThreadPostMortemTraceSP, which might break in some situations. So I'm making this constructor being harder to use to that it's only used by the bundle parser and not used by anyone else incorrectly
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.cpp:53
+ return json::Object{{"tid", thread.tid},
+ {"traceBuffer", thread.trace_buffer}};
+}
----------------
jj10306 wrote:
> if this is none, does this still create a key value pair?
yes, with null as value, but I'm changing it to
if (thread.trace_buffer)
obj["traceBuffer"] = *thread.trace_buffer;
for simplicity of the output
================
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))
----------------
jj10306 wrote:
> 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.
i like this!
================
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;
----------------
jj10306 wrote:
> same comment as above related to De Morgan's law
+1
================
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))
----------------
jj10306 wrote:
> same comment as above related to De Morgan's law
+1
================
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;
----------------
jj10306 wrote:
> 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?)
as discussed offline, we'll keep the threads section because it will be useful for handling named threads
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp:31
+
+Error TraceIntelPTSessionFileParser::ParseModule(lldb::TargetSP &target_sp,
+ const JSONModule &module) {
----------------
jj10306 wrote:
> take a reference to the underlying object directly?
good idea
================
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);
----------------
jj10306 wrote:
> what happens if system_path is invalid? or is it already guaranteed to exist at this point (ie did we already check this somewhere?)?
we are assuming it exists. But we actually really check it when we try to fetch its data.
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp:44-45
+
+ if (module.uuid.hasValue())
+ module_spec.GetUUID().SetFromStringRef(*module.uuid);
+
----------------
jj10306 wrote:
> curious, what is the debugger able to provide when the uuid is provided versus when it isn't?
if the UUID is provided and doesn't match the actual binary's UUID from the elf build section, then LLDB returns an error
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionFileParser.cpp:71
+ThreadPostMortemTraceSP
+TraceIntelPTSessionFileParser::ParseThread(ProcessSP &process_sp,
+ const JSONThread &thread) {
----------------
jj10306 wrote:
> take a reference to the underlying object directly?
+1
================
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)
----------------
jj10306 wrote:
> use make_shared instead?
can't because the constructor is private and not visible to make_shared
================
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);
----------------
jj10306 wrote:
> docs?
+1
================
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);
----------------
jj10306 wrote:
> docs?
+1
================
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;
};
----------------
jj10306 wrote:
>
+1
================
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) {
----------------
jj10306 wrote:
> docs?
+1
================
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();
----------------
jj10306 wrote:
> to better defend against if the size of data's element's changes?
i made this simpler
if (!data.empty())
out_fs.write(reinterpret_cast<const char *>(&data[0]), data.size());
================
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();
----------------
jj10306 wrote:
> is the string construction necessary?
i've changed it to
os << formatv("{0:2}", trace_session_json).str();.
we need to explicitly convert it to string somehow because std::ofstream doesn't understand formatv
================
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());
----------------
jj10306 wrote:
> 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?
i got rid of this, it was not needed
================
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());
+
----------------
jj10306 wrote:
> same comment as above related to potentially using the `ResolvePath` method?
+1
================
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();
----------------
jj10306 wrote:
> 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.
>
as we discussed offline, the structure itself doesn't matter. LLDB can use a structure and dynolog a different one
================
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.
+///
----------------
jj10306 wrote:
> <directory> is the directory that the trace.json file will live at, correct?
yes
================
Comment at: lldb/source/Target/Trace.cpp:30-32
struct JSONSimpleTraceSession {
- JSONSimplePluginSettings trace;
+ std::string type;
};
----------------
jj10306 wrote:
> do we need this struct or can we just use a string directly since the struct only has a single member?
the structure is useful using fromJSON, which gives nice error messages
================
Comment at: lldb/source/Target/Trace.cpp:134
+ return None;
+ std::unordered_map<std::string, size_t> &single_core_data = it->second;
+ auto single_thread_data_it = single_core_data.find(kind.str());
----------------
jj10306 wrote:
>
+1
================
Comment at: lldb/source/Target/Trace.cpp:135
+ std::unordered_map<std::string, size_t> &single_core_data = it->second;
+ auto single_thread_data_it = single_core_data.find(kind.str());
+ if (single_thread_data_it == single_core_data.end())
----------------
jj10306 wrote:
> why is this named thread_data, isn't this the size of the core's buffer?
silly me
================
Comment at: lldb/source/Target/Trace.cpp:325
+
+ std::unordered_map<std::string, FileSpec> &data_kind_to_file_spec_map =
+ it->second;
----------------
jj10306 wrote:
>
+1
================
Comment at: lldb/unittests/Process/Linux/PerfTests.cpp:9
-#ifdef __x86_64__
-
----------------
jj10306 wrote:
> isn't this needed since `readTsc()` uses intel specific assembly instructions?
ahh good catch
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