wallace requested changes to this revision. wallace added a comment. This revision now requires changes to proceed.
just some minor changes, including requiring the FileSpec as input to the SB function ================ Comment at: lldb/include/lldb/API/SBDebugger.h:394-395 + /// Load trace from trace session file and create Targets based on the + /// contents of the file. + /// ---------------- ================ Comment at: lldb/include/lldb/API/SBDebugger.h:403 + /// trace session. + SBTrace LoadTraceFromFile(SBError &error, const char *trace_file_path); + ---------------- JDevlieghere wrote: > Can this take a filespec? +1 ================ Comment at: lldb/include/lldb/API/SBDebugger.h:403 + /// trace session. + SBTrace LoadTraceFromFile(SBError &error, const char *trace_file_path); + ---------------- wallace wrote: > JDevlieghere wrote: > > Can this take a filespec? > +1 We need to come up with a good public name for the json file. I've been using trace session file, but I dislike. What about trace description file? Then we can have a "trace bundle" that contains "trace description file". Do you have better suggestions? ================ Comment at: lldb/include/lldb/API/SBTrace.h:15 -class TraceImpl; - ---------------- +1 ================ Comment at: lldb/include/lldb/API/SBTrace.h:24-41 + /// Load trace from trace session file and create Targets based on the + /// contents of the file. + /// + /// \param[out] error + /// An error if the trace could not be created. + /// + /// \param[in] debugger ---------------- you can also refer to the documentation of the other function if you don't want to repeat yourself ================ Comment at: lldb/source/API/SBTrace.cpp:32 + const char *trace_session_file_path) { + + llvm::Expected<lldb::TraceSP> trace_or_err = ---------------- you'll need LLDB_INSTRUMENT_VA(this, error, debugger, trace_session_file_path). This LLDB_INSTRUMENT_VA macro is used for diagnosing test failures on some systems. I haven't used this diagnostics systems myself but let's adhere to the convention ================ Comment at: lldb/source/API/SBTrace.cpp:33-38 + llvm::Expected<lldb::TraceSP> trace_or_err = + Trace::LoadPostMortemTraceFromFile(debugger.ref(), + trace_session_file_path); + + if (!trace_or_err) { + error.SetErrorString(llvm::toString(trace_or_err.takeError()).c_str()); ---------------- if you want, add `using namespace llvm` so that you don't have to write llvm:: everywhere. You can also omit lldb:: already ================ Comment at: lldb/source/Commands/CommandObjectTrace.cpp:92-105 + llvm::Expected<lldb::TraceSP> trace_or_err = + Trace::LoadPostMortemTraceFromFile(GetDebugger(), command[0].ref()); - auto buffer_or_error = llvm::MemoryBuffer::getFile(json_file.GetPath()); - if (!buffer_or_error) { - return end_with_failure(llvm::createStringError( - std::errc::invalid_argument, "could not open input file: %s - %s.", - json_file.GetPath().c_str(), - buffer_or_error.getError().message().c_str())); + if (!trace_or_err) { + result.AppendErrorWithFormat( + "%s\n", llvm::toString(trace_or_err.takeError()).c_str()); + return false; ---------------- nice refactor ================ Comment at: lldb/source/Commands/CommandObjectTrace.cpp:102 + lldb::TraceSP trace_sp = trace_or_err.get(); + if (m_options.m_verbose && trace_sp) { + result.AppendMessageWithFormatv("loading trace with plugin {0}\n", ---------------- you can drop the trace_sp check because it should be valid because otherwise you'd have gotten an Expected ================ Comment at: lldb/source/Target/Trace.cpp:92-114 +llvm::Expected<lldb::TraceSP> +Trace::LoadPostMortemTraceFromFile(Debugger &debugger, + llvm::StringRef trace_session_file_path) { + + FileSpec json_file(trace_session_file_path); + + auto buffer_or_error = llvm::MemoryBuffer::getFile(json_file.GetPath()); ---------------- you can drop all llvm:: and lldb:: if you want ================ Comment at: lldb/test/API/commands/trace/TestTraceLoad.py:12 + @testSBAPIAndCommands def testLoadMultiCoreTrace(self): ---------------- thanks! ================ Comment at: lldb/test/API/commands/trace/TestTraceLoad.py:102 # We test first an invalid type - self.expect("trace load -v " + os.path.join(src_dir, "intelpt-trace", "trace_bad.json"), error=True, - substrs=['''error: expected object at traceSession.processes[0] + self.traceLoad(traceSessionFilePath=trace_definition_file, error=True, substrs=['''error: expected object at traceSession.processes[0] ---------------- split into multiple lines Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128107/new/ https://reviews.llvm.org/D128107 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits