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

344–345

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–1147

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

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

46–47 ↗

(On Diff #428401)

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

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 ↗

(On Diff #429160)

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

47 ↗

(On Diff #429160)

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 *plugin_name);
+
+    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).
+  lldb::SBError SaveCore(const char *file_name, const char *plugin_name);
+
+  // Save the state of the process with the default plugin.
   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
   ///     A reference to an SBMemoryRegionInfo object that will contain
   ///     the details of the memory region containing load_addr.
   ///

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, "");
+}
+
+lldb::SBError SBProcess::SaveCore(const char *file_name,
+                                  const char *plugin_name) {
+  LLDB_INSTRUMENT_VA(this, file_name, plugin_name);
 
   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,
+                                        plugin_name);
   return error;
 }
 
 lldb::SBError
 SBProcess::GetMemoryRegionInfo(lldb::addr_t load_addr,
                                SBMemoryRegionInfo &sb_region_info) {
   LLDB_INSTRUMENT_VA(this, load_addr, sb_region_info);