Add new API in SBTarget for loading core from SBFile #71769

GeorgeHuyubo · 2023-11-09T05:13:50Z

Add a new API in SBTarget to Load Core from a SBFile.
This will enable a target to load core from a file descriptor.
So that in coredumper, we don't need to write core file to disk, instead we can pass the input file descriptor to lldb directly.

Test:

(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> file_object = open("/home/hyubo/210hda79ms32sr0h", "r")
>>> fd=file_object.fileno()
>>> file = lldb.SBFile(fd,'r', True)
>>> error = lldb.SBError()
>>> target = lldb.debugger.CreateTarget(None)
>>> target.LoadCore(file,error)
SBProcess: pid = 56415, state = stopped, threads = 1

llvmbot · 2023-11-09T05:14:28Z

@llvm/pr-subscribers-lldb

Author: None (GeorgeHuyubo)

Changes

Add a new API in SBTarget to Load Core from a SBFile.
This will enable a target to load core from a file descriptor.
So that in coredumper, we don't need to write core file to disk, instead we can pass the input file descriptor to lldb directly.

Test:

(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
&gt;&gt;&gt; file_object = open("/home/hyubo/210hda79ms32sr0h", "r")
&gt;&gt;&gt; fd=file_object.fileno()
&gt;&gt;&gt; file = lldb.SBFile(fd,'r', True)
&gt;&gt;&gt; error = lldb.SBError()
&gt;&gt;&gt; target = lldb.debugger.CreateTarget(None)
&gt;&gt;&gt; target.LoadCore(file,error)
SBProcess: pid = 56415, state = stopped, threads = 1

Patch is 48.99 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/71769.diff

28 Files Affected:

(modified) lldb/include/lldb/API/SBTarget.h (+2)
(modified) lldb/include/lldb/Target/PostMortemProcess.h (+15)
(modified) lldb/include/lldb/Target/Process.h (+22-15)
(modified) lldb/include/lldb/Target/ProcessTrace.h (+2-2)
(modified) lldb/include/lldb/Target/Target.h (+2-1)
(modified) lldb/include/lldb/lldb-private-interfaces.h (+2-1)
(modified) lldb/source/API/SBTarget.cpp (+39-2)
(modified) lldb/source/Commands/CommandObjectTarget.cpp (+30-17)
(modified) lldb/source/Core/IOHandlerCursesGUI.cpp (+11-2)
(modified) lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp (+16-11)
(modified) lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.h (+1-1)
(modified) lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp (+2-2)
(modified) lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.h (+3-2)
(modified) lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp (+15-13)
(modified) lldb/source/Plugins/Process/elf-core/ProcessElfCore.h (+4-4)
(modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+3-2)
(modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (+3-2)
(modified) lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp (+14-11)
(modified) lldb/source/Plugins/Process/mach-core/ProcessMachCore.h (+3-4)
(modified) lldb/source/Plugins/Process/minidump/ProcessMinidump.cpp (+9-7)
(modified) lldb/source/Plugins/Process/minidump/ProcessMinidump.h (+3-3)
(modified) lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp (+2-2)
(modified) lldb/source/Plugins/Process/scripted/ScriptedProcess.h (+1-1)
(modified) lldb/source/Target/Process.cpp (+5-6)
(modified) lldb/source/Target/ProcessTrace.cpp (+5-4)
(modified) lldb/source/Target/Target.cpp (+1-1)
(modified) lldb/test/API/functionalities/postmortem/elf-core/TestLinuxCore.py (+16)
(modified) lldb/unittests/Target/LocateModuleCallbackTest.cpp (+1-2)

diff --git a/lldb/include/lldb/API/SBTarget.h b/lldb/include/lldb/API/SBTarget.h
index 83087623088c5b4..e49c32d0548501a 100644
--- a/lldb/include/lldb/API/SBTarget.h
+++ b/lldb/include/lldb/API/SBTarget.h
@@ -184,6 +184,8 @@ class LLDB_API SBTarget {
 
   SBProcess LoadCore(const char *core_file);
   SBProcess LoadCore(const char *core_file, lldb::SBError &error);
+  SBProcess LoadCore(SBFile &file);
+  SBProcess LoadCore(SBFile &file, lldb::SBError &error);
 
   /// Launch a new process with sensible defaults.
   ///
diff --git a/lldb/include/lldb/Target/PostMortemProcess.h b/lldb/include/lldb/Target/PostMortemProcess.h
index 7207fc99ef29a41..1f563e5f1ff6b88 100644
--- a/lldb/include/lldb/Target/PostMortemProcess.h
+++ b/lldb/include/lldb/Target/PostMortemProcess.h
@@ -10,6 +10,8 @@
 #define LLDB_TARGET_POSTMORTEMPROCESS_H
 
 #include "lldb/Target/Process.h"
+#include "lldb/Utility/FileSpec.h"
+#include "lldb/lldb-forward.h"
 
 namespace lldb_private {
 
@@ -24,7 +26,20 @@ class PostMortemProcess : public Process {
   using Process::Process;
 
 public:
+  PostMortemProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
+                    lldb::FileSP file_sp)
+      : Process(target_sp, listener_sp), m_core_file(file_sp) {}
+
   bool IsLiveDebugSession() const override { return false; }
+
+  FileSpec GetCoreFile() const override {
+    const FileSpec file_spec;
+    m_core_file->GetFileSpec(const_cast<FileSpec &>(file_spec));
+    return file_spec;
+  }
+
+protected:
+  lldb::FileSP m_core_file;
 };
 
 } // namespace lldb_private
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index a6d3e6c2d16926e..62284b29886a4ac 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -52,6 +52,7 @@
 #include "lldb/Utility/TraceGDBRemotePackets.h"
 #include "lldb/Utility/UnimplementedError.h"
 #include "lldb/Utility/UserIDResolver.h"
+#include "lldb/lldb-forward.h"
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/ArrayRef.h"
@@ -354,12 +355,10 @@ class Process : public std::enable_shared_from_this<Process>,
   };
   // This is all the event bits the public process broadcaster broadcasts.
   // The process shadow listener signs up for all these bits...
-  static constexpr int g_all_event_bits = eBroadcastBitStateChanged 
-                                        | eBroadcastBitInterrupt
-                                        | eBroadcastBitSTDOUT 
-                                        | eBroadcastBitSTDERR
-                                        | eBroadcastBitProfileData 
-                                        | eBroadcastBitStructuredData;
+  static constexpr int g_all_event_bits =
+      eBroadcastBitStateChanged | eBroadcastBitInterrupt | eBroadcastBitSTDOUT |
+      eBroadcastBitSTDERR | eBroadcastBitProfileData |
+      eBroadcastBitStructuredData;
 
   enum {
     eBroadcastInternalStateControlStop = (1 << 0),
@@ -390,12 +389,12 @@ class Process : public std::enable_shared_from_this<Process>,
   ConstString &GetBroadcasterClass() const override {
     return GetStaticBroadcasterClass();
   }
-  
-/// A notification structure that can be used by clients to listen
-/// for changes in a process's lifetime.
-///
-/// \see RegisterNotificationCallbacks (const Notifications&) @see
-/// UnregisterNotificationCallbacks (const Notifications&)
+
+  /// A notification structure that can be used by clients to listen
+  /// for changes in a process's lifetime.
+  ///
+  /// \see RegisterNotificationCallbacks (const Notifications&) @see
+  /// UnregisterNotificationCallbacks (const Notifications&)
   typedef struct {
     void *baton;
     void (*initialize)(void *baton, Process *process);
@@ -507,7 +506,7 @@ class Process : public std::enable_shared_from_this<Process>,
   static lldb::ProcessSP FindPlugin(lldb::TargetSP target_sp,
                                     llvm::StringRef plugin_name,
                                     lldb::ListenerSP listener_sp,
-                                    const FileSpec *crash_file_path,
+                                    lldb::FileSP crash_file_sp,
                                     bool can_connect);
 
   /// Static function that can be used with the \b host function
@@ -579,7 +578,7 @@ class Process : public std::enable_shared_from_this<Process>,
   ///     of CommandObject like CommandObjectRaw, CommandObjectParsed,
   ///     or CommandObjectMultiword.
   virtual CommandObject *GetPluginCommandObject() { return nullptr; }
-  
+
   /// The underlying plugin might store the low-level communication history for
   /// this session.  Dump it into the provided stream.
   virtual void DumpPluginHistory(Stream &s) { return; }
@@ -614,7 +613,7 @@ class Process : public std::enable_shared_from_this<Process>,
     return error;
   }
 
-  /// The "ShadowListener" for a process is just an ordinary Listener that 
+  /// The "ShadowListener" for a process is just an ordinary Listener that
   /// listens for all the Process event bits.  It's convenient because you can
   /// specify it in the LaunchInfo or AttachInfo, so it will get events from
   /// the very start of the process.
@@ -1469,6 +1468,14 @@ class Process : public std::enable_shared_from_this<Process>,
 
   virtual bool IsLiveDebugSession() const { return true; };
 
+
+  /// Provide a way to retrieve the core dump file that is loaded for debugging.
+  /// Only available if IsLiveDebugSession() returns true.
+  ///
+  /// \return
+  ///     File path to the core file.
+  virtual FileSpec GetCoreFile() const { return {}; }
+
   /// Before lldb detaches from a process, it warns the user that they are
   /// about to lose their debug session. In some cases, this warning doesn't
   /// need to be emitted -- for instance, with core file debugging where the
diff --git a/lldb/include/lldb/Target/ProcessTrace.h b/lldb/include/lldb/Target/ProcessTrace.h
index 037dea232cc024d..ee684a97a5be71c 100644
--- a/lldb/include/lldb/Target/ProcessTrace.h
+++ b/lldb/include/lldb/Target/ProcessTrace.h
@@ -27,7 +27,7 @@ class ProcessTrace : public PostMortemProcess {
 
   static llvm::StringRef GetPluginDescriptionStatic();
 
-  ProcessTrace(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp);
+  ProcessTrace(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp, lldb::FileSP core_file_sp);
 
   ~ProcessTrace() override;
 
@@ -74,7 +74,7 @@ class ProcessTrace : public PostMortemProcess {
 private:
   static lldb::ProcessSP CreateInstance(lldb::TargetSP target_sp,
                                         lldb::ListenerSP listener_sp,
-                                        const FileSpec *crash_file_path,
+                                        lldb::FileSP core_file_sp,
                                         bool can_connect);
 };
 
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index c37682e2a03859f..a84b8a30dd15c65 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -35,6 +35,7 @@
 #include "lldb/Utility/Broadcaster.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Timeout.h"
+#include "lldb/lldb-forward.h"
 #include "lldb/lldb-public.h"
 
 namespace lldb_private {
@@ -627,7 +628,7 @@ class Target : public std::enable_shared_from_this<Target>,
   // used.
   const lldb::ProcessSP &CreateProcess(lldb::ListenerSP listener_sp,
                                        llvm::StringRef plugin_name,
-                                       const FileSpec *crash_file,
+                                       lldb::FileSP crash_file,
                                        bool can_connect);
 
   const lldb::ProcessSP &GetProcessSP() const;
diff --git a/lldb/include/lldb/lldb-private-interfaces.h b/lldb/include/lldb/lldb-private-interfaces.h
index 53d5fbb84cc92d3..21fd34faee805d5 100644
--- a/lldb/include/lldb/lldb-private-interfaces.h
+++ b/lldb/include/lldb/lldb-private-interfaces.h
@@ -79,7 +79,8 @@ typedef lldb::PlatformSP (*PlatformCreateInstance)(bool force,
                                                    const ArchSpec *arch);
 typedef lldb::ProcessSP (*ProcessCreateInstance)(
     lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
-    const FileSpec *crash_file_path, bool can_connect);
+                                                 lldb::FileSP crash_file_sp,
+                                                 bool can_connect);
 typedef lldb::RegisterTypeBuilderSP (*RegisterTypeBuilderCreateInstance)(
     Target &target);
 typedef lldb::ScriptInterpreterSP (*ScriptInterpreterCreateInstance)(
diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp
index 2d029554492a05c..46326039e8c1bf4 100644
--- a/lldb/source/API/SBTarget.cpp
+++ b/lldb/source/API/SBTarget.cpp
@@ -16,6 +16,7 @@
 #include "lldb/API/SBEnvironment.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBExpressionOptions.h"
+#include "lldb/API/SBFile.h"
 #include "lldb/API/SBFileSpec.h"
 #include "lldb/API/SBListener.h"
 #include "lldb/API/SBModule.h"
@@ -244,9 +245,45 @@ SBProcess SBTarget::LoadCore(const char *core_file, lldb::SBError &error) {
   TargetSP target_sp(GetSP());
   if (target_sp) {
     FileSpec filespec(core_file);
-    FileSystem::Instance().Resolve(filespec);
+    auto file = FileSystem::Instance().Open(
+        filespec, lldb_private::File::eOpenOptionReadOnly);
+    if (!file) {
+      error.SetErrorStringWithFormat("Failed to open the core file: %s",
+                                     llvm::toString(file.takeError()).c_str());
+      return sb_process;
+    }
+    ProcessSP process_sp(
+        target_sp->CreateProcess(target_sp->GetDebugger().GetListener(), "",
+                                 std::move(file.get()), false));
+    if (process_sp) {
+      error.SetError(process_sp->LoadCore());
+      if (error.Success())
+        sb_process.SetSP(process_sp);
+    } else {
+      error.SetErrorString("Failed to create the process");
+    }
+  } else {
+    error.SetErrorString("SBTarget is invalid");
+  }
+  return sb_process;
+}
+
+SBProcess SBTarget::LoadCore(SBFile &file) {
+  LLDB_INSTRUMENT_VA(this, file);
+
+  lldb::SBError error; // Ignored
+  return LoadCore(file, error);
+}
+
+SBProcess SBTarget::LoadCore(SBFile &file, lldb::SBError &error) {
+  LLDB_INSTRUMENT_VA(this, file, error);
+
+  SBProcess sb_process;
+  TargetSP target_sp(GetSP());
+  if (target_sp) {
+    FileSP file_sp = file.GetFile();
     ProcessSP process_sp(target_sp->CreateProcess(
-        target_sp->GetDebugger().GetListener(), "", &filespec, false));
+        target_sp->GetDebugger().GetListener(), "", file_sp, false));
     if (process_sp) {
       error.SetError(process_sp->LoadCore());
       if (error.Success())
diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index 8f052d0a7b837e2..f86effdd2ecc389 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -427,8 +427,17 @@ class CommandObjectTargetCreate : public CommandObjectParsed {
         core_file_dir.SetDirectory(core_file.GetDirectory());
         target_sp->AppendExecutableSearchPaths(core_file_dir);
 
+        auto file = FileSystem::Instance().Open(
+            core_file, lldb_private::File::eOpenOptionReadOnly);
+        if (!file) {
+          result.AppendErrorWithFormatv(
+              "Failed to open the core file '{0}': '{1}.'\n",
+              core_file.GetPath(), llvm::toString(file.takeError()));
+        }
+
         ProcessSP process_sp(target_sp->CreateProcess(
-            GetDebugger().GetListener(), llvm::StringRef(), &core_file, false));
+            GetDebugger().GetListener(), llvm::StringRef(),
+            std::move(file.get()), false));
 
         if (process_sp) {
           // Seems weird that we Launch a core file, but that is what we
@@ -2074,7 +2083,7 @@ class CommandObjectTargetModulesDumpSymtab
             result.GetOutputStream().EOL();
             result.GetOutputStream().EOL();
           }
-          if (INTERRUPT_REQUESTED(GetDebugger(), 
+          if (INTERRUPT_REQUESTED(GetDebugger(),
                                   "Interrupted in dump all symtabs with {0} "
                                   "of {1} dumped.", num_dumped, num_modules))
             break;
@@ -2104,9 +2113,10 @@ class CommandObjectTargetModulesDumpSymtab
                 result.GetOutputStream().EOL();
                 result.GetOutputStream().EOL();
               }
-              if (INTERRUPT_REQUESTED(GetDebugger(), 
-                    "Interrupted in dump symtab list with {0} of {1} dumped.", 
-                    num_dumped, num_matches))
+              if (INTERRUPT_REQUESTED(
+                      GetDebugger(),
+                      "Interrupted in dump symtab list with {0} of {1} dumped.",
+                      num_dumped, num_matches))
                 break;
 
               num_dumped++;
@@ -2167,9 +2177,10 @@ class CommandObjectTargetModulesDumpSections
       result.GetOutputStream().Format("Dumping sections for {0} modules.\n",
                                       num_modules);
       for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
-        if (INTERRUPT_REQUESTED(GetDebugger(), 
-              "Interrupted in dump all sections with {0} of {1} dumped",
-              image_idx, num_modules))
+        if (INTERRUPT_REQUESTED(
+                GetDebugger(),
+                "Interrupted in dump all sections with {0} of {1} dumped",
+                image_idx, num_modules))
           break;
 
         num_dumped++;
@@ -2188,9 +2199,10 @@ class CommandObjectTargetModulesDumpSections
             FindModulesByName(target, arg_cstr, module_list, true);
         if (num_matches > 0) {
           for (size_t i = 0; i < num_matches; ++i) {
-            if (INTERRUPT_REQUESTED(GetDebugger(), 
-                  "Interrupted in dump section list with {0} of {1} dumped.",
-                  i, num_matches))
+            if (INTERRUPT_REQUESTED(
+                    GetDebugger(),
+                    "Interrupted in dump section list with {0} of {1} dumped.",
+                    i, num_matches))
               break;
 
             Module *module = module_list.GetModulePointerAtIndex(i);
@@ -2330,9 +2342,10 @@ class CommandObjectTargetModulesDumpClangAST
       }
 
       for (size_t i = 0; i < num_matches; ++i) {
-        if (INTERRUPT_REQUESTED(GetDebugger(), 
-              "Interrupted in dump clang ast list with {0} of {1} dumped.",
-              i, num_matches))
+        if (INTERRUPT_REQUESTED(
+                GetDebugger(),
+                "Interrupted in dump clang ast list with {0} of {1} dumped.", i,
+                num_matches))
           break;
 
         Module *m = module_list.GetModulePointerAtIndex(i);
@@ -2469,10 +2482,10 @@ class CommandObjectTargetModulesDumpLineTable
         if (num_modules > 0) {
           uint32_t num_dumped = 0;
           for (ModuleSP module_sp : target_modules.ModulesNoLocking()) {
-            if (INTERRUPT_REQUESTED(GetDebugger(), 
+            if (INTERRUPT_REQUESTED(GetDebugger(),
                                     "Interrupted in dump all line tables with "
-                                    "{0} of {1} dumped", num_dumped, 
-                                    num_modules))
+                                    "{0} of {1} dumped",
+                                    num_dumped, num_modules))
               break;
 
             if (DumpCompileUnitLineTable(
diff --git a/lldb/source/Core/IOHandlerCursesGUI.cpp b/lldb/source/Core/IOHandlerCursesGUI.cpp
index 22b8cc3582eae78..5b850fd5d5d7438 100644
--- a/lldb/source/Core/IOHandlerCursesGUI.cpp
+++ b/lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -3174,8 +3174,17 @@ class TargetCreateFormDelegate : public FormDelegate {
     core_file_directory_spec.SetDirectory(core_file_spec.GetDirectory());
     target_sp->AppendExecutableSearchPaths(core_file_directory_spec);
 
-    ProcessSP process_sp(target_sp->CreateProcess(
-        m_debugger.GetListener(), llvm::StringRef(), &core_file_spec, false));
+    auto core_file = FileSystem::Instance().Open(
+        core_file_spec, lldb_private::File::eOpenOptionReadOnly);
+
+    if (!core_file) {
+      SetError(llvm::toString(core_file.takeError()).c_str());
+      return;
+    }
+
+    ProcessSP process_sp(
+        target_sp->CreateProcess(m_debugger.GetListener(), llvm::StringRef(),
+                                 std::move(core_file.get()), false));
 
     if (!process_sp) {
       SetError("Unable to find process plug-in for core file!");
diff --git a/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp b/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
index 601f5df43dbba4e..c0c6cd8a9b2ee6b 100644
--- a/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
+++ b/lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp
@@ -32,7 +32,7 @@ namespace {
 class ProcessFreeBSDKernelFVC : public ProcessFreeBSDKernel {
 public:
   ProcessFreeBSDKernelFVC(lldb::TargetSP target_sp, lldb::ListenerSP listener,
-                          fvc_t *fvc);
+                          lldb::FileSP core_file_sp, fvc_t *fvc);
 
   ~ProcessFreeBSDKernelFVC();
 
@@ -50,7 +50,7 @@ class ProcessFreeBSDKernelFVC : public ProcessFreeBSDKernel {
 class ProcessFreeBSDKernelKVM : public ProcessFreeBSDKernel {
 public:
   ProcessFreeBSDKernelKVM(lldb::TargetSP target_sp, lldb::ListenerSP listener,
-                          kvm_t *fvc);
+                          lldb::FileSP core_file_sp, kvm_t *fvc);
 
   ~ProcessFreeBSDKernelKVM();
 
@@ -67,30 +67,33 @@ class ProcessFreeBSDKernelKVM : public ProcessFreeBSDKernel {
 } // namespace
 
 ProcessFreeBSDKernel::ProcessFreeBSDKernel(lldb::TargetSP target_sp,
-                                           ListenerSP listener_sp)
-    : PostMortemProcess(target_sp, listener_sp) {}
+                                           ListenerSP listener_sp,
+                                           lldb::FileSP crash_file_sp)
+    : PostMortemProcess(target_sp, listener_sp, crash_file_sp) {}
 
 lldb::ProcessSP ProcessFreeBSDKernel::CreateInstance(lldb::TargetSP target_sp,
                                                      ListenerSP listener_sp,
-                                                     const FileSpec *crash_file,
+                                                     lldb::FileSP crash_file_sp,
                                                      bool can_connect) {
   ModuleSP executable = target_sp->GetExecutableModule();
   if (crash_file && !can_connect && executable) {
+    const FileSpec file_spec;
+    crash_file_sp->GetFileSpec(const_cast<FileSpec &>(file_spec));
 #if LLDB_ENABLE_FBSDVMCORE
     fvc_t *fvc =
         fvc_open(executable->GetFileSpec().GetPath().c_str(),
-                 crash_file->GetPath().c_str(), nullptr, nullptr, nullptr);
+                 file_spec->GetPath().c_str(), nullptr, nullptr, nullptr);
     if (fvc)
-      return std::make_shared<ProcessFreeBSDKernelFVC>(target_sp, listener_sp,
+      return std::make_shared<ProcessFreeBSDKernelFVC>(target_sp, listener_sp, crash_file_sp
                                                        fvc);
 #endif
 
 #if defined(__FreeBSD__)
     kvm_t *kvm =
         kvm_open2(executable->GetFileSpec().GetPath().c_str(),
-                  crash_file->GetPath().c_str(), O_RDONLY, nullptr, nullptr);
+                  file_spec->GetPath().c_str(), O_RDONLY, nullptr, nullptr);
     if (kvm)
-      return std::make_shared<ProcessFreeBSDKernelKVM>(target_sp, listener_sp,
+      return std::make_shared<ProcessFreeBSDKernelKVM>(target_sp, listener_sp, crash_file_sp
                                                        kvm);
 #endif
   }
@@ -276,8 +279,9 @@ lldb::addr_t ProcessFreeBSDKernel::FindSymbol(const char *name) {
 
 ProcessFreeBSDKernelFVC::ProcessFreeBSDKernelFVC(lldb::TargetSP target_sp,
                                                  ListenerSP listener_sp,
+                                                 lldb::FileSP core_file_sp,
                                                  fvc_t *fvc)
-    : ProcessFreeBSDKernel(target_sp, listener_sp), m_fvc(fvc) {}
+    : ProcessFreeBSDKernel(target_sp, listener_sp, core_file_sp), m_fvc(fvc) {}
 
 ProcessFreeBSDKernelFVC::~ProcessFreeBSDKernelFVC() {
   if (m_fvc)
@@ -303,8 +307,9 @@ const...
[truncated]

JDevlieghere · 2023-11-09T05:16:00Z

lldb/include/lldb/API/SBTarget.h

@@ -184,6 +184,8 @@ class LLDB_API SBTarget {

  SBProcess LoadCore(const char *core_file);
  SBProcess LoadCore(const char *core_file, lldb::SBError &error);
+  SBProcess LoadCore(SBFile &file);


Did you add this for consistency? I don't think we should be adding an overload without error handling.

+1, please don't add a new API that gives no feedback about what may have gone wrong.

Just add the error to your function call:

SBProcess LoadCore(SBFile &file, lldb::SBError &error);

The first SBProcess LoadCore(const char *core_file); should be marked as deprecated since we want people to use SBProcess LoadCore(const char *core_file, lldb::SBError &error); so they get error information back. In our public API we never remove an older API, and that is why SBProcess LoadCore(const char *core_file); is still there.

JDevlieghere · 2023-11-09T05:18:07Z

lldb/include/lldb/Target/PostMortemProcess.h

+    const FileSpec file_spec;
+    m_core_file->GetFileSpec(const_cast<FileSpec &>(file_spec));
+    return file_spec;


I'm not sure I understand what's going on here. You're marking the local variable as const only to immediately const_cast it away in the line below? Can't you avoid all this like so:

FileSpec file_spec; m_core_file->GetFileSpec(file_spec);

or even better, why doesn't GetFileSpec return a FileSpec instead of an out parameter?

JDevlieghere · 2023-11-09T05:18:20Z

lldb/include/lldb/Target/Process.h

+  static constexpr int g_all_event_bits =
+      eBroadcastBitStateChanged | eBroadcastBitInterrupt | eBroadcastBitSTDOUT |
+      eBroadcastBitSTDERR | eBroadcastBitProfileData |
+      eBroadcastBitStructuredData;


Unrelated formatting change?

JDevlieghere · 2023-11-09T05:20:11Z

lldb/source/Commands/CommandObjectTarget.cpp

+              if (INTERRUPT_REQUESTED(
+                      GetDebugger(),
+                      "Interrupted in dump symtab list with {0} of {1} dumped.",
+                      num_dumped, num_matches))


Please upstage unrelated code changes. I suspect this is the result of our editor removing trailing whitespace (I do the same) and then clang-format considers this a modified line and formats it.

JDevlieghere

I left some comments inline.

github-actions · 2023-11-09T05:22:41Z

✅ With the latest revision this PR passed the Python code formatter.

github-actions · 2023-11-09T05:22:45Z

✅ With the latest revision this PR passed the C/C++ code formatter.

bulbazord · 2023-11-09T17:25:31Z

lldb/include/lldb/API/SBTarget.h

@@ -184,6 +184,8 @@ class LLDB_API SBTarget {

  SBProcess LoadCore(const char *core_file);
  SBProcess LoadCore(const char *core_file, lldb::SBError &error);
+  SBProcess LoadCore(SBFile &file);


+1, please don't add a new API that gives no feedback about what may have gone wrong.

bulbazord · 2023-11-09T17:26:33Z

lldb/include/lldb/Target/PostMortemProcess.h

+    const FileSpec file_spec;
+    m_core_file->GetFileSpec(const_cast<FileSpec &>(file_spec));
+    return file_spec;


bulbazord · 2023-11-09T17:26:56Z

lldb/include/lldb/Target/Process.h

+  /// A notification structure that can be used by clients to listen
+  /// for changes in a process's lifetime.
+  ///
+  /// \see RegisterNotificationCallbacks (const Notifications&) @see
+  /// UnregisterNotificationCallbacks (const Notifications&)


Unrelated formatting change

bulbazord · 2023-11-09T17:28:14Z

lldb/include/lldb/Target/Process.h

+  /// Provide a way to retrieve the core dump file that is loaded for debugging.
+  /// Only available if IsLiveDebugSession() returns true.


Shouldn't it only be available if IsLiveDebugSession is false? You're debugging a core dump which isn't really a "live debug session" since you're examining the corpse of a process, no?

bulbazord · 2023-11-09T17:31:44Z

lldb/source/API/SBTarget.cpp

+  } else {
+    error.SetErrorString("SBTarget is invalid");
+  }


If you flip the condition that checks target_sp for validity, you can save a level of indentation for readability (if that matters to you).

bulbazord · 2023-11-09T17:34:19Z

lldb/source/Plugins/Process/FreeBSDKernel/ProcessFreeBSDKernel.cpp

+    const FileSpec file_spec;
+    crash_file_sp->GetFileSpec(const_cast<FileSpec &>(file_spec));


Same here as before, why are you making a const file spec and then const casting to fill it in?

bulbazord · 2023-11-09T17:35:37Z

lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp

                                           bool can_connect) {
  lldb::ProcessSP process_sp;
-  if (crash_file_path == NULL)
+  if (crash_file_sp == NULL)


Since you're already here, you can drop the == NULL and make it a little nicer.

bulbazord · 2023-11-09T17:35:52Z

lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp

+    const FileSpec file_spec;
+    crash_file_sp->GetFileSpec(const_cast<FileSpec &>(file_spec));


bulbazord · 2023-11-09T17:36:25Z

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

  lldb::ProcessSP process_sp;
-  if (crash_file_path == nullptr)
+  if (crash_file_sp == nullptr)


You can also drop the == nullptr here too

bulbazord · 2023-11-09T17:36:40Z

lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp

+    const FileSpec file_spec;
+    crash_file_sp->GetFileSpec(const_cast<FileSpec &>(file_spec));


clayborg · 2023-11-15T19:08:17Z

lldb/include/lldb/Target/Target.h

  const lldb::ProcessSP &CreateProcess(lldb::ListenerSP listener_sp,
                                       llvm::StringRef plugin_name,
-                                       const FileSpec *crash_file,
+                                       lldb::FileSP crash_file,
                                       bool can_connect);


I think we can have two interfaces in Target.h:

the previous one that taks a "const FileSpec *"

the new one that takes a lldb::FileSP
We have a 4 locations that have to manually create a file the code:

auto file = FileSystem::Instance().Open( filespec, lldb_private::File::eOpenOptionReadOnly); ...

This will also help reduce the number of changes in this patch. Then the version that takes a "const FileSpec *" can do the FileSystem::Instance().Open(...) and just call the other overload that takes a lldb::FileSP

If we move the open file code into Target.cpp, it's hard to handle error. Now in these 4 locations we have slightly different ways to report the error when the file is failed to open.

if (!file) { result.AppendErrorWithFormatv( "Failed to open the core file '{0}': '{1}.'\n", core_file.GetPath(), llvm::toString(file.takeError())); }

Do we want to swallow the error instead?

clayborg · 2023-11-15T19:10:15Z

lldb/source/API/SBTarget.cpp

+    auto file = FileSystem::Instance().Open(
+        filespec, lldb_private::File::eOpenOptionReadOnly);
+    if (!file) {
+      error.SetErrorStringWithFormat("Failed to open the core file: %s",
+                                     llvm::toString(file.takeError()).c_str());
+      return sb_process;
+    }
+    ProcessSP process_sp(
+        target_sp->CreateProcess(target_sp->GetDebugger().GetListener(), "",
+                                 std::move(file.get()), false));
+    if (process_sp) {
+      error.SetError(process_sp->LoadCore());
+      if (error.Success())
+        sb_process.SetSP(process_sp);
+    } else {
+      error.SetErrorString("Failed to create the process");
+    }
+  } else {
+    error.SetErrorString("SBTarget is invalid");
+  }


If we leave the Target::CreateProcess overload as suggested above, this entire change goes away.

clayborg · 2023-11-15T19:11:03Z

lldb/source/Commands/CommandObjectTarget.cpp

+        auto file = FileSystem::Instance().Open(
+            core_file, lldb_private::File::eOpenOptionReadOnly);
+        if (!file) {
+          result.AppendErrorWithFormatv(
+              "Failed to open the core file '{0}': '{1}.'\n",
+              core_file.GetPath(), llvm::toString(file.takeError()));
+        }
+
        ProcessSP process_sp(target_sp->CreateProcess(
-            GetDebugger().GetListener(), llvm::StringRef(), &core_file, false));
+            GetDebugger().GetListener(), llvm::StringRef(),
+            std::move(file.get()), false));


All of this goes away if we leave the original Target::CreateProcess overload as mentioned above.

clayborg · 2023-11-15T19:11:19Z

lldb/source/Core/IOHandlerCursesGUI.cpp

+    auto core_file = FileSystem::Instance().Open(
+        core_file_spec, lldb_private::File::eOpenOptionReadOnly);
+
+    if (!core_file) {
+      SetError(llvm::toString(core_file.takeError()).c_str());
+      return;
+    }
+
+    ProcessSP process_sp(
+        target_sp->CreateProcess(m_debugger.GetListener(), llvm::StringRef(),
+                                 std::move(core_file.get()), false));


All of this goes away if we leave the original Target::CreateProcess overload as mentioned above.

clayborg · 2023-11-15T19:14:18Z

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

  lldb::ProcessSP process_sp;
-  if (crash_file_path == nullptr)
+  if (crash_file_sp)


The logic is inverted from what it used to be, this should be:

if (!crash_file_sp)

clayborg · 2023-11-15T19:21:54Z

lldb/source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp

                                           bool can_connect) {
  lldb::ProcessSP process_sp;
-  if (crash_file_path == NULL)
+  if (crash_file_sp)


The logic is inverted from what it used to be, this should be:

if (!crash_file_sp)

bulbazord

LGTM I think, but wait a little bit for Jonas and/or Greg to give it another pass.

bulbazord · 2023-11-17T04:21:59Z

lldb/include/lldb/API/SBTarget.h

@@ -184,6 +184,7 @@ class LLDB_API SBTarget {

  SBProcess LoadCore(const char *core_file);
  SBProcess LoadCore(const char *core_file, lldb::SBError &error);
+  SBProcess LoadCore(SBFile &file, lldb::SBError &error);


Actually, can the SBFile & be marked const? This shouldn't modify the SBFile if I'm reading this correctly.

JDevlieghere · 2023-11-17T18:19:44Z

lldb/source/API/SBTarget.cpp

+SBProcess SBTarget::LoadCore(const SBFile &file, lldb::SBError &error) {
+  LLDB_INSTRUMENT_VA(this, file, error);
+
+  SBProcess sb_process;
+  TargetSP target_sp(GetSP());
+  if (target_sp) {
+    FileSP file_sp = file.GetFile();
+    FileSpec filespec;
+    file_sp->GetFileSpec(filespec);
+    FileSystem::Instance().Resolve(filespec);
+    ProcessSP process_sp(target_sp->CreateProcess(
+        target_sp->GetDebugger().GetListener(), "", &filespec, false));
+    if (process_sp) {
+      error.SetError(process_sp->LoadCore());
+      if (error.Success())
+        sb_process.SetSP(process_sp);
+    } else {
+      error.SetErrorString("Failed to create the process");
+    }
+  } else {
+    error.SetErrorString("SBTarget is invalid");
+  }
+  return sb_process;
+}


This could be a lot simpler and easier to read with early returns, with the added advantage that the error message is closer to the condition it corresponds to. The function below is a good example of that.

clayborg · 2023-11-17T18:23:34Z

I am not sure how this actually works? We pass in a SBFile and then get the FileSpec from it and then somehow that allows us to open a file that doesn't actually exist on disk? Or does this work because the fd we had is actually backed by some file on disk? In that case we didn't even need this new API as we could have extracted the file path from the descriptor and just passed that into LLDB.

The story is that linux allows a tool to get run when a core file is created and it gets a integer file descriptor for the core file that is used to access the core file. Prior to this fix we would write the file to disk and then load it into the debugger. Now we want to use the "fd" to load the core file, so our SBFile must be able to extract the actual backing file and then we will need to re-open this file in lldb???

This reverts commit e2fb816. It breaks TestLinuxCore.py on lldb-*-windows. See buildbot below: https://lab.llvm.org/buildbot/#/builders/219/builds/7014

omjavaid · 2023-11-20T06:19:56Z

I have reverted this temporarily as it broke TestLinuxCore.py on lldb-*-windows. Kindly have a look at
https://lab.llvm.org/buildbot/#/builders/219/builds/7014

Add a new API in SBTarget to Load Core from a SBFile. This will enable a target to load core from a file descriptor. So that in coredumper, we don't need to write core file to disk, instead we can pass the input file descriptor to lldb directly. Test: ``` (lldb) script Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D. >>> file_object = open("/home/hyubo/210hda79ms32sr0h", "r") >>> fd=file_object.fileno() >>> file = lldb.SBFile(fd,'r', True) >>> error = lldb.SBError() >>> target = lldb.debugger.CreateTarget(None) >>> target.LoadCore(file,error) SBProcess: pid = 56415, state = stopped, threads = 1 ```

)" This reverts commit e2fb816. It breaks TestLinuxCore.py on lldb-*-windows. See buildbot below: https://lab.llvm.org/buildbot/#/builders/219/builds/7014

Add a new API in SBTarget to Load Core from a SBFile. This will enable a target to load core from a file descriptor. So that in coredumper, we don't need to write core file to disk, instead we can pass the input file descriptor to lldb directly. Test: ``` (lldb) script Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D. >>> file_object = open("/home/hyubo/210hda79ms32sr0h", "r") >>> fd=file_object.fileno() >>> file = lldb.SBFile(fd,'r', True) >>> error = lldb.SBError() >>> target = lldb.debugger.CreateTarget(None) >>> target.LoadCore(file,error) SBProcess: pid = 56415, state = stopped, threads = 1 ```

)" This reverts commit e2fb816. It breaks TestLinuxCore.py on lldb-*-windows. See buildbot below: https://lab.llvm.org/buildbot/#/builders/219/builds/7014

GeorgeHuyubo requested a review from JDevlieghere as a code owner November 9, 2023 05:13

llvmbot added the lldb label Nov 9, 2023

GeorgeHuyubo requested review from clayborg and jeffreytan81 November 9, 2023 05:14

JDevlieghere reviewed Nov 9, 2023

View reviewed changes

JDevlieghere requested changes Nov 9, 2023

View reviewed changes

bulbazord reviewed Nov 9, 2023

View reviewed changes

GeorgeHuyubo requested review from JDevlieghere and bulbazord November 15, 2023 05:48

clayborg requested changes Nov 15, 2023

View reviewed changes

GeorgeHuyubo requested a review from clayborg November 16, 2023 22:17

bulbazord approved these changes Nov 17, 2023

View reviewed changes

bulbazord reviewed Nov 17, 2023

View reviewed changes

Add new API in SBTarget for loading core from SBFile

749453a

GeorgeHuyubo merged commit e2fb816 into llvm:main Nov 17, 2023
2 checks passed

GeorgeHuyubo deleted the new-load-core branch November 17, 2023 17:53

JDevlieghere reviewed Nov 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new API in SBTarget for loading core from SBFile #71769

Add new API in SBTarget for loading core from SBFile #71769

GeorgeHuyubo commented Nov 9, 2023

llvmbot commented Nov 9, 2023

JDevlieghere Nov 9, 2023

bulbazord Nov 9, 2023

clayborg Nov 11, 2023

JDevlieghere Nov 9, 2023

bulbazord Nov 9, 2023

JDevlieghere Nov 9, 2023

JDevlieghere Nov 9, 2023

JDevlieghere left a comment

github-actions bot commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

bulbazord Nov 9, 2023

bulbazord Nov 9, 2023

bulbazord Nov 9, 2023

bulbazord Nov 9, 2023

bulbazord Nov 9, 2023

bulbazord Nov 9, 2023

bulbazord Nov 9, 2023

bulbazord Nov 9, 2023

bulbazord Nov 9, 2023

bulbazord Nov 9, 2023

clayborg Nov 15, 2023

GeorgeHuyubo Nov 16, 2023 •

edited

Loading

clayborg Nov 15, 2023

clayborg Nov 15, 2023

clayborg Nov 15, 2023

clayborg Nov 15, 2023

clayborg Nov 15, 2023

bulbazord left a comment

bulbazord Nov 17, 2023

JDevlieghere Nov 17, 2023

clayborg commented Nov 17, 2023

omjavaid commented Nov 20, 2023

		/// Provide a way to retrieve the core dump file that is loaded for debugging.
		/// Only available if IsLiveDebugSession() returns true.

		const FileSpec file_spec;
		crash_file_sp->GetFileSpec(const_cast<FileSpec &>(file_spec));

Add new API in SBTarget for loading core from SBFile #71769

Add new API in SBTarget for loading core from SBFile #71769

Conversation

GeorgeHuyubo commented Nov 9, 2023

llvmbot commented Nov 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JDevlieghere left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 9, 2023 • edited Loading

github-actions bot commented Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgeHuyubo Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bulbazord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clayborg commented Nov 17, 2023

omjavaid commented Nov 20, 2023

github-actions bot commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading

GeorgeHuyubo Nov 16, 2023 •

edited

Loading