https://github.com/Jlalond updated https://github.com/llvm/llvm-project/pull/98403
>From 4752adac6b8d39512bbfb46726205ceb2301d1c2 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Tue, 9 Jul 2024 13:30:46 -0700 Subject: [PATCH 1/6] Create CoreDumpOption class, and SB equivalent --- lldb/include/lldb/API/SBCoreDumpOptions.h | 37 +++++++++++++++++++ lldb/source/API/SBCoreDumpOptions.cpp | 16 ++++++++ .../Plugins/ObjectFile/CoreDumpOptions.cpp | 25 +++++++++++++ .../Plugins/ObjectFile/CoreDumpOptions.h | 37 +++++++++++++++++++ 4 files changed, 115 insertions(+) create mode 100644 lldb/include/lldb/API/SBCoreDumpOptions.h create mode 100644 lldb/source/API/SBCoreDumpOptions.cpp create mode 100644 lldb/source/Plugins/ObjectFile/CoreDumpOptions.cpp create mode 100644 lldb/source/Plugins/ObjectFile/CoreDumpOptions.h diff --git a/lldb/include/lldb/API/SBCoreDumpOptions.h b/lldb/include/lldb/API/SBCoreDumpOptions.h new file mode 100644 index 0000000000000..523fa34c17f36 --- /dev/null +++ b/lldb/include/lldb/API/SBCoreDumpOptions.h @@ -0,0 +1,37 @@ +//===-- SBCoreDumpOptions.h -------------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLDB_API_SBCOREDUMPOPTIONS_H +#define LLDB_API_SBCOREDUMPOPTIONS_H + +namespace lldb { + +class LLDB_API SBCoreDumpOptions { +public: + SBCoreDumpOptions() {}; + SBStatisticsOptions(const lldb::SBCoreDumpOptions &rhs); + ~SBExpressionOptions() = default; + + const SBStatisticsOptions &operator=(const lldb::SBStatisticsOptions &rhs); + + void SetCoreDumpPluginName(const char* plugin); + const char* GetCoreDumpPluginName(); + + void SetCoreDumpStyle(const char* style); + const char* GetCoreDumpStyle(); + + void SetOutputFilePath(const char* path); + const char* GetOutputFilePath(); + +private: + std::unique_ptr<lldb_private::SBCoreDumpOptions> m_opaque_up; +}; // SBCoreDumpOptions + +}; // namespace lldb + +#endif // LLDB_API_SBCOREDUMPOPTIONS_H diff --git a/lldb/source/API/SBCoreDumpOptions.cpp b/lldb/source/API/SBCoreDumpOptions.cpp new file mode 100644 index 0000000000000..d051c5b6bef6b --- /dev/null +++ b/lldb/source/API/SBCoreDumpOptions.cpp @@ -0,0 +1,16 @@ +//===-- SBCoreDumpOptions.cpp -----------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "lldb/API/SBCoreDumpOptions.h" + +using namespace lldb; +using namespace lldb_private; + +void SBCoreDumpOptions::SetCoreDumpPluginName(const char *name) { + +}; diff --git a/lldb/source/Plugins/ObjectFile/CoreDumpOptions.cpp b/lldb/source/Plugins/ObjectFile/CoreDumpOptions.cpp new file mode 100644 index 0000000000000..00333d1550790 --- /dev/null +++ b/lldb/source/Plugins/ObjectFile/CoreDumpOptions.cpp @@ -0,0 +1,25 @@ +//===-- CoreDumpOptions.cpp -------------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "lldb/API/CoreDumpOptions.h" + +CoreDumpOptions::SetCoreDumpPluginName(const char *name) { + m_core_dump_plugin_name = name; +} + +CoreDumpOptions::GetCoreDumpPluginName() { return m_core_dump_plugin_name; } + +CoreDumpOptions::SetCoreDumpStyle(lldb::SaveCoreStyle style) { + m_core_dump_style = style; +} + +CoreDumpOptions::GetCoreDumpStyle() { return m_core_dump_style; } + +CoreDumpOptions::SetCoreDumpFile(const char *file) { m_core_dump_file = file; } + +CoreDumpOptions::GetCoreDumpFile() { return m_core_dump_file; } diff --git a/lldb/source/Plugins/ObjectFile/CoreDumpOptions.h b/lldb/source/Plugins/ObjectFile/CoreDumpOptions.h new file mode 100644 index 0000000000000..fe369b0490674 --- /dev/null +++ b/lldb/source/Plugins/ObjectFile/CoreDumpOptions.h @@ -0,0 +1,37 @@ +//===-- CoreDumpOptions.h ---------------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLDB_SOURCE_PLUGINS_OBJECTFILE_COREDUMPOPTIONS_H +#define LLDB_SOURCE_PLUGINS_OBJECTFILE_COREDUMPOPTIONS_H + +#include <string> + +using namespace lldb; + +class CoreDumpOptions { + public: + CoreDumpOptions() {}; + ~CoreDumpOptions() = default; + + + void SetCoreDumpPluginName(const char* name); + const char* GetCoreDumpPluginName() const; + + void SetCoreDumpStyle(lldb::SaveCoreStyle style); + lldb::SaveCoreStyle GetCoreDumpStyle() const; + + void SetOutputFilePath(const char* path); + const char* GetOutputFilePath() const; + +private: + std::string m_core_dump_plugin_name; + std::string m_output_file_path; + lldb::SaveCoreStyle m_core_dump_style = lldb::eSaveCoreStyleNone; +}; + +#endif // LLDB_SOURCE_PLUGINS_OBJECTFILE_COREDUMPOPTIONS_H >From 3acae92ae678092064164e8e492c4789132b81cc Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 10 Jul 2024 15:07:41 -0700 Subject: [PATCH 2/6] Finish all the plumbing and successfully migrate TestProcessSaveCoreMinidump to the new SBCoreDumpOptions API --- lldb/bindings/headers.swig | 1 + .../interface/SBCoreDumpOptionsDocstrings.i | 0 lldb/bindings/interfaces.swig | 2 + lldb/include/lldb/API/LLDB.h | 1 + lldb/include/lldb/API/SBCoreDumpOptions.h | 29 +++++----- lldb/include/lldb/API/SBDefines.h | 1 + lldb/include/lldb/API/SBProcess.h | 6 +++ lldb/include/lldb/Core/PluginManager.h | 4 +- .../lldb/Symbol}/CoreDumpOptions.h | 24 +++++---- lldb/include/lldb/lldb-private-interfaces.h | 4 +- lldb/source/API/CMakeLists.txt | 1 + lldb/source/API/SBCoreDumpOptions.cpp | 53 ++++++++++++++++++- lldb/source/API/SBProcess.cpp | 19 +++++-- lldb/source/Commands/CommandObjectProcess.cpp | 6 ++- lldb/source/Core/PluginManager.cpp | 22 ++++---- .../Plugins/ObjectFile/CoreDumpOptions.cpp | 25 --------- .../ObjectFile/Mach-O/ObjectFileMachO.cpp | 8 +-- .../ObjectFile/Mach-O/ObjectFileMachO.h | 3 +- .../Minidump/ObjectFileMinidump.cpp | 11 ++-- .../ObjectFile/Minidump/ObjectFileMinidump.h | 3 +- .../ObjectFile/PECOFF/ObjectFilePECOFF.cpp | 7 ++- .../ObjectFile/PECOFF/ObjectFilePECOFF.h | 3 +- .../ObjectFile/PECOFF/WindowsMiniDump.cpp | 3 +- .../ObjectFile/PECOFF/WindowsMiniDump.h | 2 +- lldb/source/Symbol/CMakeLists.txt | 1 + lldb/source/Symbol/CoreDumpOptions.cpp | 35 ++++++++++++ .../postmortem/minidump/TestMiniDump.py | 1 + .../process_save_core/TestProcessSaveCore.py | 1 + .../TestProcessSaveCoreMinidump.py | 15 ++++-- 29 files changed, 198 insertions(+), 93 deletions(-) create mode 100644 lldb/bindings/interface/SBCoreDumpOptionsDocstrings.i rename lldb/{source/Plugins/ObjectFile => include/lldb/Symbol}/CoreDumpOptions.h (54%) delete mode 100644 lldb/source/Plugins/ObjectFile/CoreDumpOptions.cpp create mode 100644 lldb/source/Symbol/CoreDumpOptions.cpp diff --git a/lldb/bindings/headers.swig b/lldb/bindings/headers.swig index c91504604b6ac..d65472648ec59 100644 --- a/lldb/bindings/headers.swig +++ b/lldb/bindings/headers.swig @@ -21,6 +21,7 @@ #include "lldb/API/SBCommandReturnObject.h" #include "lldb/API/SBCommunication.h" #include "lldb/API/SBCompileUnit.h" +#include "lldb/API/SBCoreDumpOptions.h" #include "lldb/API/SBData.h" #include "lldb/API/SBDebugger.h" #include "lldb/API/SBDeclaration.h" diff --git a/lldb/bindings/interface/SBCoreDumpOptionsDocstrings.i b/lldb/bindings/interface/SBCoreDumpOptionsDocstrings.i new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/lldb/bindings/interfaces.swig b/lldb/bindings/interfaces.swig index 0953f4c72a910..d25c25dbfc2af 100644 --- a/lldb/bindings/interfaces.swig +++ b/lldb/bindings/interfaces.swig @@ -25,6 +25,7 @@ %include "./interface/SBCommandReturnObjectDocstrings.i" %include "./interface/SBCommunicationDocstrings.i" %include "./interface/SBCompileUnitDocstrings.i" +%include "./interface/SBCoreDumpOptionsDocstrings.i" %include "./interface/SBDataDocstrings.i" %include "./interface/SBDebuggerDocstrings.i" %include "./interface/SBDeclarationDocstrings.i" @@ -101,6 +102,7 @@ %include "lldb/API/SBCommandReturnObject.h" %include "lldb/API/SBCommunication.h" %include "lldb/API/SBCompileUnit.h" +%include "lldb/API/SBCoreDumpOptions.h" %include "lldb/API/SBData.h" %include "lldb/API/SBDebugger.h" %include "lldb/API/SBDeclaration.h" diff --git a/lldb/include/lldb/API/LLDB.h b/lldb/include/lldb/API/LLDB.h index d8cc9f5067fe9..74817ac99ed2b 100644 --- a/lldb/include/lldb/API/LLDB.h +++ b/lldb/include/lldb/API/LLDB.h @@ -23,6 +23,7 @@ #include "lldb/API/SBCommandReturnObject.h" #include "lldb/API/SBCommunication.h" #include "lldb/API/SBCompileUnit.h" +#include "lldb/API/SBCoreDumpOptions.h" #include "lldb/API/SBData.h" #include "lldb/API/SBDebugger.h" #include "lldb/API/SBDeclaration.h" diff --git a/lldb/include/lldb/API/SBCoreDumpOptions.h b/lldb/include/lldb/API/SBCoreDumpOptions.h index 523fa34c17f36..b3e81ceee852a 100644 --- a/lldb/include/lldb/API/SBCoreDumpOptions.h +++ b/lldb/include/lldb/API/SBCoreDumpOptions.h @@ -9,29 +9,34 @@ #ifndef LLDB_API_SBCOREDUMPOPTIONS_H #define LLDB_API_SBCOREDUMPOPTIONS_H +#include "lldb/API/SBDefines.h" +#include "lldb/Symbol/CoreDumpOptions.h" + namespace lldb { class LLDB_API SBCoreDumpOptions { public: - SBCoreDumpOptions() {}; - SBStatisticsOptions(const lldb::SBCoreDumpOptions &rhs); - ~SBExpressionOptions() = default; + SBCoreDumpOptions(const char* filePath); + SBCoreDumpOptions(const lldb::SBCoreDumpOptions &rhs); + ~SBCoreDumpOptions() = default; - const SBStatisticsOptions &operator=(const lldb::SBStatisticsOptions &rhs); + const SBCoreDumpOptions &operator=(const lldb::SBCoreDumpOptions &rhs); void SetCoreDumpPluginName(const char* plugin); - const char* GetCoreDumpPluginName(); + const std::optional<const char *> GetCoreDumpPluginName() const; + + void SetCoreDumpStyle(lldb::SaveCoreStyle style); + const std::optional<lldb::SaveCoreStyle> GetCoreDumpStyle() const; - void SetCoreDumpStyle(const char* style); - const char* GetCoreDumpStyle(); + const char * GetOutputFile() const; - void SetOutputFilePath(const char* path); - const char* GetOutputFilePath(); +protected: + friend class SBProcess; + lldb_private::CoreDumpOptions &Ref() const; private: - std::unique_ptr<lldb_private::SBCoreDumpOptions> m_opaque_up; + std::unique_ptr<lldb_private::CoreDumpOptions> m_opaque_up; }; // SBCoreDumpOptions - -}; // namespace lldb +} // namespace lldb #endif // LLDB_API_SBCOREDUMPOPTIONS_H diff --git a/lldb/include/lldb/API/SBDefines.h b/lldb/include/lldb/API/SBDefines.h index 87c0a1c3661ca..eb9c59169eaed 100644 --- a/lldb/include/lldb/API/SBDefines.h +++ b/lldb/include/lldb/API/SBDefines.h @@ -61,6 +61,7 @@ class LLDB_API SBCommandPluginInterface; class LLDB_API SBCommandReturnObject; class LLDB_API SBCommunication; class LLDB_API SBCompileUnit; +class LLDB_API SBCoreDumpOptions; class LLDB_API SBData; class LLDB_API SBDebugger; class LLDB_API SBDeclaration; diff --git a/lldb/include/lldb/API/SBProcess.h b/lldb/include/lldb/API/SBProcess.h index a6ab7ae759918..913aa7992a4fd 100644 --- a/lldb/include/lldb/API/SBProcess.h +++ b/lldb/include/lldb/API/SBProcess.h @@ -378,6 +378,12 @@ class LLDB_API SBProcess { /// \param[in] file_name - The name of the file to save the core file to. lldb::SBError SaveCore(const char *file_name); + /// Save the state of the process with the desired settings + /// as defined in the options object. + /// + /// \param[in] options - The options to use when saving the core file. + lldb::SBError SaveCore(SBCoreDumpOptions &options); + /// 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. diff --git a/lldb/include/lldb/Core/PluginManager.h b/lldb/include/lldb/Core/PluginManager.h index f2296e2920238..dcc3a8a062265 100644 --- a/lldb/include/lldb/Core/PluginManager.h +++ b/lldb/include/lldb/Core/PluginManager.h @@ -191,9 +191,7 @@ class PluginManager { GetObjectFileCreateMemoryCallbackForPluginName(llvm::StringRef name); static Status SaveCore(const lldb::ProcessSP &process_sp, - const FileSpec &outfile, - lldb::SaveCoreStyle &core_style, - llvm::StringRef plugin_name); + lldb_private::CoreDumpOptions &core_options); // ObjectContainer static bool RegisterPlugin( diff --git a/lldb/source/Plugins/ObjectFile/CoreDumpOptions.h b/lldb/include/lldb/Symbol/CoreDumpOptions.h similarity index 54% rename from lldb/source/Plugins/ObjectFile/CoreDumpOptions.h rename to lldb/include/lldb/Symbol/CoreDumpOptions.h index fe369b0490674..435031a510d80 100644 --- a/lldb/source/Plugins/ObjectFile/CoreDumpOptions.h +++ b/lldb/include/lldb/Symbol/CoreDumpOptions.h @@ -9,29 +9,35 @@ #ifndef LLDB_SOURCE_PLUGINS_OBJECTFILE_COREDUMPOPTIONS_H #define LLDB_SOURCE_PLUGINS_OBJECTFILE_COREDUMPOPTIONS_H +#include "lldb/Utility/FileSpec.h" +#include "lldb/lldb-forward.h" +#include "lldb/lldb-types.h" + +#include <optional> #include <string> -using namespace lldb; +namespace lldb_private { class CoreDumpOptions { public: - CoreDumpOptions() {}; + CoreDumpOptions(const lldb_private::FileSpec &fspec) : + m_core_dump_file(std::move(fspec)) {}; ~CoreDumpOptions() = default; - void SetCoreDumpPluginName(const char* name); - const char* GetCoreDumpPluginName() const; + void SetCoreDumpPluginName(llvm::StringRef name); + std::optional<llvm::StringRef> GetCoreDumpPluginName() const; void SetCoreDumpStyle(lldb::SaveCoreStyle style); lldb::SaveCoreStyle GetCoreDumpStyle() const; - void SetOutputFilePath(const char* path); - const char* GetOutputFilePath() const; + const lldb_private::FileSpec& GetOutputFile() const; private: - std::string m_core_dump_plugin_name; - std::string m_output_file_path; - lldb::SaveCoreStyle m_core_dump_style = lldb::eSaveCoreStyleNone; + std::optional<std::string> m_core_dump_plugin_name; + const lldb_private::FileSpec m_core_dump_file; + lldb::SaveCoreStyle m_core_dump_style = lldb::eSaveCoreUnspecified; }; +} // namespace lldb_private #endif // LLDB_SOURCE_PLUGINS_OBJECTFILE_COREDUMPOPTIONS_H diff --git a/lldb/include/lldb/lldb-private-interfaces.h b/lldb/include/lldb/lldb-private-interfaces.h index cdd9b51d9329c..0948a0482b201 100644 --- a/lldb/include/lldb/lldb-private-interfaces.h +++ b/lldb/include/lldb/lldb-private-interfaces.h @@ -9,6 +9,7 @@ #ifndef LLDB_LLDB_PRIVATE_INTERFACES_H #define LLDB_LLDB_PRIVATE_INTERFACES_H +#include "lldb/Symbol/CoreDumpOptions.h" #include "lldb/lldb-enumerations.h" #include "lldb/lldb-forward.h" #include "lldb/lldb-private-enumerations.h" @@ -55,8 +56,7 @@ typedef ObjectFile *(*ObjectFileCreateMemoryInstance)( const lldb::ModuleSP &module_sp, lldb::WritableDataBufferSP data_sp, const lldb::ProcessSP &process_sp, lldb::addr_t offset); typedef bool (*ObjectFileSaveCore)(const lldb::ProcessSP &process_sp, - const FileSpec &outfile, - lldb::SaveCoreStyle &core_style, + lldb_private::CoreDumpOptions &core_options, Status &error); typedef EmulateInstruction *(*EmulateInstructionCreateInstance)( const ArchSpec &arch, InstructionType inst_type); diff --git a/lldb/source/API/CMakeLists.txt b/lldb/source/API/CMakeLists.txt index 6397101609315..d8cb532f4015f 100644 --- a/lldb/source/API/CMakeLists.txt +++ b/lldb/source/API/CMakeLists.txt @@ -56,6 +56,7 @@ add_lldb_library(liblldb SHARED ${option_framework} SBCommandReturnObject.cpp SBCommunication.cpp SBCompileUnit.cpp + SBCoreDumpOptions.cpp SBData.cpp SBDebugger.cpp SBDeclaration.cpp diff --git a/lldb/source/API/SBCoreDumpOptions.cpp b/lldb/source/API/SBCoreDumpOptions.cpp index d051c5b6bef6b..f7eab4231d819 100644 --- a/lldb/source/API/SBCoreDumpOptions.cpp +++ b/lldb/source/API/SBCoreDumpOptions.cpp @@ -7,10 +7,59 @@ //===----------------------------------------------------------------------===// #include "lldb/API/SBCoreDumpOptions.h" +#include "lldb/Symbol/CoreDumpOptions.h" +#include "lldb/Utility/Instrumentation.h" +#include "lldb/Host/FileSystem.h" + +#include "Utils.h" using namespace lldb; -using namespace lldb_private; + +SBCoreDumpOptions::SBCoreDumpOptions(const char* filePath) { + LLDB_INSTRUMENT_VA(this, filePath); + lldb_private::FileSpec fspec(filePath); + lldb_private::FileSystem::Instance().Resolve(fspec); + m_opaque_up = std::make_unique<lldb_private::CoreDumpOptions>(fspec); +} + +SBCoreDumpOptions::SBCoreDumpOptions(const SBCoreDumpOptions &rhs) { + LLDB_INSTRUMENT_VA(this, rhs); + + m_opaque_up = clone(rhs.m_opaque_up); +} + +const SBCoreDumpOptions & +SBCoreDumpOptions::operator=(const SBCoreDumpOptions &rhs) { + LLDB_INSTRUMENT_VA(this, rhs); + + if (this != &rhs) + m_opaque_up = clone(rhs.m_opaque_up); + return *this; +} void SBCoreDumpOptions::SetCoreDumpPluginName(const char *name) { + m_opaque_up->SetCoreDumpPluginName(name); +} + +void SBCoreDumpOptions::SetCoreDumpStyle(lldb::SaveCoreStyle style) { + m_opaque_up->SetCoreDumpStyle(style); +} + +const std::optional<const char *> SBCoreDumpOptions::GetCoreDumpPluginName() const { + const auto &name = m_opaque_up->GetCoreDumpPluginName(); + if (name->empty()) + return std::nullopt; + return name->data(); +} + +const char * SBCoreDumpOptions::GetOutputFile() const { + return m_opaque_up->GetOutputFile().GetFilename().AsCString(); +} + +const std::optional<lldb::SaveCoreStyle> SBCoreDumpOptions::GetCoreDumpStyle() const { + return m_opaque_up->GetCoreDumpStyle(); +} -}; +lldb_private::CoreDumpOptions& SBCoreDumpOptions::Ref() const { + return *m_opaque_up.get(); +} diff --git a/lldb/source/API/SBProcess.cpp b/lldb/source/API/SBProcess.cpp index efb3c8def2796..ef4641c43464b 100644 --- a/lldb/source/API/SBProcess.cpp +++ b/lldb/source/API/SBProcess.cpp @@ -34,6 +34,7 @@ #include "lldb/API/SBBroadcaster.h" #include "lldb/API/SBCommandReturnObject.h" +#include "lldb/API/SBCoreDumpOptions.h" #include "lldb/API/SBDebugger.h" #include "lldb/API/SBEvent.h" #include "lldb/API/SBFile.h" @@ -1222,7 +1223,18 @@ lldb::SBError SBProcess::SaveCore(const char *file_name) { lldb::SBError SBProcess::SaveCore(const char *file_name, const char *flavor, SaveCoreStyle core_style) { - LLDB_INSTRUMENT_VA(this, file_name, flavor, core_style); + SBCoreDumpOptions options(file_name); + options.SetCoreDumpPluginName(flavor); + options.SetCoreDumpStyle(core_style); + return SaveCore(options); +} + +lldb::SBError SBProcess::SaveCore(SBCoreDumpOptions &options) { + + LLDB_INSTRUMENT_VA(this, + options.GetOutputFile(), + options.GetCoreDumpPluginName(), + options.GetCoreDumpStyle()); lldb::SBError error; ProcessSP process_sp(GetSP()); @@ -1239,10 +1251,7 @@ lldb::SBError SBProcess::SaveCore(const char *file_name, return error; } - FileSpec core_file(file_name); - FileSystem::Instance().Resolve(core_file); - error.ref() = PluginManager::SaveCore(process_sp, core_file, core_style, - flavor); + error.ref() = PluginManager::SaveCore(process_sp, options.Ref()); return error; } diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp index 3587a8f529e4a..382b4058c74b3 100644 --- a/lldb/source/Commands/CommandObjectProcess.cpp +++ b/lldb/source/Commands/CommandObjectProcess.cpp @@ -1304,9 +1304,11 @@ class CommandObjectProcessSaveCore : public CommandObjectParsed { FileSpec output_file(command.GetArgumentAtIndex(0)); FileSystem::Instance().Resolve(output_file); SaveCoreStyle corefile_style = m_options.m_requested_save_core_style; + CoreDumpOptions core_dump_options(output_file); + core_dump_options.SetCoreDumpPluginName(m_options.m_requested_plugin_name); + core_dump_options.SetCoreDumpStyle(corefile_style); Status error = - PluginManager::SaveCore(process_sp, output_file, corefile_style, - m_options.m_requested_plugin_name); + PluginManager::SaveCore(process_sp, core_dump_options); if (error.Success()) { if (corefile_style == SaveCoreStyle::eSaveCoreDirtyOnly || corefile_style == SaveCoreStyle::eSaveCoreStackOnly) { diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp index b428370d7f333..346d38248e7c2 100644 --- a/lldb/source/Core/PluginManager.cpp +++ b/lldb/source/Core/PluginManager.cpp @@ -8,6 +8,7 @@ #include "lldb/Core/PluginManager.h" +#include "lldb/Symbol/CoreDumpOptions.h" #include "lldb/Core/Debugger.h" #include "lldb/Host/FileSystem.h" #include "lldb/Host/HostInfo.h" @@ -689,12 +690,10 @@ PluginManager::GetObjectFileCreateMemoryCallbackForPluginName( } Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp, - const FileSpec &outfile, - lldb::SaveCoreStyle &core_style, - llvm::StringRef plugin_name) { - if (plugin_name.empty()) { + lldb_private::CoreDumpOptions &options) { + if (options.GetCoreDumpPluginName()->empty()) { // Try saving core directly from the process plugin first. - llvm::Expected<bool> ret = process_sp->SaveCore(outfile.GetPath()); + llvm::Expected<bool> ret = process_sp->SaveCore(options.GetOutputFile().GetPath()); if (!ret) return Status(ret.takeError()); if (ret.get()) @@ -705,17 +704,22 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp, Status error; auto &instances = GetObjectFileInstances().GetInstances(); for (auto &instance : instances) { - if (plugin_name.empty() || instance.name == plugin_name) { + if (options.GetCoreDumpPluginName()->empty() || instance.name == options.GetCoreDumpPluginName()) { if (instance.save_core && - instance.save_core(process_sp, outfile, core_style, error)) + instance.save_core(process_sp, options, error)) return error; } } - error.SetErrorString( - "no ObjectFile plugins were able to save a core for this process"); + + // Check to see if any of the object file plugins tried and failed to save. + // If none ran, set the error message. + if (error.Success()) + error.SetErrorString( + "no ObjectFile plugins were able to save a core for this process"); return error; } + #pragma mark ObjectContainer struct ObjectContainerInstance diff --git a/lldb/source/Plugins/ObjectFile/CoreDumpOptions.cpp b/lldb/source/Plugins/ObjectFile/CoreDumpOptions.cpp deleted file mode 100644 index 00333d1550790..0000000000000 --- a/lldb/source/Plugins/ObjectFile/CoreDumpOptions.cpp +++ /dev/null @@ -1,25 +0,0 @@ -//===-- CoreDumpOptions.cpp -------------------------------------*- C++ -*-===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -#include "lldb/API/CoreDumpOptions.h" - -CoreDumpOptions::SetCoreDumpPluginName(const char *name) { - m_core_dump_plugin_name = name; -} - -CoreDumpOptions::GetCoreDumpPluginName() { return m_core_dump_plugin_name; } - -CoreDumpOptions::SetCoreDumpStyle(lldb::SaveCoreStyle style) { - m_core_dump_style = style; -} - -CoreDumpOptions::GetCoreDumpStyle() { return m_core_dump_style; } - -CoreDumpOptions::SetCoreDumpFile(const char *file) { m_core_dump_file = file; } - -CoreDumpOptions::GetCoreDumpFile() { return m_core_dump_file; } diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp index 0dcb1bed23548..f5ade48af07dc 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp @@ -6518,9 +6518,11 @@ struct page_object { uint32_t prot; }; -bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp, - const FileSpec &outfile, - lldb::SaveCoreStyle &core_style, Status &error) { +bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp, + lldb_private::CoreDumpOptions &core_options, + Status &error) { + auto core_style = core_options.GetCoreDumpStyle(); + const auto outfile = core_options.GetOutputFile(); if (!process_sp) return false; diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h index 55bc688126eb3..d77f0f68cdf11 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h @@ -62,8 +62,7 @@ class ObjectFileMachO : public lldb_private::ObjectFile { lldb_private::ModuleSpecList &specs); static bool SaveCore(const lldb::ProcessSP &process_sp, - const lldb_private::FileSpec &outfile, - lldb::SaveCoreStyle &core_style, + lldb_private::CoreDumpOptions &core_options, lldb_private::Status &error); static bool MagicBytesMatch(lldb::DataBufferSP data_sp, lldb::addr_t offset, diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index 7c875aa3223ed..f7c833edefb5d 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -56,18 +56,17 @@ size_t ObjectFileMinidump::GetModuleSpecifications( } bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, - const lldb_private::FileSpec &outfile, - lldb::SaveCoreStyle &core_style, + lldb_private::CoreDumpOptions &core_options, lldb_private::Status &error) { // Set default core style if it isn't set. - if (core_style == SaveCoreStyle::eSaveCoreUnspecified) - core_style = SaveCoreStyle::eSaveCoreStackOnly; + if (core_options.GetCoreDumpStyle() == SaveCoreStyle::eSaveCoreUnspecified) + core_options.SetCoreDumpStyle(SaveCoreStyle::eSaveCoreStackOnly); if (!process_sp) return false; llvm::Expected<lldb::FileUP> maybe_core_file = FileSystem::Instance().Open( - outfile, File::eOpenOptionWriteOnly | File::eOpenOptionCanCreate); + core_options.GetOutputFile(), File::eOpenOptionWriteOnly | File::eOpenOptionCanCreate); if (!maybe_core_file) { error = maybe_core_file.takeError(); return false; @@ -119,7 +118,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, // Note: add memory HAS to be the last thing we do. It can overflow into 64b // land and many RVA's only support 32b - error = builder.AddMemoryList(core_style); + error = builder.AddMemoryList(core_options.GetCoreDumpStyle()); if (error.Fail()) { LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString()); return false; diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h index b5c40445fe742..81e7b3339ab39 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h @@ -55,8 +55,7 @@ class ObjectFileMinidump : public lldb_private::PluginInterface { // Saves dump in Minidump file format static bool SaveCore(const lldb::ProcessSP &process_sp, - const lldb_private::FileSpec &outfile, - lldb::SaveCoreStyle &core_style, + lldb_private::CoreDumpOptions &core_options, lldb_private::Status &error); private: diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp index be0020cad5bee..4cd01d7f74bd6 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp +++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp @@ -355,11 +355,10 @@ size_t ObjectFilePECOFF::GetModuleSpecifications( } bool ObjectFilePECOFF::SaveCore(const lldb::ProcessSP &process_sp, - const lldb_private::FileSpec &outfile, - lldb::SaveCoreStyle &core_style, + lldb_private::CoreDumpOptions &core_options, lldb_private::Status &error) { - core_style = eSaveCoreFull; - return SaveMiniDump(process_sp, outfile, error); + core_options.SetCoreDumpStyle(lldb::eSaveCoreFull); + return SaveMiniDump(process_sp, core_options, error); } bool ObjectFilePECOFF::MagicBytesMatch(DataBufferSP data_sp) { diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h index c59701fa65ec3..1074604864978 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h +++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h @@ -82,8 +82,7 @@ class ObjectFilePECOFF : public lldb_private::ObjectFile { lldb_private::ModuleSpecList &specs); static bool SaveCore(const lldb::ProcessSP &process_sp, - const lldb_private::FileSpec &outfile, - lldb::SaveCoreStyle &core_style, + lldb_private::CoreDumpOptions &core_options, lldb_private::Status &error); static bool MagicBytesMatch(lldb::DataBufferSP data_sp); diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp index e5cb36910dd25..7fafc9a4370e8 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp +++ b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp @@ -21,11 +21,12 @@ namespace lldb_private { bool SaveMiniDump(const lldb::ProcessSP &process_sp, - const lldb_private::FileSpec &outfile, + CoreDumpOptions &core_options, lldb_private::Status &error) { if (!process_sp) return false; #ifdef _WIN32 + const auto outfile = core_options.GetOutputFile(); HANDLE process_handle = ::OpenProcess( PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, process_sp->GetID()); const std::string file_name = outfile.GetPath(); diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h index 04b93e221362a..f94d873989a18 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h +++ b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h @@ -14,7 +14,7 @@ namespace lldb_private { bool SaveMiniDump(const lldb::ProcessSP &process_sp, - const lldb_private::FileSpec &outfile, + CoreDumpOptions &core_options, lldb_private::Status &error); } // namespace lldb_private diff --git a/lldb/source/Symbol/CMakeLists.txt b/lldb/source/Symbol/CMakeLists.txt index ad3488dcdfcec..f1a2e25cdef48 100644 --- a/lldb/source/Symbol/CMakeLists.txt +++ b/lldb/source/Symbol/CMakeLists.txt @@ -6,6 +6,7 @@ add_lldb_library(lldbSymbol NO_PLUGIN_DEPENDENCIES CompilerDecl.cpp CompilerDeclContext.cpp CompilerType.cpp + CoreDumpOptions.cpp DWARFCallFrameInfo.cpp DebugMacros.cpp DeclVendor.cpp diff --git a/lldb/source/Symbol/CoreDumpOptions.cpp b/lldb/source/Symbol/CoreDumpOptions.cpp new file mode 100644 index 0000000000000..688105a6aea2f --- /dev/null +++ b/lldb/source/Symbol/CoreDumpOptions.cpp @@ -0,0 +1,35 @@ +//===-- CoreDumpOptions.cpp -------------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "lldb/Symbol/CoreDumpOptions.h" + +using namespace lldb; +using namespace lldb_private; + +void CoreDumpOptions::SetCoreDumpPluginName(const llvm::StringRef name) { + m_core_dump_plugin_name = name.data(); +} + +std::optional<llvm::StringRef> CoreDumpOptions::GetCoreDumpPluginName() const { + if (!m_core_dump_plugin_name) + return std::nullopt; + return m_core_dump_plugin_name->data(); +} + +void CoreDumpOptions::SetCoreDumpStyle(lldb::SaveCoreStyle style) { + m_core_dump_style = style; +} + +lldb::SaveCoreStyle CoreDumpOptions::GetCoreDumpStyle() const { + // If unspecified, default to stack only + if (m_core_dump_style == lldb::eSaveCoreUnspecified) + return lldb::eSaveCoreStackOnly; + return m_core_dump_style; +} + +const lldb_private::FileSpec& CoreDumpOptions::GetOutputFile() const { return m_core_dump_file; } diff --git a/lldb/test/API/functionalities/postmortem/minidump/TestMiniDump.py b/lldb/test/API/functionalities/postmortem/minidump/TestMiniDump.py index 8fe5d2a1d8562..101950da16d36 100644 --- a/lldb/test/API/functionalities/postmortem/minidump/TestMiniDump.py +++ b/lldb/test/API/functionalities/postmortem/minidump/TestMiniDump.py @@ -130,6 +130,7 @@ def test_deeper_stack_in_mini_dump(self): process = target.LaunchSimple( None, None, self.get_process_working_directory() ) + self.assertState(process.GetState(), lldb.eStateStopped) self.assertTrue(process.SaveCore(core)) self.assertTrue(os.path.isfile(core)) diff --git a/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py b/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py index c3f0e7597758a..b6406579f3c55 100644 --- a/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py +++ b/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py @@ -21,6 +21,7 @@ def test_cannot_save_core_unless_process_stopped(self): target = self.dbg.CreateTarget(exe) process = target.LaunchSimple(None, None, self.get_process_working_directory()) self.assertNotEqual(process.GetState(), lldb.eStateStopped) + options = SBCoreDumpOptions(core) error = process.SaveCore(core) self.assertTrue(error.Fail()) diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py index 1e171e726fb6b..27492756f6b4a 100644 --- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py +++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py @@ -132,8 +132,11 @@ def test_save_linux_mini_dump(self): stacks_to_sp_map, ) + options = lldb.SBCoreDumpOptions(core_sb_stack) + options.SetCoreDumpPluginName("minidump") + options.SetCoreDumpStyle(lldb.eSaveCoreStackOnly) # validate saving via SBProcess - error = process.SaveCore(core_sb_stack, "minidump", lldb.eSaveCoreStackOnly) + error = process.SaveCore(options) self.assertTrue(error.Success()) self.assertTrue(os.path.isfile(core_sb_stack)) self.verify_core_file( @@ -144,7 +147,10 @@ def test_save_linux_mini_dump(self): stacks_to_sp_map, ) - error = process.SaveCore(core_sb_dirty, "minidump", lldb.eSaveCoreDirtyOnly) + options = lldb.SBCoreDumpOptions(core_sb_dirty) + options.SetCoreDumpPluginName("minidump") + options.SetCoreDumpStyle(lldb.eSaveCoreDirtyOnly) + error = process.SaveCore(options) self.assertTrue(error.Success()) self.assertTrue(os.path.isfile(core_sb_dirty)) self.verify_core_file( @@ -157,7 +163,10 @@ def test_save_linux_mini_dump(self): # Minidump can now save full core files, but they will be huge and # they might cause this test to timeout. - error = process.SaveCore(core_sb_full, "minidump", lldb.eSaveCoreFull) + options = lldb.SBCoreDumpOptions(core_sb_full) + options.SetCoreDumpPluginName("minidump") + options.SetCoreDumpStyle(lldb.eSaveCoreFull) + error = process.SaveCore(options) self.assertTrue(error.Success()) self.assertTrue(os.path.isfile(core_sb_full)) self.verify_core_file( >From e8425792f02c60e74c0070c3674e54d41f06205b Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 10 Jul 2024 15:20:51 -0700 Subject: [PATCH 3/6] Add descriptions to sbcoredumpoptions methods. Run git-clang-format --- lldb/include/lldb/API/SBCoreDumpOptions.h | 23 ++++++++++++++++--- lldb/include/lldb/Symbol/CoreDumpOptions.h | 11 ++++----- lldb/source/API/SBCoreDumpOptions.cpp | 14 ++++++----- lldb/source/API/SBProcess.cpp | 7 +++--- lldb/source/Commands/CommandObjectProcess.cpp | 6 ++--- lldb/source/Core/PluginManager.cpp | 12 +++++----- .../ObjectFile/Mach-O/ObjectFileMachO.cpp | 2 +- .../Minidump/ObjectFileMinidump.cpp | 3 ++- .../ObjectFile/PECOFF/WindowsMiniDump.cpp | 3 +-- .../ObjectFile/PECOFF/WindowsMiniDump.h | 3 +-- lldb/source/Symbol/CoreDumpOptions.cpp | 8 ++++--- 11 files changed, 55 insertions(+), 37 deletions(-) diff --git a/lldb/include/lldb/API/SBCoreDumpOptions.h b/lldb/include/lldb/API/SBCoreDumpOptions.h index b3e81ceee852a..48baf448492b4 100644 --- a/lldb/include/lldb/API/SBCoreDumpOptions.h +++ b/lldb/include/lldb/API/SBCoreDumpOptions.h @@ -16,19 +16,36 @@ namespace lldb { class LLDB_API SBCoreDumpOptions { public: - SBCoreDumpOptions(const char* filePath); + SBCoreDumpOptions(const char *filePath); SBCoreDumpOptions(const lldb::SBCoreDumpOptions &rhs); ~SBCoreDumpOptions() = default; const SBCoreDumpOptions &operator=(const lldb::SBCoreDumpOptions &rhs); - void SetCoreDumpPluginName(const char* plugin); + /// Set the Core dump plugin name. + /// + /// \param plugin Name of the object file plugin. + void SetCoreDumpPluginName(const char *plugin); + + /// Get the Core dump plugin name, if set. + /// + /// \return The name of the plugin, or nullopt if not set. const std::optional<const char *> GetCoreDumpPluginName() const; + /// Set the Core dump style. + /// + /// \param style The style of the core dump. void SetCoreDumpStyle(lldb::SaveCoreStyle style); + + /// Get the Core dump style, if set. + /// + /// \return The core dump style, or nullopt if not set. const std::optional<lldb::SaveCoreStyle> GetCoreDumpStyle() const; - const char * GetOutputFile() const; + /// Get the output file path + /// + /// \return The output file path. + const char *GetOutputFile() const; protected: friend class SBProcess; diff --git a/lldb/include/lldb/Symbol/CoreDumpOptions.h b/lldb/include/lldb/Symbol/CoreDumpOptions.h index 435031a510d80..b5482bcf24359 100644 --- a/lldb/include/lldb/Symbol/CoreDumpOptions.h +++ b/lldb/include/lldb/Symbol/CoreDumpOptions.h @@ -19,11 +19,10 @@ namespace lldb_private { class CoreDumpOptions { - public: - CoreDumpOptions(const lldb_private::FileSpec &fspec) : - m_core_dump_file(std::move(fspec)) {}; - ~CoreDumpOptions() = default; - +public: + CoreDumpOptions(const lldb_private::FileSpec &fspec) + : m_core_dump_file(std::move(fspec)){}; + ~CoreDumpOptions() = default; void SetCoreDumpPluginName(llvm::StringRef name); std::optional<llvm::StringRef> GetCoreDumpPluginName() const; @@ -31,7 +30,7 @@ class CoreDumpOptions { void SetCoreDumpStyle(lldb::SaveCoreStyle style); lldb::SaveCoreStyle GetCoreDumpStyle() const; - const lldb_private::FileSpec& GetOutputFile() const; + const lldb_private::FileSpec &GetOutputFile() const; private: std::optional<std::string> m_core_dump_plugin_name; diff --git a/lldb/source/API/SBCoreDumpOptions.cpp b/lldb/source/API/SBCoreDumpOptions.cpp index f7eab4231d819..944f44e75039b 100644 --- a/lldb/source/API/SBCoreDumpOptions.cpp +++ b/lldb/source/API/SBCoreDumpOptions.cpp @@ -7,15 +7,15 @@ //===----------------------------------------------------------------------===// #include "lldb/API/SBCoreDumpOptions.h" +#include "lldb/Host/FileSystem.h" #include "lldb/Symbol/CoreDumpOptions.h" #include "lldb/Utility/Instrumentation.h" -#include "lldb/Host/FileSystem.h" #include "Utils.h" using namespace lldb; -SBCoreDumpOptions::SBCoreDumpOptions(const char* filePath) { +SBCoreDumpOptions::SBCoreDumpOptions(const char *filePath) { LLDB_INSTRUMENT_VA(this, filePath); lldb_private::FileSpec fspec(filePath); lldb_private::FileSystem::Instance().Resolve(fspec); @@ -45,21 +45,23 @@ void SBCoreDumpOptions::SetCoreDumpStyle(lldb::SaveCoreStyle style) { m_opaque_up->SetCoreDumpStyle(style); } -const std::optional<const char *> SBCoreDumpOptions::GetCoreDumpPluginName() const { +const std::optional<const char *> +SBCoreDumpOptions::GetCoreDumpPluginName() const { const auto &name = m_opaque_up->GetCoreDumpPluginName(); if (name->empty()) return std::nullopt; return name->data(); } -const char * SBCoreDumpOptions::GetOutputFile() const { +const char *SBCoreDumpOptions::GetOutputFile() const { return m_opaque_up->GetOutputFile().GetFilename().AsCString(); } -const std::optional<lldb::SaveCoreStyle> SBCoreDumpOptions::GetCoreDumpStyle() const { +const std::optional<lldb::SaveCoreStyle> +SBCoreDumpOptions::GetCoreDumpStyle() const { return m_opaque_up->GetCoreDumpStyle(); } -lldb_private::CoreDumpOptions& SBCoreDumpOptions::Ref() const { +lldb_private::CoreDumpOptions &SBCoreDumpOptions::Ref() const { return *m_opaque_up.get(); } diff --git a/lldb/source/API/SBProcess.cpp b/lldb/source/API/SBProcess.cpp index ef4641c43464b..436b615bab920 100644 --- a/lldb/source/API/SBProcess.cpp +++ b/lldb/source/API/SBProcess.cpp @@ -1231,10 +1231,9 @@ lldb::SBError SBProcess::SaveCore(const char *file_name, lldb::SBError SBProcess::SaveCore(SBCoreDumpOptions &options) { - LLDB_INSTRUMENT_VA(this, - options.GetOutputFile(), - options.GetCoreDumpPluginName(), - options.GetCoreDumpStyle()); + LLDB_INSTRUMENT_VA(this, options.GetOutputFile(), + options.GetCoreDumpPluginName(), + options.GetCoreDumpStyle()); lldb::SBError error; ProcessSP process_sp(GetSP()); diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp index 382b4058c74b3..f17c50ee22ac3 100644 --- a/lldb/source/Commands/CommandObjectProcess.cpp +++ b/lldb/source/Commands/CommandObjectProcess.cpp @@ -1305,10 +1305,10 @@ class CommandObjectProcessSaveCore : public CommandObjectParsed { FileSystem::Instance().Resolve(output_file); SaveCoreStyle corefile_style = m_options.m_requested_save_core_style; CoreDumpOptions core_dump_options(output_file); - core_dump_options.SetCoreDumpPluginName(m_options.m_requested_plugin_name); + core_dump_options.SetCoreDumpPluginName( + m_options.m_requested_plugin_name); core_dump_options.SetCoreDumpStyle(corefile_style); - Status error = - PluginManager::SaveCore(process_sp, core_dump_options); + Status error = PluginManager::SaveCore(process_sp, core_dump_options); if (error.Success()) { if (corefile_style == SaveCoreStyle::eSaveCoreDirtyOnly || corefile_style == SaveCoreStyle::eSaveCoreStackOnly) { diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp index 346d38248e7c2..90231af030d90 100644 --- a/lldb/source/Core/PluginManager.cpp +++ b/lldb/source/Core/PluginManager.cpp @@ -8,11 +8,11 @@ #include "lldb/Core/PluginManager.h" -#include "lldb/Symbol/CoreDumpOptions.h" #include "lldb/Core/Debugger.h" #include "lldb/Host/FileSystem.h" #include "lldb/Host/HostInfo.h" #include "lldb/Interpreter/OptionValueProperties.h" +#include "lldb/Symbol/CoreDumpOptions.h" #include "lldb/Target/Process.h" #include "lldb/Utility/FileSpec.h" #include "lldb/Utility/Status.h" @@ -693,7 +693,8 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp, lldb_private::CoreDumpOptions &options) { if (options.GetCoreDumpPluginName()->empty()) { // Try saving core directly from the process plugin first. - llvm::Expected<bool> ret = process_sp->SaveCore(options.GetOutputFile().GetPath()); + llvm::Expected<bool> ret = + process_sp->SaveCore(options.GetOutputFile().GetPath()); if (!ret) return Status(ret.takeError()); if (ret.get()) @@ -704,9 +705,9 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp, Status error; auto &instances = GetObjectFileInstances().GetInstances(); for (auto &instance : instances) { - if (options.GetCoreDumpPluginName()->empty() || instance.name == options.GetCoreDumpPluginName()) { - if (instance.save_core && - instance.save_core(process_sp, options, error)) + if (options.GetCoreDumpPluginName()->empty() || + instance.name == options.GetCoreDumpPluginName()) { + if (instance.save_core && instance.save_core(process_sp, options, error)) return error; } } @@ -719,7 +720,6 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp, return error; } - #pragma mark ObjectContainer struct ObjectContainerInstance diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp index f5ade48af07dc..bfb1d6d69f002 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp @@ -6518,7 +6518,7 @@ struct page_object { uint32_t prot; }; -bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp, +bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp, lldb_private::CoreDumpOptions &core_options, Status &error) { auto core_style = core_options.GetCoreDumpStyle(); diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index f7c833edefb5d..a4c7f9d8cf610 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -66,7 +66,8 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, return false; llvm::Expected<lldb::FileUP> maybe_core_file = FileSystem::Instance().Open( - core_options.GetOutputFile(), File::eOpenOptionWriteOnly | File::eOpenOptionCanCreate); + core_options.GetOutputFile(), + File::eOpenOptionWriteOnly | File::eOpenOptionCanCreate); if (!maybe_core_file) { error = maybe_core_file.takeError(); return false; diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp index 7fafc9a4370e8..79eaf36752a85 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp +++ b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp @@ -21,8 +21,7 @@ namespace lldb_private { bool SaveMiniDump(const lldb::ProcessSP &process_sp, - CoreDumpOptions &core_options, - lldb_private::Status &error) { + CoreDumpOptions &core_options, lldb_private::Status &error) { if (!process_sp) return false; #ifdef _WIN32 diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h index f94d873989a18..eaddfa17b7455 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h +++ b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h @@ -14,8 +14,7 @@ namespace lldb_private { bool SaveMiniDump(const lldb::ProcessSP &process_sp, - CoreDumpOptions &core_options, - lldb_private::Status &error); + CoreDumpOptions &core_options, lldb_private::Status &error); } // namespace lldb_private diff --git a/lldb/source/Symbol/CoreDumpOptions.cpp b/lldb/source/Symbol/CoreDumpOptions.cpp index 688105a6aea2f..78e7e1a3e8617 100644 --- a/lldb/source/Symbol/CoreDumpOptions.cpp +++ b/lldb/source/Symbol/CoreDumpOptions.cpp @@ -15,7 +15,7 @@ void CoreDumpOptions::SetCoreDumpPluginName(const llvm::StringRef name) { m_core_dump_plugin_name = name.data(); } -std::optional<llvm::StringRef> CoreDumpOptions::GetCoreDumpPluginName() const { +std::optional<llvm::StringRef> CoreDumpOptions::GetCoreDumpPluginName() const { if (!m_core_dump_plugin_name) return std::nullopt; return m_core_dump_plugin_name->data(); @@ -25,11 +25,13 @@ void CoreDumpOptions::SetCoreDumpStyle(lldb::SaveCoreStyle style) { m_core_dump_style = style; } -lldb::SaveCoreStyle CoreDumpOptions::GetCoreDumpStyle() const { +lldb::SaveCoreStyle CoreDumpOptions::GetCoreDumpStyle() const { // If unspecified, default to stack only if (m_core_dump_style == lldb::eSaveCoreUnspecified) return lldb::eSaveCoreStackOnly; return m_core_dump_style; } -const lldb_private::FileSpec& CoreDumpOptions::GetOutputFile() const { return m_core_dump_file; } +const lldb_private::FileSpec &CoreDumpOptions::GetOutputFile() const { + return m_core_dump_file; +} >From 2673674ffaff7e1e62480928f88f7525c378dbc7 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Tue, 16 Jul 2024 17:38:16 -0700 Subject: [PATCH 4/6] Code review feedback on all API's, fix tests, and remove default behaviors in each flavor of save core --- lldb/include/lldb/API/SBCoreDumpOptions.h | 25 ++++++++----- lldb/include/lldb/API/SBFileSpec.h | 1 + lldb/include/lldb/Core/PluginManager.h | 2 +- lldb/include/lldb/Symbol/CoreDumpOptions.h | 18 +++++----- lldb/include/lldb/lldb-private-interfaces.h | 2 +- lldb/source/API/SBCoreDumpOptions.cpp | 35 ++++++++++++------- lldb/source/API/SBProcess.cpp | 4 ++- lldb/source/Commands/CommandObjectProcess.cpp | 25 ++++++------- lldb/source/Core/PluginManager.cpp | 11 ++++-- .../ObjectFile/Mach-O/ObjectFileMachO.cpp | 12 +++---- .../ObjectFile/Mach-O/ObjectFileMachO.h | 2 +- .../Minidump/ObjectFileMinidump.cpp | 10 ++---- .../ObjectFile/Minidump/ObjectFileMinidump.h | 2 +- .../ObjectFile/PECOFF/ObjectFilePECOFF.cpp | 3 +- .../ObjectFile/PECOFF/ObjectFilePECOFF.h | 2 +- .../ObjectFile/PECOFF/WindowsMiniDump.cpp | 2 +- .../ObjectFile/PECOFF/WindowsMiniDump.h | 2 +- lldb/source/Symbol/CoreDumpOptions.cpp | 32 ++++++++++------- .../TestProcessSaveCoreMinidump.py | 12 +++++-- 19 files changed, 115 insertions(+), 87 deletions(-) diff --git a/lldb/include/lldb/API/SBCoreDumpOptions.h b/lldb/include/lldb/API/SBCoreDumpOptions.h index 48baf448492b4..c9cec5b841ba3 100644 --- a/lldb/include/lldb/API/SBCoreDumpOptions.h +++ b/lldb/include/lldb/API/SBCoreDumpOptions.h @@ -16,7 +16,7 @@ namespace lldb { class LLDB_API SBCoreDumpOptions { public: - SBCoreDumpOptions(const char *filePath); + SBCoreDumpOptions(); SBCoreDumpOptions(const lldb::SBCoreDumpOptions &rhs); ~SBCoreDumpOptions() = default; @@ -29,8 +29,8 @@ class LLDB_API SBCoreDumpOptions { /// Get the Core dump plugin name, if set. /// - /// \return The name of the plugin, or nullopt if not set. - const std::optional<const char *> GetCoreDumpPluginName() const; + /// \return The name of the plugin, or null if not set. + const char * GetCoreDumpPluginName() const; /// Set the Core dump style. /// @@ -39,13 +39,22 @@ class LLDB_API SBCoreDumpOptions { /// Get the Core dump style, if set. /// - /// \return The core dump style, or nullopt if not set. - const std::optional<lldb::SaveCoreStyle> GetCoreDumpStyle() const; + /// \return The core dump style, or undefined if not set. + lldb::SaveCoreStyle GetCoreDumpStyle() const; - /// Get the output file path + /// Set the output file path /// - /// \return The output file path. - const char *GetOutputFile() const; + /// \param output_file a + /// \class SBFileSpec object that describes the output file. + void SetOutputFile(SBFileSpec &output_file); + + /// Get the output file spec + /// + /// \return The output file spec. + SBFileSpec GetOutputFile() const; + + /// Reset all options. + void Clear(); protected: friend class SBProcess; diff --git a/lldb/include/lldb/API/SBFileSpec.h b/lldb/include/lldb/API/SBFileSpec.h index beefa19ad7f36..bcf090d4c0ae2 100644 --- a/lldb/include/lldb/API/SBFileSpec.h +++ b/lldb/include/lldb/API/SBFileSpec.h @@ -78,6 +78,7 @@ class LLDB_API SBFileSpec { friend class SBTarget; friend class SBThread; friend class SBTrace; + friend class SBCoreDumpOptions; SBFileSpec(const lldb_private::FileSpec &fspec); diff --git a/lldb/include/lldb/Core/PluginManager.h b/lldb/include/lldb/Core/PluginManager.h index dcc3a8a062265..297381fa911e6 100644 --- a/lldb/include/lldb/Core/PluginManager.h +++ b/lldb/include/lldb/Core/PluginManager.h @@ -191,7 +191,7 @@ class PluginManager { GetObjectFileCreateMemoryCallbackForPluginName(llvm::StringRef name); static Status SaveCore(const lldb::ProcessSP &process_sp, - lldb_private::CoreDumpOptions &core_options); + const lldb_private::CoreDumpOptions &core_options); // ObjectContainer static bool RegisterPlugin( diff --git a/lldb/include/lldb/Symbol/CoreDumpOptions.h b/lldb/include/lldb/Symbol/CoreDumpOptions.h index b5482bcf24359..75d70a5967c9f 100644 --- a/lldb/include/lldb/Symbol/CoreDumpOptions.h +++ b/lldb/include/lldb/Symbol/CoreDumpOptions.h @@ -20,22 +20,24 @@ namespace lldb_private { class CoreDumpOptions { public: - CoreDumpOptions(const lldb_private::FileSpec &fspec) - : m_core_dump_file(std::move(fspec)){}; + CoreDumpOptions() {}; ~CoreDumpOptions() = default; - void SetCoreDumpPluginName(llvm::StringRef name); - std::optional<llvm::StringRef> GetCoreDumpPluginName() const; + void SetCoreDumpPluginName(const char * name); + std::optional<std::string> GetCoreDumpPluginName() const; void SetCoreDumpStyle(lldb::SaveCoreStyle style); lldb::SaveCoreStyle GetCoreDumpStyle() const; - const lldb_private::FileSpec &GetOutputFile() const; + void SetOutputFile(lldb_private::FileSpec file); + const std::optional<lldb_private::FileSpec> GetOutputFile() const; + + void Clear(); private: - std::optional<std::string> m_core_dump_plugin_name; - const lldb_private::FileSpec m_core_dump_file; - lldb::SaveCoreStyle m_core_dump_style = lldb::eSaveCoreUnspecified; + std::optional<std::string> m_plugin_name; + std::optional<lldb_private::FileSpec> m_file; + std::optional<lldb::SaveCoreStyle> m_style; }; } // namespace lldb_private diff --git a/lldb/include/lldb/lldb-private-interfaces.h b/lldb/include/lldb/lldb-private-interfaces.h index 0948a0482b201..044a57f87d492 100644 --- a/lldb/include/lldb/lldb-private-interfaces.h +++ b/lldb/include/lldb/lldb-private-interfaces.h @@ -56,7 +56,7 @@ typedef ObjectFile *(*ObjectFileCreateMemoryInstance)( const lldb::ModuleSP &module_sp, lldb::WritableDataBufferSP data_sp, const lldb::ProcessSP &process_sp, lldb::addr_t offset); typedef bool (*ObjectFileSaveCore)(const lldb::ProcessSP &process_sp, - lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &core_options, Status &error); typedef EmulateInstruction *(*EmulateInstructionCreateInstance)( const ArchSpec &arch, InstructionType inst_type); diff --git a/lldb/source/API/SBCoreDumpOptions.cpp b/lldb/source/API/SBCoreDumpOptions.cpp index 944f44e75039b..fc00430a2809f 100644 --- a/lldb/source/API/SBCoreDumpOptions.cpp +++ b/lldb/source/API/SBCoreDumpOptions.cpp @@ -6,6 +6,7 @@ // //===----------------------------------------------------------------------===// +#include "lldb/API/SBFileSpec.h" #include "lldb/API/SBCoreDumpOptions.h" #include "lldb/Host/FileSystem.h" #include "lldb/Symbol/CoreDumpOptions.h" @@ -15,11 +16,8 @@ using namespace lldb; -SBCoreDumpOptions::SBCoreDumpOptions(const char *filePath) { - LLDB_INSTRUMENT_VA(this, filePath); - lldb_private::FileSpec fspec(filePath); - lldb_private::FileSystem::Instance().Resolve(fspec); - m_opaque_up = std::make_unique<lldb_private::CoreDumpOptions>(fspec); +SBCoreDumpOptions::SBCoreDumpOptions() { + m_opaque_up = std::make_unique<lldb_private::CoreDumpOptions>(); } SBCoreDumpOptions::SBCoreDumpOptions(const SBCoreDumpOptions &rhs) { @@ -45,19 +43,26 @@ void SBCoreDumpOptions::SetCoreDumpStyle(lldb::SaveCoreStyle style) { m_opaque_up->SetCoreDumpStyle(style); } -const std::optional<const char *> +void SBCoreDumpOptions::SetOutputFile(lldb::SBFileSpec &file_spec) { + m_opaque_up->SetOutputFile(file_spec.ref()); +} + +const char * SBCoreDumpOptions::GetCoreDumpPluginName() const { - const auto &name = m_opaque_up->GetCoreDumpPluginName(); - if (name->empty()) - return std::nullopt; - return name->data(); + const auto name = m_opaque_up->GetCoreDumpPluginName(); + if (!name) + return nullptr; + return lldb_private::ConstString(name.value()).GetCString(); } -const char *SBCoreDumpOptions::GetOutputFile() const { - return m_opaque_up->GetOutputFile().GetFilename().AsCString(); +SBFileSpec SBCoreDumpOptions::GetOutputFile() const { + const auto file_spec = m_opaque_up->GetOutputFile(); + if (file_spec) + return SBFileSpec(file_spec.value()); + return SBFileSpec(); } -const std::optional<lldb::SaveCoreStyle> +lldb::SaveCoreStyle SBCoreDumpOptions::GetCoreDumpStyle() const { return m_opaque_up->GetCoreDumpStyle(); } @@ -65,3 +70,7 @@ SBCoreDumpOptions::GetCoreDumpStyle() const { lldb_private::CoreDumpOptions &SBCoreDumpOptions::Ref() const { return *m_opaque_up.get(); } + +void SBCoreDumpOptions::Clear() { + m_opaque_up->Clear(); +} diff --git a/lldb/source/API/SBProcess.cpp b/lldb/source/API/SBProcess.cpp index 436b615bab920..a5a8190d97180 100644 --- a/lldb/source/API/SBProcess.cpp +++ b/lldb/source/API/SBProcess.cpp @@ -1223,7 +1223,9 @@ lldb::SBError SBProcess::SaveCore(const char *file_name) { lldb::SBError SBProcess::SaveCore(const char *file_name, const char *flavor, SaveCoreStyle core_style) { - SBCoreDumpOptions options(file_name); + SBFileSpec fspec(file_name); + SBCoreDumpOptions options; + options.SetOutputFile(fspec); options.SetCoreDumpPluginName(flavor); options.SetCoreDumpStyle(core_style); return SaveCore(options); diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp index f17c50ee22ac3..fedc12b4232fe 100644 --- a/lldb/source/Commands/CommandObjectProcess.cpp +++ b/lldb/source/Commands/CommandObjectProcess.cpp @@ -1256,7 +1256,7 @@ class CommandObjectProcessSaveCore : public CommandObjectParsed { class CommandOptions : public Options { public: - CommandOptions() = default; + CommandOptions() : m_core_dump_options() {}; ~CommandOptions() override = default; @@ -1271,13 +1271,13 @@ class CommandObjectProcessSaveCore : public CommandObjectParsed { switch (short_option) { case 'p': - m_requested_plugin_name = option_arg.str(); + m_core_dump_options.SetCoreDumpPluginName(option_arg.data()); break; case 's': - m_requested_save_core_style = + m_core_dump_options.SetCoreDumpStyle( (lldb::SaveCoreStyle)OptionArgParser::ToOptionEnum( option_arg, GetDefinitions()[option_idx].enum_values, - eSaveCoreUnspecified, error); + eSaveCoreUnspecified, error)); break; default: llvm_unreachable("Unimplemented option"); @@ -1287,13 +1287,11 @@ class CommandObjectProcessSaveCore : public CommandObjectParsed { } void OptionParsingStarting(ExecutionContext *execution_context) override { - m_requested_save_core_style = eSaveCoreUnspecified; - m_requested_plugin_name.clear(); + m_core_dump_options.Clear(); } // Instance variables to hold the values for command options. - SaveCoreStyle m_requested_save_core_style = eSaveCoreUnspecified; - std::string m_requested_plugin_name; + CoreDumpOptions m_core_dump_options; }; protected: @@ -1303,15 +1301,12 @@ class CommandObjectProcessSaveCore : public CommandObjectParsed { if (command.GetArgumentCount() == 1) { FileSpec output_file(command.GetArgumentAtIndex(0)); FileSystem::Instance().Resolve(output_file); - SaveCoreStyle corefile_style = m_options.m_requested_save_core_style; - CoreDumpOptions core_dump_options(output_file); - core_dump_options.SetCoreDumpPluginName( - m_options.m_requested_plugin_name); - core_dump_options.SetCoreDumpStyle(corefile_style); + auto core_dump_options = m_options.m_core_dump_options; + core_dump_options.SetOutputFile(output_file); Status error = PluginManager::SaveCore(process_sp, core_dump_options); if (error.Success()) { - if (corefile_style == SaveCoreStyle::eSaveCoreDirtyOnly || - corefile_style == SaveCoreStyle::eSaveCoreStackOnly) { + if (core_dump_options.GetCoreDumpStyle() == SaveCoreStyle::eSaveCoreDirtyOnly || + core_dump_options.GetCoreDumpStyle() == SaveCoreStyle::eSaveCoreStackOnly) { result.AppendMessageWithFormat( "\nModified-memory or stack-memory only corefile " "created. This corefile may \n" diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp index 90231af030d90..c0a0630cccb9e 100644 --- a/lldb/source/Core/PluginManager.cpp +++ b/lldb/source/Core/PluginManager.cpp @@ -690,11 +690,17 @@ PluginManager::GetObjectFileCreateMemoryCallbackForPluginName( } Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp, - lldb_private::CoreDumpOptions &options) { + const lldb_private::CoreDumpOptions &options) { + Status error; + if (!options.GetOutputFile()) { + error.SetErrorString("No output file specified"); + return error; + } + if (options.GetCoreDumpPluginName()->empty()) { // Try saving core directly from the process plugin first. llvm::Expected<bool> ret = - process_sp->SaveCore(options.GetOutputFile().GetPath()); + process_sp->SaveCore(options.GetOutputFile()->GetPath()); if (!ret) return Status(ret.takeError()); if (ret.get()) @@ -702,7 +708,6 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp, } // Fall back to object plugins. - Status error; auto &instances = GetObjectFileInstances().GetInstances(); for (auto &instance : instances) { if (options.GetCoreDumpPluginName()->empty() || diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp index bfb1d6d69f002..23183ada04b28 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp @@ -6519,17 +6519,13 @@ struct page_object { }; bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp, - lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &core_options, Status &error) { auto core_style = core_options.GetCoreDumpStyle(); const auto outfile = core_options.GetOutputFile(); - if (!process_sp) + if (!process_sp || !outfile) return false; - // Default on macOS is to create a dirty-memory-only corefile. - if (core_style == SaveCoreStyle::eSaveCoreUnspecified) - core_style = SaveCoreStyle::eSaveCoreDirtyOnly; - Target &target = process_sp->GetTarget(); const ArchSpec target_arch = target.GetArchitecture(); const llvm::Triple &target_triple = target_arch.GetTriple(); @@ -6813,9 +6809,9 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp, buffer.PutHex32(segment.flags); } - std::string core_file_path(outfile.GetPath()); + std::string core_file_path(core_options.GetOutputFile()->GetPath()); auto core_file = FileSystem::Instance().Open( - outfile, File::eOpenOptionWriteOnly | File::eOpenOptionTruncate | + outfile.value(), File::eOpenOptionWriteOnly | File::eOpenOptionTruncate | File::eOpenOptionCanCreate); if (!core_file) { error = core_file.takeError(); diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h index d77f0f68cdf11..108b4ad506a3b 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h @@ -62,7 +62,7 @@ class ObjectFileMachO : public lldb_private::ObjectFile { lldb_private::ModuleSpecList &specs); static bool SaveCore(const lldb::ProcessSP &process_sp, - lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &core_options, lldb_private::Status &error); static bool MagicBytesMatch(lldb::DataBufferSP data_sp, lldb::addr_t offset, diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index a4c7f9d8cf610..cbcc616fda190 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -56,17 +56,13 @@ size_t ObjectFileMinidump::GetModuleSpecifications( } bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, - lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &core_options, lldb_private::Status &error) { - // Set default core style if it isn't set. - if (core_options.GetCoreDumpStyle() == SaveCoreStyle::eSaveCoreUnspecified) - core_options.SetCoreDumpStyle(SaveCoreStyle::eSaveCoreStackOnly); - - if (!process_sp) + if (!process_sp || !core_options.GetOutputFile()) return false; llvm::Expected<lldb::FileUP> maybe_core_file = FileSystem::Instance().Open( - core_options.GetOutputFile(), + core_options.GetOutputFile().value(), File::eOpenOptionWriteOnly | File::eOpenOptionCanCreate); if (!maybe_core_file) { error = maybe_core_file.takeError(); diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h index 81e7b3339ab39..775906b11b3a1 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h @@ -55,7 +55,7 @@ class ObjectFileMinidump : public lldb_private::PluginInterface { // Saves dump in Minidump file format static bool SaveCore(const lldb::ProcessSP &process_sp, - lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &core_options, lldb_private::Status &error); private: diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp index 4cd01d7f74bd6..7fad1d9434ad5 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp +++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp @@ -355,9 +355,8 @@ size_t ObjectFilePECOFF::GetModuleSpecifications( } bool ObjectFilePECOFF::SaveCore(const lldb::ProcessSP &process_sp, - lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &core_options, lldb_private::Status &error) { - core_options.SetCoreDumpStyle(lldb::eSaveCoreFull); return SaveMiniDump(process_sp, core_options, error); } diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h index 1074604864978..0718d5ff72cfa 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h +++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h @@ -82,7 +82,7 @@ class ObjectFilePECOFF : public lldb_private::ObjectFile { lldb_private::ModuleSpecList &specs); static bool SaveCore(const lldb::ProcessSP &process_sp, - lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &core_options, lldb_private::Status &error); static bool MagicBytesMatch(lldb::DataBufferSP data_sp); diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp index 79eaf36752a85..5932fcd70d56f 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp +++ b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp @@ -21,7 +21,7 @@ namespace lldb_private { bool SaveMiniDump(const lldb::ProcessSP &process_sp, - CoreDumpOptions &core_options, lldb_private::Status &error) { + const CoreDumpOptions &core_options, lldb_private::Status &error) { if (!process_sp) return false; #ifdef _WIN32 diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h index eaddfa17b7455..64e5011384f31 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h +++ b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h @@ -14,7 +14,7 @@ namespace lldb_private { bool SaveMiniDump(const lldb::ProcessSP &process_sp, - CoreDumpOptions &core_options, lldb_private::Status &error); + const CoreDumpOptions &core_options, lldb_private::Status &error); } // namespace lldb_private diff --git a/lldb/source/Symbol/CoreDumpOptions.cpp b/lldb/source/Symbol/CoreDumpOptions.cpp index 78e7e1a3e8617..0a8fdbb667487 100644 --- a/lldb/source/Symbol/CoreDumpOptions.cpp +++ b/lldb/source/Symbol/CoreDumpOptions.cpp @@ -11,27 +11,35 @@ using namespace lldb; using namespace lldb_private; -void CoreDumpOptions::SetCoreDumpPluginName(const llvm::StringRef name) { - m_core_dump_plugin_name = name.data(); +void CoreDumpOptions::SetCoreDumpPluginName(const char * name) { + m_plugin_name = name; } -std::optional<llvm::StringRef> CoreDumpOptions::GetCoreDumpPluginName() const { - if (!m_core_dump_plugin_name) - return std::nullopt; - return m_core_dump_plugin_name->data(); +void CoreDumpOptions::SetCoreDumpStyle(lldb::SaveCoreStyle style) { + m_style = style; } -void CoreDumpOptions::SetCoreDumpStyle(lldb::SaveCoreStyle style) { - m_core_dump_style = style; +void CoreDumpOptions::SetOutputFile(FileSpec file) { + m_file = file; +} + +std::optional<std::string> CoreDumpOptions::GetCoreDumpPluginName() const { + return m_plugin_name; } lldb::SaveCoreStyle CoreDumpOptions::GetCoreDumpStyle() const { // If unspecified, default to stack only - if (m_core_dump_style == lldb::eSaveCoreUnspecified) + if (m_style == lldb::eSaveCoreUnspecified) return lldb::eSaveCoreStackOnly; - return m_core_dump_style; + return m_style.value(); +} + +const std::optional<lldb_private::FileSpec> CoreDumpOptions::GetOutputFile() const { + return m_file; } -const lldb_private::FileSpec &CoreDumpOptions::GetOutputFile() const { - return m_core_dump_file; +void CoreDumpOptions::Clear() { + m_file = std::nullopt; + m_plugin_name = std::nullopt; + m_style = std::nullopt; } diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py index 27492756f6b4a..d4df539d28f60 100644 --- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py +++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py @@ -132,7 +132,9 @@ def test_save_linux_mini_dump(self): stacks_to_sp_map, ) - options = lldb.SBCoreDumpOptions(core_sb_stack) + options = lldb.SBCoreDumpOptions() + core_sb_stack_spec = lldb.SBFileSpec(core_sb_stack) + options.SetOutputFile(core_sb_stack_spec) options.SetCoreDumpPluginName("minidump") options.SetCoreDumpStyle(lldb.eSaveCoreStackOnly) # validate saving via SBProcess @@ -147,7 +149,9 @@ def test_save_linux_mini_dump(self): stacks_to_sp_map, ) - options = lldb.SBCoreDumpOptions(core_sb_dirty) + options = lldb.SBCoreDumpOptions() + core_sb_dirty_spec = lldb.SBFileSpec(core_sb_dirty) + options.SetOutputFile(core_sb_dirty_spec) options.SetCoreDumpPluginName("minidump") options.SetCoreDumpStyle(lldb.eSaveCoreDirtyOnly) error = process.SaveCore(options) @@ -163,7 +167,9 @@ def test_save_linux_mini_dump(self): # Minidump can now save full core files, but they will be huge and # they might cause this test to timeout. - options = lldb.SBCoreDumpOptions(core_sb_full) + options = lldb.SBCoreDumpOptions() + core_sb_full_spec = lldb.SBFileSpec(core_sb_full) + options.SetOutputFile(core_sb_full_spec) options.SetCoreDumpPluginName("minidump") options.SetCoreDumpStyle(lldb.eSaveCoreFull) error = process.SaveCore(options) >From 92bc0b39b5dd9edbb06c360b0946dc1a2b574012 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 17 Jul 2024 13:36:07 -0700 Subject: [PATCH 5/6] Code review feedback. Run git-clang-format. Run Python Format --- lldb/include/lldb/API/SBCoreDumpOptions.h | 16 ++++----- lldb/include/lldb/API/SBError.h | 1 + lldb/include/lldb/Core/PluginManager.h | 2 ++ lldb/include/lldb/Symbol/CoreDumpOptions.h | 10 +++--- lldb/include/lldb/lldb-private-interfaces.h | 2 +- lldb/source/API/SBCoreDumpOptions.cpp | 32 ++++++++--------- lldb/source/API/SBProcess.cpp | 13 +++---- lldb/source/Commands/CommandObjectProcess.cpp | 14 ++++---- lldb/source/Core/PluginManager.cpp | 18 ++++++++-- .../ObjectFile/Mach-O/ObjectFileMachO.cpp | 16 +++++---- .../ObjectFile/Mach-O/ObjectFileMachO.h | 2 +- .../Minidump/ObjectFileMinidump.cpp | 14 +++++--- .../ObjectFile/Minidump/ObjectFileMinidump.h | 2 +- .../ObjectFile/PECOFF/ObjectFilePECOFF.cpp | 5 +-- .../ObjectFile/PECOFF/ObjectFilePECOFF.h | 2 +- .../ObjectFile/PECOFF/WindowsMiniDump.cpp | 5 +-- .../ObjectFile/PECOFF/WindowsMiniDump.h | 3 +- lldb/source/Symbol/CoreDumpOptions.cpp | 36 ++++++++++++------- .../process_save_core/TestProcessSaveCore.py | 4 +-- .../TestProcessSaveCoreMinidump.py | 12 +++---- .../TestSBCoreDumpOptions.py | 25 +++++++++++++ 21 files changed, 148 insertions(+), 86 deletions(-) create mode 100644 lldb/test/API/python_api/sbcoredumpoptions/TestSBCoreDumpOptions.py diff --git a/lldb/include/lldb/API/SBCoreDumpOptions.h b/lldb/include/lldb/API/SBCoreDumpOptions.h index c9cec5b841ba3..2bbe74dcc6829 100644 --- a/lldb/include/lldb/API/SBCoreDumpOptions.h +++ b/lldb/include/lldb/API/SBCoreDumpOptions.h @@ -22,31 +22,31 @@ class LLDB_API SBCoreDumpOptions { const SBCoreDumpOptions &operator=(const lldb::SBCoreDumpOptions &rhs); - /// Set the Core dump plugin name. + /// Set the plugin name. /// /// \param plugin Name of the object file plugin. - void SetCoreDumpPluginName(const char *plugin); + SBError SetPluginName(const char *plugin); /// Get the Core dump plugin name, if set. /// /// \return The name of the plugin, or null if not set. - const char * GetCoreDumpPluginName() const; + const char *GetPluginName() const; /// Set the Core dump style. /// /// \param style The style of the core dump. - void SetCoreDumpStyle(lldb::SaveCoreStyle style); + void SetStyle(lldb::SaveCoreStyle style); /// Get the Core dump style, if set. /// /// \return The core dump style, or undefined if not set. - lldb::SaveCoreStyle GetCoreDumpStyle() const; + lldb::SaveCoreStyle GetStyle() const; /// Set the output file path /// - /// \param output_file a + /// \param output_file a /// \class SBFileSpec object that describes the output file. - void SetOutputFile(SBFileSpec &output_file); + void SetOutputFile(SBFileSpec output_file); /// Get the output file spec /// @@ -58,7 +58,7 @@ class LLDB_API SBCoreDumpOptions { protected: friend class SBProcess; - lldb_private::CoreDumpOptions &Ref() const; + lldb_private::CoreDumpOptions &ref() const; private: std::unique_ptr<lldb_private::CoreDumpOptions> m_opaque_up; diff --git a/lldb/include/lldb/API/SBError.h b/lldb/include/lldb/API/SBError.h index 1a720a479d9a6..ef5383fc033f1 100644 --- a/lldb/include/lldb/API/SBError.h +++ b/lldb/include/lldb/API/SBError.h @@ -77,6 +77,7 @@ class LLDB_API SBError { friend class SBBreakpointName; friend class SBCommandReturnObject; friend class SBCommunication; + friend class SBCoreDumpOptions; friend class SBData; friend class SBDebugger; friend class SBFile; diff --git a/lldb/include/lldb/Core/PluginManager.h b/lldb/include/lldb/Core/PluginManager.h index 297381fa911e6..bb5a360059d21 100644 --- a/lldb/include/lldb/Core/PluginManager.h +++ b/lldb/include/lldb/Core/PluginManager.h @@ -178,6 +178,8 @@ class PluginManager { static bool UnregisterPlugin(ObjectFileCreateInstance create_callback); + static bool IsRegisteredPluginName(const char *name); + static ObjectFileCreateInstance GetObjectFileCreateCallbackAtIndex(uint32_t idx); diff --git a/lldb/include/lldb/Symbol/CoreDumpOptions.h b/lldb/include/lldb/Symbol/CoreDumpOptions.h index 75d70a5967c9f..544d7704eb2b5 100644 --- a/lldb/include/lldb/Symbol/CoreDumpOptions.h +++ b/lldb/include/lldb/Symbol/CoreDumpOptions.h @@ -20,14 +20,14 @@ namespace lldb_private { class CoreDumpOptions { public: - CoreDumpOptions() {}; + CoreDumpOptions(){}; ~CoreDumpOptions() = default; - void SetCoreDumpPluginName(const char * name); - std::optional<std::string> GetCoreDumpPluginName() const; + lldb_private::Status SetPluginName(const char *name); + std::optional<std::string> GetPluginName() const; - void SetCoreDumpStyle(lldb::SaveCoreStyle style); - lldb::SaveCoreStyle GetCoreDumpStyle() const; + void SetStyle(lldb::SaveCoreStyle style); + lldb::SaveCoreStyle GetStyle() const; void SetOutputFile(lldb_private::FileSpec file); const std::optional<lldb_private::FileSpec> GetOutputFile() const; diff --git a/lldb/include/lldb/lldb-private-interfaces.h b/lldb/include/lldb/lldb-private-interfaces.h index 044a57f87d492..985040b98d848 100644 --- a/lldb/include/lldb/lldb-private-interfaces.h +++ b/lldb/include/lldb/lldb-private-interfaces.h @@ -56,7 +56,7 @@ typedef ObjectFile *(*ObjectFileCreateMemoryInstance)( const lldb::ModuleSP &module_sp, lldb::WritableDataBufferSP data_sp, const lldb::ProcessSP &process_sp, lldb::addr_t offset); typedef bool (*ObjectFileSaveCore)(const lldb::ProcessSP &process_sp, - const lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &options, Status &error); typedef EmulateInstruction *(*EmulateInstructionCreateInstance)( const ArchSpec &arch, InstructionType inst_type); diff --git a/lldb/source/API/SBCoreDumpOptions.cpp b/lldb/source/API/SBCoreDumpOptions.cpp index fc00430a2809f..2f572d4a8a3ad 100644 --- a/lldb/source/API/SBCoreDumpOptions.cpp +++ b/lldb/source/API/SBCoreDumpOptions.cpp @@ -6,8 +6,9 @@ // //===----------------------------------------------------------------------===// -#include "lldb/API/SBFileSpec.h" #include "lldb/API/SBCoreDumpOptions.h" +#include "lldb/API/SBError.h" +#include "lldb/API/SBFileSpec.h" #include "lldb/Host/FileSystem.h" #include "lldb/Symbol/CoreDumpOptions.h" #include "lldb/Utility/Instrumentation.h" @@ -17,6 +18,8 @@ using namespace lldb; SBCoreDumpOptions::SBCoreDumpOptions() { + LLDB_INSTRUMENT_VA(this) + m_opaque_up = std::make_unique<lldb_private::CoreDumpOptions>(); } @@ -35,21 +38,21 @@ SBCoreDumpOptions::operator=(const SBCoreDumpOptions &rhs) { return *this; } -void SBCoreDumpOptions::SetCoreDumpPluginName(const char *name) { - m_opaque_up->SetCoreDumpPluginName(name); +SBError SBCoreDumpOptions::SetPluginName(const char *name) { + lldb_private::Status error = m_opaque_up->SetPluginName(name); + return SBError(error); } -void SBCoreDumpOptions::SetCoreDumpStyle(lldb::SaveCoreStyle style) { - m_opaque_up->SetCoreDumpStyle(style); +void SBCoreDumpOptions::SetStyle(lldb::SaveCoreStyle style) { + m_opaque_up->SetStyle(style); } -void SBCoreDumpOptions::SetOutputFile(lldb::SBFileSpec &file_spec) { +void SBCoreDumpOptions::SetOutputFile(lldb::SBFileSpec file_spec) { m_opaque_up->SetOutputFile(file_spec.ref()); } -const char * -SBCoreDumpOptions::GetCoreDumpPluginName() const { - const auto name = m_opaque_up->GetCoreDumpPluginName(); +const char *SBCoreDumpOptions::GetPluginName() const { + const auto name = m_opaque_up->GetPluginName(); if (!name) return nullptr; return lldb_private::ConstString(name.value()).GetCString(); @@ -62,15 +65,12 @@ SBFileSpec SBCoreDumpOptions::GetOutputFile() const { return SBFileSpec(); } -lldb::SaveCoreStyle -SBCoreDumpOptions::GetCoreDumpStyle() const { - return m_opaque_up->GetCoreDumpStyle(); +lldb::SaveCoreStyle SBCoreDumpOptions::GetStyle() const { + return m_opaque_up->GetStyle(); } -lldb_private::CoreDumpOptions &SBCoreDumpOptions::Ref() const { +lldb_private::CoreDumpOptions &SBCoreDumpOptions::ref() const { return *m_opaque_up.get(); } -void SBCoreDumpOptions::Clear() { - m_opaque_up->Clear(); -} +void SBCoreDumpOptions::Clear() { m_opaque_up->Clear(); } diff --git a/lldb/source/API/SBProcess.cpp b/lldb/source/API/SBProcess.cpp index a5a8190d97180..63fc1b28f42b8 100644 --- a/lldb/source/API/SBProcess.cpp +++ b/lldb/source/API/SBProcess.cpp @@ -1223,19 +1223,16 @@ lldb::SBError SBProcess::SaveCore(const char *file_name) { lldb::SBError SBProcess::SaveCore(const char *file_name, const char *flavor, SaveCoreStyle core_style) { - SBFileSpec fspec(file_name); SBCoreDumpOptions options; - options.SetOutputFile(fspec); - options.SetCoreDumpPluginName(flavor); - options.SetCoreDumpStyle(core_style); + options.SetOutputFile(SBFileSpec(file_name)); + options.SetPluginName(flavor); + options.SetStyle(core_style); return SaveCore(options); } lldb::SBError SBProcess::SaveCore(SBCoreDumpOptions &options) { - LLDB_INSTRUMENT_VA(this, options.GetOutputFile(), - options.GetCoreDumpPluginName(), - options.GetCoreDumpStyle()); + LLDB_INSTRUMENT_VA(this, options); lldb::SBError error; ProcessSP process_sp(GetSP()); @@ -1252,7 +1249,7 @@ lldb::SBError SBProcess::SaveCore(SBCoreDumpOptions &options) { return error; } - error.ref() = PluginManager::SaveCore(process_sp, options.Ref()); + error.ref() = PluginManager::SaveCore(process_sp, options.ref()); return error; } diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp index fedc12b4232fe..232e8e22dfcf8 100644 --- a/lldb/source/Commands/CommandObjectProcess.cpp +++ b/lldb/source/Commands/CommandObjectProcess.cpp @@ -1256,7 +1256,7 @@ class CommandObjectProcessSaveCore : public CommandObjectParsed { class CommandOptions : public Options { public: - CommandOptions() : m_core_dump_options() {}; + CommandOptions(){}; ~CommandOptions() override = default; @@ -1271,10 +1271,10 @@ class CommandObjectProcessSaveCore : public CommandObjectParsed { switch (short_option) { case 'p': - m_core_dump_options.SetCoreDumpPluginName(option_arg.data()); + m_core_dump_options.SetPluginName(option_arg.data()); break; case 's': - m_core_dump_options.SetCoreDumpStyle( + m_core_dump_options.SetStyle( (lldb::SaveCoreStyle)OptionArgParser::ToOptionEnum( option_arg, GetDefinitions()[option_idx].enum_values, eSaveCoreUnspecified, error)); @@ -1301,12 +1301,14 @@ class CommandObjectProcessSaveCore : public CommandObjectParsed { if (command.GetArgumentCount() == 1) { FileSpec output_file(command.GetArgumentAtIndex(0)); FileSystem::Instance().Resolve(output_file); - auto core_dump_options = m_options.m_core_dump_options; + auto &core_dump_options = m_options.m_core_dump_options; core_dump_options.SetOutputFile(output_file); Status error = PluginManager::SaveCore(process_sp, core_dump_options); if (error.Success()) { - if (core_dump_options.GetCoreDumpStyle() == SaveCoreStyle::eSaveCoreDirtyOnly || - core_dump_options.GetCoreDumpStyle() == SaveCoreStyle::eSaveCoreStackOnly) { + if (core_dump_options.GetStyle() == + SaveCoreStyle::eSaveCoreDirtyOnly || + core_dump_options.GetStyle() == + SaveCoreStyle::eSaveCoreStackOnly) { result.AppendMessageWithFormat( "\nModified-memory or stack-memory only corefile " "created. This corefile may \n" diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp index c0a0630cccb9e..7df8a048253c9 100644 --- a/lldb/source/Core/PluginManager.cpp +++ b/lldb/source/Core/PluginManager.cpp @@ -640,6 +640,18 @@ static ObjectFileInstances &GetObjectFileInstances() { return g_instances; } +bool PluginManager::IsRegisteredPluginName(const char *name) { + if (!name || !name[0]) + return false; + + const auto &instances = GetObjectFileInstances().GetInstances(); + for (auto &instance : instances) { + if (instance.name == name) + return true; + } + return false; +} + bool PluginManager::RegisterPlugin( llvm::StringRef name, llvm::StringRef description, ObjectFileCreateInstance create_callback, @@ -697,7 +709,7 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp, return error; } - if (options.GetCoreDumpPluginName()->empty()) { + if (!options.GetPluginName().has_value()) { // Try saving core directly from the process plugin first. llvm::Expected<bool> ret = process_sp->SaveCore(options.GetOutputFile()->GetPath()); @@ -708,10 +720,10 @@ Status PluginManager::SaveCore(const lldb::ProcessSP &process_sp, } // Fall back to object plugins. + const auto &plugin_name = options.GetPluginName().value_or(""); auto &instances = GetObjectFileInstances().GetInstances(); for (auto &instance : instances) { - if (options.GetCoreDumpPluginName()->empty() || - instance.name == options.GetCoreDumpPluginName()) { + if (plugin_name.empty() || instance.name == plugin_name) { if (instance.save_core && instance.save_core(process_sp, options, error)) return error; } diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp index 23183ada04b28..4b8cc7546bf6e 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp @@ -6519,11 +6519,15 @@ struct page_object { }; bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp, - const lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &options, Status &error) { - auto core_style = core_options.GetCoreDumpStyle(); - const auto outfile = core_options.GetOutputFile(); - if (!process_sp || !outfile) + auto core_style = options.GetStyle(); + if (core_style == SaveCoreStyle::eSaveCoreUnspecified) + core_style = SaveCoreStyle::eSaveCoreDirtyOnly; + // The FileSpec is already checked in PluginManager::SaveCore. + assert(options.GetOutputFile().has_value()); + const FileSpec outfile = options.GetOutputFile().value(); + if (!process_sp) return false; Target &target = process_sp->GetTarget(); @@ -6809,9 +6813,9 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp, buffer.PutHex32(segment.flags); } - std::string core_file_path(core_options.GetOutputFile()->GetPath()); + std::string core_file_path(outfile.GetPath()); auto core_file = FileSystem::Instance().Open( - outfile.value(), File::eOpenOptionWriteOnly | File::eOpenOptionTruncate | + outfile, File::eOpenOptionWriteOnly | File::eOpenOptionTruncate | File::eOpenOptionCanCreate); if (!core_file) { error = core_file.takeError(); diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h index 108b4ad506a3b..540597b58f9e7 100644 --- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h +++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h @@ -62,7 +62,7 @@ class ObjectFileMachO : public lldb_private::ObjectFile { lldb_private::ModuleSpecList &specs); static bool SaveCore(const lldb::ProcessSP &process_sp, - const lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &options, lldb_private::Status &error); static bool MagicBytesMatch(lldb::DataBufferSP data_sp, lldb::addr_t offset, diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp index cbcc616fda190..9c9fbc4d54e0d 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp @@ -56,13 +56,19 @@ size_t ObjectFileMinidump::GetModuleSpecifications( } bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, - const lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &options, lldb_private::Status &error) { - if (!process_sp || !core_options.GetOutputFile()) + assert(options.GetOutputFile().has_value()); + if (!process_sp) return false; + // Minidump defaults to stacks only. + SaveCoreStyle core_style = options.GetStyle(); + if (core_style == SaveCoreStyle::eSaveCoreUnspecified) + core_style = SaveCoreStyle::eSaveCoreStackOnly; + llvm::Expected<lldb::FileUP> maybe_core_file = FileSystem::Instance().Open( - core_options.GetOutputFile().value(), + options.GetOutputFile().value(), File::eOpenOptionWriteOnly | File::eOpenOptionCanCreate); if (!maybe_core_file) { error = maybe_core_file.takeError(); @@ -115,7 +121,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp, // Note: add memory HAS to be the last thing we do. It can overflow into 64b // land and many RVA's only support 32b - error = builder.AddMemoryList(core_options.GetCoreDumpStyle()); + error = builder.AddMemoryList(core_style); if (error.Fail()) { LLDB_LOGF(log, "AddMemoryList failed: %s", error.AsCString()); return false; diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h index 775906b11b3a1..df5727785dc4b 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h +++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h @@ -55,7 +55,7 @@ class ObjectFileMinidump : public lldb_private::PluginInterface { // Saves dump in Minidump file format static bool SaveCore(const lldb::ProcessSP &process_sp, - const lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &options, lldb_private::Status &error); private: diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp index 7fad1d9434ad5..05b2c675dea4c 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp +++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp @@ -355,9 +355,10 @@ size_t ObjectFilePECOFF::GetModuleSpecifications( } bool ObjectFilePECOFF::SaveCore(const lldb::ProcessSP &process_sp, - const lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &options, lldb_private::Status &error) { - return SaveMiniDump(process_sp, core_options, error); + assert(options.GetOutputFile().has_value()); + return SaveMiniDump(process_sp, options, error); } bool ObjectFilePECOFF::MagicBytesMatch(DataBufferSP data_sp) { diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h index 0718d5ff72cfa..da71323c89e47 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h +++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h @@ -82,7 +82,7 @@ class ObjectFilePECOFF : public lldb_private::ObjectFile { lldb_private::ModuleSpecList &specs); static bool SaveCore(const lldb::ProcessSP &process_sp, - const lldb_private::CoreDumpOptions &core_options, + const lldb_private::CoreDumpOptions &options, lldb_private::Status &error); static bool MagicBytesMatch(lldb::DataBufferSP data_sp); diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp index 5932fcd70d56f..ff99ea3778511 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp +++ b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp @@ -21,11 +21,12 @@ namespace lldb_private { bool SaveMiniDump(const lldb::ProcessSP &process_sp, - const CoreDumpOptions &core_options, lldb_private::Status &error) { + const CoreDumpOptions &core_options, + lldb_private::Status &error) { if (!process_sp) return false; #ifdef _WIN32 - const auto outfile = core_options.GetOutputFile(); + const auto &outfile = core_options.GetOutputFile().value(); HANDLE process_handle = ::OpenProcess( PROCESS_QUERY_INFORMATION | PROCESS_VM_READ, FALSE, process_sp->GetID()); const std::string file_name = outfile.GetPath(); diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h index 64e5011384f31..7501bd8683d57 100644 --- a/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h +++ b/lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.h @@ -14,7 +14,8 @@ namespace lldb_private { bool SaveMiniDump(const lldb::ProcessSP &process_sp, - const CoreDumpOptions &core_options, lldb_private::Status &error); + const CoreDumpOptions &core_options, + lldb_private::Status &error); } // namespace lldb_private diff --git a/lldb/source/Symbol/CoreDumpOptions.cpp b/lldb/source/Symbol/CoreDumpOptions.cpp index 0a8fdbb667487..728eaffeabfd9 100644 --- a/lldb/source/Symbol/CoreDumpOptions.cpp +++ b/lldb/source/Symbol/CoreDumpOptions.cpp @@ -7,34 +7,44 @@ //===----------------------------------------------------------------------===// #include "lldb/Symbol/CoreDumpOptions.h" +#include "lldb/Core/PluginManager.h" using namespace lldb; using namespace lldb_private; -void CoreDumpOptions::SetCoreDumpPluginName(const char * name) { +Status CoreDumpOptions::SetPluginName(const char *name) { + Status error; + if (!name || !name[0]) { + m_plugin_name = std::nullopt; + error.SetErrorString("no plugin name specified"); + } + + if (!PluginManager::IsRegisteredPluginName(name)) { + error.SetErrorStringWithFormat( + "plugin name '%s' is not a registered plugin", name); + return error; + } + m_plugin_name = name; + return error; } -void CoreDumpOptions::SetCoreDumpStyle(lldb::SaveCoreStyle style) { - m_style = style; -} +void CoreDumpOptions::SetStyle(lldb::SaveCoreStyle style) { m_style = style; } -void CoreDumpOptions::SetOutputFile(FileSpec file) { - m_file = file; -} +void CoreDumpOptions::SetOutputFile(FileSpec file) { m_file = file; } -std::optional<std::string> CoreDumpOptions::GetCoreDumpPluginName() const { +std::optional<std::string> CoreDumpOptions::GetPluginName() const { return m_plugin_name; } -lldb::SaveCoreStyle CoreDumpOptions::GetCoreDumpStyle() const { - // If unspecified, default to stack only - if (m_style == lldb::eSaveCoreUnspecified) - return lldb::eSaveCoreStackOnly; +lldb::SaveCoreStyle CoreDumpOptions::GetStyle() const { + if (!m_style.has_value()) + return lldb::eSaveCoreUnspecified; return m_style.value(); } -const std::optional<lldb_private::FileSpec> CoreDumpOptions::GetOutputFile() const { +const std::optional<lldb_private::FileSpec> +CoreDumpOptions::GetOutputFile() const { return m_file; } diff --git a/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py b/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py index b6406579f3c55..c136739a2fb74 100644 --- a/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py +++ b/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py @@ -2,7 +2,6 @@ Test saving a core file (or mini dump). """ - import os import lldb from lldbsuite.test.decorators import * @@ -21,7 +20,8 @@ def test_cannot_save_core_unless_process_stopped(self): target = self.dbg.CreateTarget(exe) process = target.LaunchSimple(None, None, self.get_process_working_directory()) self.assertNotEqual(process.GetState(), lldb.eStateStopped) - options = SBCoreDumpOptions(core) + options = SBCoreDumpOptions() + options.SetOutputFile(SBFileSpec(core)) error = process.SaveCore(core) self.assertTrue(error.Fail()) diff --git a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py index d4df539d28f60..20096d7b4d8d6 100644 --- a/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py +++ b/lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py @@ -135,8 +135,8 @@ def test_save_linux_mini_dump(self): options = lldb.SBCoreDumpOptions() core_sb_stack_spec = lldb.SBFileSpec(core_sb_stack) options.SetOutputFile(core_sb_stack_spec) - options.SetCoreDumpPluginName("minidump") - options.SetCoreDumpStyle(lldb.eSaveCoreStackOnly) + options.SetPluginName("minidump") + options.SetStyle(lldb.eSaveCoreStackOnly) # validate saving via SBProcess error = process.SaveCore(options) self.assertTrue(error.Success()) @@ -152,8 +152,8 @@ def test_save_linux_mini_dump(self): options = lldb.SBCoreDumpOptions() core_sb_dirty_spec = lldb.SBFileSpec(core_sb_dirty) options.SetOutputFile(core_sb_dirty_spec) - options.SetCoreDumpPluginName("minidump") - options.SetCoreDumpStyle(lldb.eSaveCoreDirtyOnly) + options.SetPluginName("minidump") + options.SetStyle(lldb.eSaveCoreDirtyOnly) error = process.SaveCore(options) self.assertTrue(error.Success()) self.assertTrue(os.path.isfile(core_sb_dirty)) @@ -170,8 +170,8 @@ def test_save_linux_mini_dump(self): options = lldb.SBCoreDumpOptions() core_sb_full_spec = lldb.SBFileSpec(core_sb_full) options.SetOutputFile(core_sb_full_spec) - options.SetCoreDumpPluginName("minidump") - options.SetCoreDumpStyle(lldb.eSaveCoreFull) + options.SetPluginName("minidump") + options.SetStyle(lldb.eSaveCoreFull) error = process.SaveCore(options) self.assertTrue(error.Success()) self.assertTrue(os.path.isfile(core_sb_full)) diff --git a/lldb/test/API/python_api/sbcoredumpoptions/TestSBCoreDumpOptions.py b/lldb/test/API/python_api/sbcoredumpoptions/TestSBCoreDumpOptions.py new file mode 100644 index 0000000000000..c9232a56bb768 --- /dev/null +++ b/lldb/test/API/python_api/sbcoredumpoptions/TestSBCoreDumpOptions.py @@ -0,0 +1,25 @@ +"""Test the SBCoreDumpOptions APIs.""" + +import lldb +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * + + +class SBCoreDumpOptionsAPICase(TestBase): + def test_plugin_name_assignment(self): + """Test""" + options = lldb.SBCoreDumpOptions() + error = options.SetPluginName(None) + self.assertTrue(error.Fail()) + self.assertEqual(options.GetPluginName(), None) + error = options.SetPluginName("Not a real plugin") + self.assertTrue(error.Fail()) + self.assertEqual(options.GetPluginName(), None) + error = options.SetPluginName("minidump") + self.assertTrue(error.Success()) + self.assertEqual(options.GetPluginName(), "minidump") + + def test_default_corestyle_behavior(self): + """Test that the default core style is unspecified.""" + options = lldb.SBCoreDumpOptions() + self.assertEqual(options.GetStyle(), lldb.eSaveCoreUnspecified) >From d4d702dec960cf55af046653d1153876f05ff643 Mon Sep 17 00:00:00 2001 From: Jacob Lalonde <jalalo...@fb.com> Date: Wed, 17 Jul 2024 13:46:15 -0700 Subject: [PATCH 6/6] Revert TestMinidump.py as it only had a line added, and assign the error output of SetPluginName in CommandOptions for savecore --- lldb/source/Commands/CommandObjectProcess.cpp | 2 +- .../API/functionalities/postmortem/minidump/TestMiniDump.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/lldb/source/Commands/CommandObjectProcess.cpp b/lldb/source/Commands/CommandObjectProcess.cpp index 232e8e22dfcf8..dbe8035af2a8c 100644 --- a/lldb/source/Commands/CommandObjectProcess.cpp +++ b/lldb/source/Commands/CommandObjectProcess.cpp @@ -1271,7 +1271,7 @@ class CommandObjectProcessSaveCore : public CommandObjectParsed { switch (short_option) { case 'p': - m_core_dump_options.SetPluginName(option_arg.data()); + error = m_core_dump_options.SetPluginName(option_arg.data()); break; case 's': m_core_dump_options.SetStyle( diff --git a/lldb/test/API/functionalities/postmortem/minidump/TestMiniDump.py b/lldb/test/API/functionalities/postmortem/minidump/TestMiniDump.py index 101950da16d36..8fe5d2a1d8562 100644 --- a/lldb/test/API/functionalities/postmortem/minidump/TestMiniDump.py +++ b/lldb/test/API/functionalities/postmortem/minidump/TestMiniDump.py @@ -130,7 +130,6 @@ def test_deeper_stack_in_mini_dump(self): process = target.LaunchSimple( None, None, self.get_process_working_directory() ) - self.assertState(process.GetState(), lldb.eStateStopped) self.assertTrue(process.SaveCore(core)) self.assertTrue(os.path.isfile(core)) _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
