[Lldb-commits] [PATCH] D128107: [trace] Add LoadTraceFromFile to SBDebugger and SBTrace

[walter erquinigo via Phabricator via lldb-commits]

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