This is an archive of the discontinued LLVM Phabricator instance.

Pass plugin_name in SBProcess::SaveCore
ClosedPublic

Authored by PatriosTheGreat on May 10 2022, 9:24 AM.

Details

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.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 9:24 AM
PatriosTheGreat requested review of this revision.May 10 2022, 9:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 9:24 AM
clayborg requested changes to this revision.May 10 2022, 1:16 PM

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.

This revision now requires changes to proceed.May 10 2022, 1:16 PM
PatriosTheGreat edited the summary of this revision. (Show Details)
PatriosTheGreat marked 3 inline comments as done.May 11 2022, 12:50 AM

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.

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 added inline comments.May 11 2022, 3:04 PM
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 marked 3 inline comments as done.

Thanks for clarifications.
Fixed header doc and parameter name.

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 requested changes to this revision.May 13 2022, 1:36 PM

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.

This revision now requires changes to proceed.May 13 2022, 1:36 PM
PatriosTheGreat marked 5 inline comments as done.

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

clayborg accepted this revision.May 18 2022, 2:15 PM

Thanks for the changes. LGTM

This revision is now accepted and ready to land.May 18 2022, 2:15 PM

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

This revision was automatically updated to reflect the committed changes.
thieta added a subscriber: thieta.Jun 9 2022, 7:31 AM

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