[LLDB][SaveCore] Add SBSaveCoreOptions Object, and SBProcess::SaveCore() overload #98403
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesThis PR adds Patch is 29.05 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/98403.diff 28 Files Affected:
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
new file mode 100644
index 0000000000000..48baf448492b4
--- /dev/null
+++ b/lldb/include/lldb/API/SBCoreDumpOptions.h
@@ -0,0 +1,59 @@
+//===-- 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
+
+#include "lldb/API/SBDefines.h"
+#include "lldb/Symbol/CoreDumpOptions.h"
+
+namespace lldb {
+
+class LLDB_API SBCoreDumpOptions {
+public:
+ SBCoreDumpOptions(const char *filePath);
+ SBCoreDumpOptions(const lldb::SBCoreDumpOptions &rhs);
+ ~SBCoreDumpOptions() = default;
+
+ const SBCoreDumpOptions &operator=(const lldb::SBCoreDumpOptions &rhs);
+
+ /// 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;
+
+ /// Get the output file path
+ ///
+ /// \return The output file path.
+ const char *GetOutputFile() const;
+
+protected:
+ friend class SBProcess;
+ lldb_private::CoreDumpOptions &Ref() const;
+
+private:
+ std::unique_ptr<lldb_private::CoreDumpOptions> m_opaque_up;
+}; // SBCoreDumpOptions
+} // 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/include/lldb/Symbol/CoreDumpOptions.h b/lldb/include/lldb/Symbol/CoreDumpOptions.h
new file mode 100644
index 0000000000000..b5482bcf24359
--- /dev/null
+++ b/lldb/include/lldb/Symbol/CoreDumpOptions.h
@@ -0,0 +1,42 @@
+//===-- 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 "lldb/Utility/FileSpec.h"
+#include "lldb/lldb-forward.h"
+#include "lldb/lldb-types.h"
+
+#include <optional>
+#include <string>
+
+namespace lldb_private {
+
+class CoreDumpOptions {
+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;
+
+ void SetCoreDumpStyle(lldb::SaveCoreStyle style);
+ lldb::SaveCoreStyle GetCoreDumpStyle() const;
+
+ const lldb_private::FileSpec &GetOutputFile() const;
+
+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;
+};
+} // 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
new file mode 100644
index 0000000000000..944f44e75039b
--- /dev/null
+++ b/lldb/source/API/SBCoreDumpOptions.cpp
@@ -0,0 +1,67 @@
+//===-- 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"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Symbol/CoreDumpOptions.h"
+#include "lldb/Utility/Instrumentation.h"
+
+#include "Utils.h"
+
+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(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..436b615bab920 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,17 @@ 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 +1250,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..f17c50ee22ac3 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;
- Status error =
- PluginManager::SaveCore(process_sp, output_file, corefile_style,
- m_options.m_requested_plugin_name);
+ 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, 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..90231af030d90 100644
--- a/lldb/source/Core/PluginManager.cpp
+++ b/lldb/source/Core/PluginManager.cpp
@@ -12,6 +12,7 @@
#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"
@@ -689,12 +690,11 @@ 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,14 +705,18 @@ 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 (instance.save_core &&
- instance.save_core(process_sp, outfile, core_style, error))
+ if (options.GetCoreDumpPluginName()->empty() ||
+ instance.name == options.GetCoreDumpPluginName()) {
+ if (instance.save_core && 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;
}
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 0dcb1bed23548..bfb1d6d69f002 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6519,8 +6519,10 @@ struct page_object {
};
bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
- const FileSpec &outfile,
- lldb::SaveCoreStyle &core_style, Status &error) {
+ 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..a4c7f9d8cf610 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -56,18 +56,18 @@ 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 +119,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 : pu...
[truncated]
|
You can test this locally with the following command:git-clang-format --diff 2a086dce691e3cc34a2fc27f4fb255bb2cbbfac9 d555d849d6368fffd909154fbbfbbf3d17377b3c --extensions cpp,h -- lldb/include/lldb/API/SBSaveCoreOptions.h lldb/include/lldb/Symbol/SaveCoreOptions.h lldb/source/API/SBSaveCoreOptions.cpp lldb/source/Symbol/SaveCoreOptions.cpp lldb/include/lldb/API/LLDB.h lldb/include/lldb/API/SBDefines.h lldb/include/lldb/API/SBError.h lldb/include/lldb/API/SBFileSpec.h lldb/include/lldb/API/SBProcess.h lldb/include/lldb/Core/PluginManager.h lldb/include/lldb/lldb-private-interfaces.h lldb/source/API/SBProcess.cpp lldb/source/Commands/CommandObjectProcess.cpp lldb/source/Core/PluginManager.cpp lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.h lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.cpp lldb/source/Plugins/ObjectFile/PECOFF/WindowsMiniDump.hView the diff from clang-format here.diff --git a/lldb/include/lldb/Symbol/SaveCoreOptions.h b/lldb/include/lldb/Symbol/SaveCoreOptions.h
index 583bc1f483..34b53b7688 100644
--- a/lldb/include/lldb/Symbol/SaveCoreOptions.h
+++ b/lldb/include/lldb/Symbol/SaveCoreOptions.h
@@ -20,7 +20,7 @@ namespace lldb_private {
class SaveCoreOptions {
public:
- SaveCoreOptions(){};
+ SaveCoreOptions() {};
~SaveCoreOptions() = default;
lldb_private::Status SetPluginName(const char *name);
|
|
I only looked briefly, but the first thing I noticed was the std::optional return types. None of the SB API methods return any STL types/containers, like the std::optionals returned in this PR. I worry if this will make the bridging to other languages more complicated, but it's never something I've thought about before. @jimingham @bulbazord @clayborg @JDevlieghere may have a more informed opinion on whether this is important or not. |
Yeah, we've been discussing introducing an Besides that, I've also noticed that your classes are named |
@clayborg actually mentioned this when I started work originally that we should avoid STL types. It seems I forgot while figuring out SWIG. As for |
|
I prefer CoreDumpOptions, because CoreOptions could mean "options for reading in core files" or "options for writing core files", but CoreDumpOptions - reading the Dump as a verb - resolves the ambiguity. Or you could use SaveCoreOptions if you want to be really explicit. We already use lldb::SaveCoreStyle, so that's a fairly consistent naming. |
clayborg
requested changes
Jul 11, 2024
| /// 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; |
There was a problem hiding this comment.
Jason is right: no std::optional in the public API. The std::optional should only be used in the internal lldb_private::CoreDumpOptions and the accessors for lldb_private::CoreDumpOptions should not return std::optional either, it should use it to return something like:
return m_optional.value_or(<default-value>);
The std::optional stuff is there just so we know when a user has set an option value in a command or on via the API. If the value hasn't been set, then we return a good default value.
This function should return NULL if there is no value.
| /// Get the Core dump style, if set. | ||
| /// | ||
| /// \return The core dump style, or nullopt if not set. | ||
| const std::optional<lldb::SaveCoreStyle> GetCoreDumpStyle() const; |
There was a problem hiding this comment.
return lldb::SaveCoreStyle with no optional. lldb_private::CoreDumpOptions::GetCoreDumpStyle() should return a good default value for this, and that will be returned as the result of this function.
| /// Get the output file path | ||
| /// | ||
| /// \return The output file path. | ||
| const char *GetOutputFile() const; |
There was a problem hiding this comment.
Return a lldb::SBFileSpec here instead of a const char *.
We also need a way to set this output file value as well, so add an accessor:
void SetOutputFile(lldb::SBFileSpec &file);
| const FileSpec &outfile, | ||
| lldb::SaveCoreStyle &core_style, | ||
| llvm::StringRef plugin_name); | ||
| lldb_private::CoreDumpOptions &core_options); |
There was a problem hiding this comment.
change to const: const lldb_private::CoreDumpOptions &core_options
There was a problem hiding this comment.
If we do this then we will need to remove all the current mutation of style in MachO, ELF, Minidump. (Or keep them as local variables, but in that case then it's confusing that a plugin could mutate your options and you woudln't know.
If we want to remove all the default settings per flavor in this patch let me know.
| ~CoreDumpOptions() = default; | ||
|
|
||
| void SetCoreDumpPluginName(llvm::StringRef name); | ||
| std::optional<llvm::StringRef> GetCoreDumpPluginName() const; |
There was a problem hiding this comment.
return an std::optional<std::string> to match how it is stored in the instance variable.
| return m_core_dump_style; | ||
| } | ||
|
|
||
| const lldb_private::FileSpec &CoreDumpOptions::GetOutputFile() const { |
There was a problem hiding this comment.
Add a SetOutputFile(const lldb_private::FileSpec &f) accessor
| @@ -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) | |||
There was a problem hiding this comment.
I would make this default construct and call the SetOutputFile accessor:
options = lldb.SBCoreDumpOptions()
options.SetOutputFile(core)
|
|
||
| class LLDB_API SBCoreDumpOptions { | ||
| public: | ||
| SBCoreDumpOptions(const char *filePath); |
There was a problem hiding this comment.
Remove the file path from constructor. Clients should use the SBCoreDumpOptions::SetOutputFile() accessor.
|
|
||
| class CoreDumpOptions { | ||
| public: | ||
| CoreDumpOptions(const lldb_private::FileSpec &fspec) |
There was a problem hiding this comment.
Remove the FileSpec from the constructor and add a accessor to set the output file.
bulbazord
reviewed
Jul 11, 2024
| std::optional<llvm::StringRef> CoreDumpOptions::GetCoreDumpPluginName() const { | ||
| if (!m_core_dump_plugin_name) | ||
| return std::nullopt; | ||
| return m_core_dump_plugin_name->data(); |
There was a problem hiding this comment.
No need to call data() right? Can you not make a StringRef from a std::string? This would incur an additional strlen
…viors in each flavor of save core
clayborg
requested changes
Jul 17, 2024
Comment on lines
31
to
33
| // If unspecified, default to stack only | ||
| if (m_style == lldb::eSaveCoreUnspecified) | ||
| return lldb::eSaveCoreStackOnly; |
There was a problem hiding this comment.
We need to make sure people are ok with us switching the default. The only one that will care if the MachO core file. I tagged @jasonmolenda in a comment asking him (Apple core file expert) if the default switch is ok.
There was a problem hiding this comment.
@jasonmolenda I think we should keep the default behaviors for this diff (as it's getting fairly big), this does mean we'll need a non const options object being passed around. With the const-ness being the only reason I removed the default behaviors.
|
|
||
| lldb::SaveCoreStyle CoreDumpOptions::GetCoreDumpStyle() const { | ||
| // If unspecified, default to stack only | ||
| if (m_style == lldb::eSaveCoreUnspecified) |
There was a problem hiding this comment.
m_style is an optional, it might not have a value. So you need to do:
if (!m_style.has_value() || m_style.value() == lldb::eSaveCoreUnspecified)
return lldb::eSaveCoreStackOnly;
return m_style.value();
Other wise this will crash if we call m_style.value() when it has no value. Add a test for this.
lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py
Outdated
Show resolved
Hide resolved
lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
Outdated
Show resolved
Hide resolved
Jlalond
commented
Jul 17, 2024
…ror output of SetPluginName in CommandOptions for savecore
…, instrument all methods in SBCoreDumpOptions
clayborg
reviewed
Jul 18, 2024
clayborg
approved these changes
Jul 18, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/2381 Here is the relevant piece of the build log for the reference: |
keith
added a commit
that referenced
this pull request
Jul 19, 2024
keith
added a commit
to keith/llvm-project
that referenced
this pull request
Jul 19, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/1824 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/1036 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/897 Here is the relevant piece of the build log for the reference: |
|
Reverted locally and still got consistent failures. @jimingham should I file an issue here? |
|
Were those the same failures that were showing up on the buildbots? If not it could just be that there's an issue with your build configuration. In this case, the fix was pretty simple (see a27037b), but generally, if don't know what's the problem and you get a clear signal that this is a real failure and not a fluke (three builtbots going red at the same time is definitely sufficient signal), it's best to revert and figure out the problem later. |
|
lldb-aarch64-windows is still failing: https://lab.llvm.org/buildbot/#/builders/141/builds/912 |
Hey @labath appreciate the advice, and the help on forward fixing the issue! @luporl I will look into Windows today. |
|
Addressed Windows issue in #99692 |
Jlalond
added a commit
that referenced
this pull request
Jul 19, 2024
In #98403 some of the tests were transitioned to the new `SBProcess::SaveCore(SBSaveCoreOptions)` API, but were not detected in testing. This patch addresses that.
sgundapa
pushed a commit
to sgundapa/upstream_effort
that referenced
this pull request
Jul 23, 2024
…e() overload (llvm#98403) This PR adds `SBSaveCoreOptions`, which is a container class for options when LLDB is taking coredumps. For this first iteration this container just keeps parity with the extant API of `file, style, plugin`. In the future this options object can be extended to allow users to take a subset of their core dumps.
sgundapa
pushed a commit
to sgundapa/upstream_effort
that referenced
this pull request
Jul 23, 2024
sgundapa
pushed a commit
to sgundapa/upstream_effort
that referenced
this pull request
Jul 23, 2024
In llvm#98403 some of the tests were transitioned to the new `SBProcess::SaveCore(SBSaveCoreOptions)` API, but were not detected in testing. This patch addresses that.
| lldb_private::Status &error) { | ||
| if (!process_sp) | ||
| return false; | ||
| #ifdef _WIN32 | ||
| const auto &outfile = core_options.GetOutputFile().value(); |
There was a problem hiding this comment.
- This is unused but I assume it will be used later?
- This warning is produced in a clang-cl build on Windows:
C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\source\Plugins\ObjectFile\PECOFF\WindowsMiniDump.cpp(29,25): warning: object backing the pointer will be destroyed at the end of the full-expression [-Wdangling-gsl]
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
…e() overload (#98403) Summary: This PR adds `SBSaveCoreOptions`, which is a container class for options when LLDB is taking coredumps. For this first iteration this container just keeps parity with the extant API of `file, style, plugin`. In the future this options object can be extended to allow users to take a subset of their core dumps. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251387
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251432
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
Summary: In #98403 some of the tests were transitioned to the new `SBProcess::SaveCore(SBSaveCoreOptions)` API, but were not detected in testing. This patch addresses that. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251264
Jlalond
added a commit
that referenced
this pull request
Aug 2, 2024
#100443) In #98403 I enabled the SBSaveCoreOptions object, which allows users via the scripting API to define what they want saved into their core file. As the first option I've added a threadlist, so users can scan and identify which threads and corresponding stacks they want to save. In order to support this, I had to add a new method to `Process.h` on how we identify which threads are to be saved, and I had to change the book keeping in minidump to ensure we don't double save the stacks. Important to @jasonmolenda I also changed the MachO coredump to accept these new APIs.
banach-space
pushed a commit
to banach-space/llvm-project
that referenced
this pull request
Aug 7, 2024
llvm#100443) In llvm#98403 I enabled the SBSaveCoreOptions object, which allows users via the scripting API to define what they want saved into their core file. As the first option I've added a threadlist, so users can scan and identify which threads and corresponding stacks they want to save. In order to support this, I had to add a new method to `Process.h` on how we identify which threads are to be saved, and I had to change the book keeping in minidump to ensure we don't double save the stacks. Important to @jasonmolenda I also changed the MachO coredump to accept these new APIs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds
SBSaveCoreOptions, which is a container class for options when LLDB is taking coredumps. For this first iteration this container just keeps parity with the extant API offile, style, plugin. In the future this options object can be extended to allow users to take a subset of their core dumps.