Pass plugin_name in SBProcess::SaveCore

[PatriosTheGreat]

Summary

This CL allows to use minidump save-core functionality (https://reviews.llvm.org/D108233) via SBProcess interface.
After adding a support from gdb-remote client (https://reviews.llvm.org/D101329) if the plugin name is empty the plugin manager will try to save the core directly from the process plugin.
See https://github.com/llvm/llvm-project/blob/main/lldb/source/Core/PluginManager.cpp#L696

To have an ability to save the core with minidump plugin I added plugin name as a parameter in SBProcess::SaveCore.

[clayborg]

We can't change any public API calls since other tools might link against the existing API, but we can add new variants to the API. Inline fixes have been suggested.

lldb/bindings/interface/SBProcess.i

403–404

We can't change public API, but we can add variants.

lldb/include/lldb/API/SBProcess.h

356–357

We have a public API, we cannot change any existing functions in the API, but we are free too add new variants.

lldb/source/API/SBProcess.cpp

1140–1148

Can't change public API, so just. add a new variant.

lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py

46–47

revert this as we should be able to still save a core file without specifying the plug-in name.

[PatriosTheGreat]

Hi Greg.

Thanks for the review.
I fixed the feedback.
I forgot the default initialization of plugin_name parameter in SBProcess.i in previous version, but I assume it's still better to explicitly create a separate method.

[jingham]

In D125325#3505305, @PatriosTheGreat wrote:

Hi Greg.

Thanks for the review.
I fixed the feedback.
I forgot the default initialization of plugin_name parameter in SBProcess.i in previous version, but I assume it's still better to explicitly create a separate method.

Yes, a method with a default parameter has a different mangled name from the method w/o the parameter, so it's still a binary incompatible change.

[clayborg]

lldb/include/lldb/API/SBProcess.h

340–355

I would modify the header doc to be a bit more descriptive. Also, the user doesn't need to know about the plug-in name, we should call it something else like maybe "flavor"? Right now we can save "mach-o" (from ObjectFileMachO) or "minidump" (from ObjectFileMinidump. We don't have ELF core file support yet in ObjectFileELF. So the users should be able to pick from a list.

341

343

[PatriosTheGreat]

Thanks for clarifications.
Fixed header doc and parameter name.

[clayborg]

It might be a good idea to add a test for this. There seems to be a test already in this file:

lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py

In fact looking at this test, it seems they specify the "SaveCoreStyle" via an option. As long as we are adding new APIs to SBProcess, we should probably add the "SaveCoreStyle style" as a parameter to the new SaveCore, this enum is already in the public header file (lldb-enumerations.h).

[clayborg]

Looking great, just a few more fixes. Thanks for making the changes.

lldb/include/lldb/API/SBProcess.h

348

Might not need to mention this in the header file as long as the SBError mentions that it isn't supported in the error message.

lldb/source/API/SBProcess.cpp

1147

missing core_style in the LLDB_INSTRUMENT_VA macro

lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py

47

The 3rd argument is being passed by string value which isn't correct. We need to use the enumeration.

47

We might think about adding a test for "minidump" to save it with a style of "lldb.eSaveCoreFull" and "lldb.eSaveCoreDirtyOnly" and verify we get an error back. If this ever changes, then we can modify the test.

[PatriosTheGreat]

Hi Greg,
Sorry for a delay had an issue with tests local run.
I fixed the previous feedback.

[clayborg]

Thanks for the changes. LGTM

[PatriosTheGreat]

Thanks Greg,
Can you or someone please take this commit to main branch since I don't have a commit permission?

[thieta]

FYI: I helped @PatriosTheGreat to commit this after he asked for help in discord.

lldb/bindings/interface/SBProcess.i

public:
 
     lldb::SBThreadCollection
     GetHistoryThreads (addr_t addr);
 
     bool
     IsInstrumentationRuntimePresent(lldb::InstrumentationRuntimeType type);
 
     lldb::SBError
+    SaveCore(const char *file_name, const char *flavor, lldb::SaveCoreStyle core_style);
+
+    lldb::SBError
     SaveCore(const char *file_name);
 
     lldb::SBError
     GetMemoryRegionInfo(lldb::addr_t load_addr, lldb::SBMemoryRegionInfo &region_info);
 
     lldb::SBMemoryRegionInfoList
     GetMemoryRegions();
 
     %feature("autodoc", "

lldb/include/lldb/API/SBProcess.h

public:
   /// \return
   ///   The name at that index.
   const char *GetExtendedBacktraceTypeAtIndex(uint32_t idx);
 
   lldb::SBThreadCollection GetHistoryThreads(addr_t addr);
 
   bool IsInstrumentationRuntimePresent(InstrumentationRuntimeType type);
 
-  /// Save the state of the process in a core file (or mini dump on Windows).
+  /// Save the state of the process in a core file.
+  ///
+  /// \param[in] file_name - The name of the file to save the core file to.
+  ///
+  /// \param[in] flavor - Specify the flavor of a core file plug-in to save.
+  /// Currently supported flavors include "mach-o" and "minidump"
+  ///
+  /// \param[in] core_style - Specify the style of a core file to save.
+  /// "minidump" plug-in supports only "eSaveCoreStackOnly".
+  lldb::SBError SaveCore(const char *file_name, const char *flavor,
+                         SaveCoreStyle core_style);
+
+  /// Save the state of the process with the a flavor that matches the
+  /// current process' main executable (if supported).
+  ///
+  /// \param[in] file_name - The name of the file to save the core file to.
   lldb::SBError SaveCore(const char *file_name);
 
   /// Query the address load_addr and store the details of the memory
   /// region that contains it in the supplied SBMemoryRegionInfo object.
   /// To iterate over all memory regions use GetMemoryRegionList.
   ///
   /// \param[in] load_addr
   ///     The address to be queried.
   ///
   /// \param[out] region_info

lldb/source/API/SBProcess.cpp

bool SBProcess::IsInstrumentationRuntimePresent(
 
   if (!runtime_sp.get())
     return false;
 
   return runtime_sp->IsActive();
 }
 
 lldb::SBError SBProcess::SaveCore(const char *file_name) {
   LLDB_INSTRUMENT_VA(this, file_name);
+  return SaveCore(file_name, "", SaveCoreStyle::eSaveCoreFull);
+}
+
+lldb::SBError SBProcess::SaveCore(const char *file_name,
+                                  const char *flavor,
+                                  SaveCoreStyle core_style) {
+  LLDB_INSTRUMENT_VA(this, file_name, flavor);
 
   lldb::SBError error;
   ProcessSP process_sp(GetSP());
   if (!process_sp) {
     error.SetErrorString("SBProcess is invalid");
     return error;
   }
 
   std::lock_guard<std::recursive_mutex> guard(
       process_sp->GetTarget().GetAPIMutex());
 
   if (process_sp->GetState() != eStateStopped) {
     error.SetErrorString("the process is not stopped");
     return error;
   }
 
   FileSpec core_file(file_name);
-  SaveCoreStyle core_style = SaveCoreStyle::eSaveCoreFull;
-  error.ref() = PluginManager::SaveCore(process_sp, core_file, core_style, "");
+  error.ref() = PluginManager::SaveCore(process_sp, core_file, core_style,
+                                        flavor);
   return error;
 }
 
 lldb::SBError
 SBProcess::GetMemoryRegionInfo(lldb::addr_t load_addr,
                                SBMemoryRegionInfo &sb_region_info) {
   LLDB_INSTRUMENT_VA(this, load_addr, sb_region_info);
 

lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py

class ProcessSaveCoreMinidumpTestCase(TestBase):
 
     @skipUnlessArch("x86_64")
     @skipUnlessPlatform(["linux"])
     def test_save_linux_mini_dump(self):
         """Test that we can save a Linux mini dump."""
         self.build()
         exe = self.getBuildArtifact("a.out")
         core = self.getBuildArtifact("core.dmp")
+        core_sb = self.getBuildArtifact("core_sb.dmp")
         try:
             target = self.dbg.CreateTarget(exe)
             process = target.LaunchSimple(
                 None, None, self.get_process_working_directory())
             self.assertEqual(process.GetState(), lldb.eStateStopped)
 
             # get neccessary data for the verification phase
             process_info = process.GetProcessInfo()
             expected_pid = process_info.GetProcessID() if process_info.IsValid() else -1
             expected_number_of_modules = target.GetNumModules()
             expected_modules = target.modules
             expected_number_of_threads = process.GetNumThreads()
             expected_threads = []
 
             for thread_idx in range(process.GetNumThreads()):
                 thread = process.GetThreadAtIndex(thread_idx)
                 thread_id = thread.GetThreadID()
                 expected_threads.append(thread_id)
 
             # save core and, kill process and verify corefile existence
             self.runCmd("process save-core --plugin-name=minidump --style=stack " + core)
+            # validate savinig via SBProcess
+            lldb.SBProcess().SaveCore(core_sb, "minidump", "stack")
             self.assertTrue(os.path.isfile(core))
+            self.assertTrue(os.path.isfile(core_sb))
             self.assertSuccess(process.Kill())
 
             # To verify, we'll launch with the mini dump
             target = self.dbg.CreateTarget(None)
             process = target.LoadCore(core)
 
             # check if the core is in desired state
             self.assertTrue(process, PROCESS_IS_VALID)
def test_save_linux_mini_dump(self):
                 self.assertTrue(thread.IsValid())
                 thread_id = thread.GetThreadID()
                 self.assertTrue(thread_id in expected_threads)
         finally:
             # Clean up the mini dump file.
             self.assertTrue(self.dbg.DeleteTarget(target))
             if (os.path.isfile(core)):
                 os.unlink(core)
+            if (os.path.isfile(core_sb)):
+                os.unlink(core_sb)