[Lldb-commits] [PATCH] D107669: [trace] [intel pt] Create a "process trace save" command
walter erquinigo via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Fri Aug 13 23:22:55 PDT 2021
wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.
Almost there. Many cosmetic changes but the logic is all good.
================
Comment at: lldb/include/lldb/Target/Trace.h:178
+ /// \param[in] tid
/// The thread in question.
///
----------------
The id of the thread in question
================
Comment at: lldb/source/Commands/Options.td:739
+
+let Command = "process trace save" in {
----------------
remove this line
================
Comment at: lldb/source/Commands/Options.td:748
+
+
let Command = "script import" in {
----------------
remove this line
================
Comment at: lldb/source/Plugins/Trace/common/TraceJSONStructs.cpp:28-29
+ oss << "0x" << std::hex << module.load_address.value;
+ std::string str(oss.str());
+ result["loadAddress"] = str;
+ if (module.uuid)
----------------
merge into one single line
================
Comment at: lldb/source/Plugins/Trace/common/TraceJSONStructs.cpp:45-47
+ for (JSONThread e : process.threads) {
+ threads_arr.push_back(toJSON(e));
+ }
----------------
delete braces
================
Comment at: lldb/source/Plugins/Trace/common/TraceJSONStructs.cpp:51-53
+ for (JSONModule e : process.modules) {
+ modules_arr.push_back(toJSON(e));
+ }
----------------
same here
================
Comment at: lldb/source/Plugins/Trace/common/TraceJSONStructs.cpp:61-63
+ for (JSONProcess e : session_base.processes) {
+ arr.push_back(toJSON(e));
+ }
----------------
ditto
================
Comment at: lldb/source/Plugins/Trace/common/TraceJSONStructs.h:28-33
+ JSONModule() = default;
+
+ JSONModule(std::string system_path, llvm::Optional<std::string> file,
+ JSONAddress load_address, llvm::Optional<std::string> uuid)
+ : system_path(system_path), file(file), load_address(load_address),
+ uuid(uuid) {}
----------------
you can omit these constructors because they don't have anything custom
================
Comment at: lldb/source/Plugins/Trace/common/TraceJSONStructs.h:42-46
+ JSONThread() = default;
+
+ JSONThread(int64_t tid, std::string trace_file)
+ : tid(tid), trace_file(trace_file) {}
+
----------------
same here, just delete these lines
================
Comment at: lldb/source/Plugins/Trace/common/TraceJSONStructs.h:52-57
+ JSONProcess() = default;
+
+ JSONProcess(int64_t pid, std::string triple, std::vector<JSONThread> threads,
+ std::vector<JSONModule> modules)
+ : pid(pid), triple(triple), threads(threads), modules(modules) {}
+
----------------
similarly, delete these
================
Comment at: lldb/source/Plugins/Trace/common/TraceJSONStructs.h:69-72
+ JSONTraceSessionBase() = default;
+
+ JSONTraceSessionBase(std::vector<JSONProcess> processes)
+ : processes(processes) {}
----------------
delete
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.cpp:27
+llvm::Error
+TraceSessionSaver::WriteSessionToFile(const llvm::json::Value &trace_session_jo,
+ FileSpec directory) {
----------------
trace_session_description
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.cpp:34
+ std::ofstream os(trace_path.GetPath());
+ os << out;
+ os.close();
----------------
move line 30 here to avoid that unnecessary out variable
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.cpp:50
+
+ JSONTraceSessionBase result;
+ Expected<std::vector<JSONThread>> json_threads =
----------------
json_session_description
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.cpp:58-60
+ if (!json_modules) {
+ return json_modules.takeError();
+ }
----------------
delete braces
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.cpp:77
+ FileSpec directory) {
+ std::vector<JSONThread> result;
+ for (ThreadSP thread_sp : live_process.Threads()) {
----------------
rename this to json_threads
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.cpp:86
+ ".trace");
+ json_thread.trace_file = raw_trace_path.GetPath().c_str();
+ result.push_back(json_thread);
----------------
avoid this. Instead of creating first an empty json_thread with invalid values, wait until this moment to create a valid JSONThread
json_threads.push_back(JSONThread { thread_sp->GetID(), raw_trace_path.GetPath().c_str() });
that way all the code that you write is valid at all times
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.cpp:94-96
+ if (!raw_trace.get()) {
+ continue;
+ }
----------------
delete braces
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.cpp:114
+ FileSpec directory) {
+ std::vector<JSONModule> result;
+ ModuleList module_list = live_process.GetTarget().GetImages();
----------------
json_modules
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.cpp:122-124
+ if (!module_sp->GetFileSpec().IsAbsolute()) {
+ continue;
+ }
----------------
delete braces
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.cpp:127-129
+ if (objfile == nullptr) {
+ continue;
+ }
----------------
delete braces
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:19
+public:
+ /// Save the trace's session to a given directory as a JSON file.
+ /// The filename is \a <directory>/trace.json
----------------
Save the trace session description JSON object inside the given directory as a file named \a trace.json.
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:22
+ ///
+ /// \param[in] trace_session_jo
+ /// The trace's session, as JSON Object.
----------------
trace_session_description is more readable. Reading 'jo' is not intuitive and confuses a bit
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:31
+ /// otherwise.
+ ///
+ static llvm::Error
----------------
delete this line
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:33
+ static llvm::Error
+ WriteSessionToFile(const llvm::json::Value &trace_session_jo,
+ FileSpec directory);
----------------
trace_session_description_json
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:36
+
+ /// Build processes section of the trace's session.
+ ///
----------------
Build the processes section of the trace session description file. Besides returning the processes information, this method saves to disk all modules and raw traces corresponding to the traced threads of the given process.
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:42-45
+ /// Callback function that receives a thread ID and returns its raw trace.
+ /// return raw trace in bytes if the operation was successful.
+ /// return an \a llvm::Error if failed at getting raw trace.
+ /// return \a None if raw trace got is INVALID.
----------------
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:49
+ /// The directory where the thread files will be saved when building
+ /// processes section.
+ ///
----------------
The directory where files will be saved when building the processes section.
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:51-53
+ /// \return The processes section to be built. Or an error if we can't build
+ /// processes section.
+ ///
----------------
```
/// \return
/// The processes section or an error in case of failures.
```
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:53
+ /// processes section.
+ ///
+ static llvm::Expected<JSONTraceSessionBase> BuildProcessesSection(
----------------
delete
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:61
+
+ /// Build threads sub-section inside processes section of the trace's session.
+ /// The raw trace for a thread is written to a file with filename
----------------
Build the threads ... of the trace session description file.
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:62-63
+ /// Build threads sub-section inside processes section of the trace's session.
+ /// The raw trace for a thread is written to a file with filename
+ /// \a <directory>/thread_id.trace.
+ ///
----------------
For each traced thread, its raw trace is also written to the file thread_id_.trace inside of the given directory.
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:69-72
+ /// Callback function that receives a thread ID and returns its raw trace.
+ /// return raw trace in bytes if the operation was successful.
+ /// return an \a llvm::Error if failed at getting raw trace.
+ /// return \a None if raw trace got is INVALID.
----------------
copy the comment from above
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:75-76
+ /// \param[in] directory
+ /// The directory where the thread files will be saved when building
+ /// the threads section.
+ ///
----------------
The directory where files will be saved when building the threads section.
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:78-79
+ ///
+ /// \return The threads section to be built. Or an error if we can't build
+ /// threads sections.
+ ///
----------------
You are making a lot of mistakes at formatting the comments. Make sure to look for existing examples as reference.
```
/// \return
/// The threads section or an error in case of failures.
```
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:80
+ /// threads sections.
+ ///
+ static llvm::Expected<std::vector<JSONThread>> BuildThreadsSection(
----------------
delete
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:89
+
+/// Build modules sub-section inside processes section of the trace's session.
+/// The original modules will be copied over to the \a <directory/modules>
----------------
Build the modules sub-section of the trace session description file.
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:92
+/// folder. Invalid modules are skipped.
+/// Saving modules have the benifits of making trace saving more
+/// self-contained, as both trace and modules can now be saved on one machine,
----------------
Copying the modules has the benefit of making these trace session directories self-contained, as the raw traces and modules are part of the output directory and can be sent to another machine, where lldb can load them and replicate exactly the same trace session.
================
Comment at: lldb/source/Plugins/Trace/common/TraceSessionSaver.h:106-108
+/// \return The modules section to be built. Or an error if we can't build
+/// modules section.
+///
----------------
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:75
+ return createStringError(inconvertibleErrorCode(),
+ "Saving live trace requires a live process.");
+
----------------
Saving a trace requires...
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h:35-38
+ JSONTraceIntelPTTrace() = default;
+
+ JSONTraceIntelPTTrace(std::string type, JSONTraceIntelPTCPUInfo cpuInfo)
+ : type(type), cpuInfo(cpuInfo) {}
----------------
delete
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h:45-49
+ JSONTraceIntelPTSession() = default;
+
+ JSONTraceIntelPTSession(JSONTraceIntelPTTrace ipt_trace,
+ JSONTraceSessionBase session_base)
+ : ipt_trace(ipt_trace), session_base(session_base) {}
----------------
delete
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.cpp:60-61
+
+ JSONTraceIntelPTSession json_trace_intel_pt_session(
+ json_trace_intel_pt_trace.get(), json_trace_session_base.get());
+
----------------
Use {} constructors
```
JSONTraceIntelPTSession json_trace_intel_pt_session {
json_trace_intel_pt_trace.get(), json_trace_session_base.get() };
```
that way you don't need to specify your own constructor
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.h:31-33
+ /// \param[in] live_process
+ /// The process whose trace will be saved to disk.
+ ///
----------------
you don't need to pass this variable. You should be able to get the live_process directly from trace_ipt. You can create a method in TraceIntelPT for this if needed
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.h:40-54
+ /// \return
+ /// \a llvm::success if the operation was successful, or an \a llvm::Error
+ /// otherwise.
+ ///
+ llvm::Error SaveToDisk(Process &live_process, TraceIntelPT &trace_ipt,
+ FileSpec directory);
+
----------------
in the first return you are using the correct formatting, but in the second you don't. Try to be more careful
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.h:43
+ /// otherwise.
+ ///
+ llvm::Error SaveToDisk(Process &live_process, TraceIntelPT &trace_ipt,
----------------
delete
================
Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSessionSaver.h:55
+ /// trace section.
+ ///
+ llvm::Expected<JSONTraceIntelPTTrace>
----------------
delete
================
Comment at: lldb/test/API/commands/trace/TestTraceSave.py:38-46
+ self.expect("n")
+ self.expect("n")
+ self.expect("n")
+ self.expect("n")
+ self.expect("n")
+ self.expect("n")
+ self.expect("n")
----------------
this a little bit weird, you can instead place a breakpoint at the line that you want and stop just there
================
Comment at: lldb/test/API/commands/trace/TestTraceSave.py:75
+ self.assertEqual(res.Succeeded(), True)
+ self.assertEqual(res.GetOutput(), last_ten_instructions)
----------------
Also add a test in which you pass an invalid directory
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107669/new/
https://reviews.llvm.org/D107669
More information about the lldb-commits
mailing list