[Lldb-commits] [lldb] [LLDB][SBSaveCore] Fix bug where default values are not propagated. (PR #101770)
Jacob Lalonde via lldb-commits
lldb-commits at lists.llvm.org
Fri Aug 2 16:07:00 PDT 2024
https://github.com/Jlalond created https://github.com/llvm/llvm-project/pull/101770
In #100443, Mach-o and Minidump now only call process API's that take a `SaveCoreOption` as the container for the style and information if a thread should be included in the core or not. This introduced a bug where in subsequent method calls we were not honoring the defaults of both implementations.
To solve this I have made a copy of each SaveCoreOptions that is mutable by the respective plugin. Originally I wanted to leave the SaveCoreOptions as non const so these default value mutations could be shown back to the user. Changing that behavior is outside of the scope of this bugfix, but is context for why we are making a copy.
CC: @Michael137
>From 11f8323bb40a537161c76b89d564c9ae96888d01 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Fri, 2 Aug 2024 15:44:17 -0700
Subject: [PATCH 1/3] Create copies in both minidump and mach-o so we can set a
default value
---
.../ObjectFile/Mach-O/ObjectFileMachO.cpp | 16 ++++++++++------
.../ObjectFile/Minidump/ObjectFileMinidump.cpp | 11 +++++++----
.../macosx/skinny-corefile/TestSkinnyCorefile.py | 1 -
3 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 4322bd7e2674f..d09eec0f9cb62 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6524,13 +6524,17 @@ struct page_object {
bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
const lldb_private::SaveCoreOptions &options,
Status &error) {
- auto core_style = options.GetStyle();
- if (core_style == SaveCoreStyle::eSaveCoreUnspecified)
- core_style = SaveCoreStyle::eSaveCoreDirtyOnly;
// The FileSpec and Process are already checked in PluginManager::SaveCore.
assert(options.GetOutputFile().has_value());
assert(process_sp);
const FileSpec outfile = options.GetOutputFile().value();
+
+ // We have to make a local copy of the options, so that if we default
+ // the save core style, we can proprogate that when we pass options
+ // to process calculate save core ranges
+ lldb_private::SaveCoreOptions core_options = options;
+ if (core_options.GetStyle() == SaveCoreStyle::eSaveCoreUnspecified)
+ core_options.SetStyle(eSaveCoreDirtyOnly);
Target &target = process_sp->GetTarget();
const ArchSpec target_arch = target.GetArchitecture();
@@ -6561,7 +6565,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
if (make_core) {
Process::CoreFileMemoryRanges core_ranges;
- error = process_sp->CalculateCoreFileSaveRanges(options, core_ranges);
+ error = process_sp->CalculateCoreFileSaveRanges(core_options, core_ranges);
if (error.Success()) {
const uint32_t addr_byte_size = target_arch.GetAddressByteSize();
const ByteOrder byte_order = target_arch.GetByteOrder();
@@ -6733,7 +6737,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
StructuredData::ArraySP threads(
std::make_shared<StructuredData::Array>());
for (const ThreadSP &thread_sp :
- process_sp->CalculateCoreFileThreadList(options)) {
+ process_sp->CalculateCoreFileThreadList(core_options)) {
StructuredData::DictionarySP thread(
std::make_shared<StructuredData::Dictionary>());
thread->AddIntegerItem("thread_id", thread_sp->GetID());
@@ -6756,7 +6760,7 @@ bool ObjectFileMachO::SaveCore(const lldb::ProcessSP &process_sp,
all_image_infos_lcnote_up->payload_file_offset = file_offset;
file_offset = CreateAllImageInfosPayload(
process_sp, file_offset, all_image_infos_lcnote_up->payload,
- options);
+ core_options);
lc_notes.push_back(std::move(all_image_infos_lcnote_up));
// Add LC_NOTE load commands
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index 2380ff4c00ca9..22f1323503115 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -62,10 +62,13 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
assert(options.GetOutputFile().has_value());
assert(process_sp);
+ // We have to make a local copy of the options, so that if we default
+ // the save core style, we can proprogate that when we pass options
+ // to process calculate save core ranges
+ lldb_private::SaveCoreOptions core_options = options;
// Minidump defaults to stacks only.
- SaveCoreStyle core_style = options.GetStyle();
- if (core_style == SaveCoreStyle::eSaveCoreUnspecified)
- core_style = SaveCoreStyle::eSaveCoreStackOnly;
+ if (core_options.GetStyle() == SaveCoreStyle::eSaveCoreUnspecified)
+ core_options.SetStyle(SaveCoreStyle::eSaveCoreStackOnly);
llvm::Expected<lldb::FileUP> maybe_core_file = FileSystem::Instance().Open(
options.GetOutputFile().value(),
@@ -75,7 +78,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
return false;
}
MinidumpFileBuilder builder(std::move(maybe_core_file.get()), process_sp,
- options);
+ core_options);
Log *log = GetLog(LLDBLog::Object);
error = builder.AddHeaderAndCalculateDirectories();
diff --git a/lldb/test/API/macosx/skinny-corefile/TestSkinnyCorefile.py b/lldb/test/API/macosx/skinny-corefile/TestSkinnyCorefile.py
index 138792d817dbf..56593c29f0579 100644
--- a/lldb/test/API/macosx/skinny-corefile/TestSkinnyCorefile.py
+++ b/lldb/test/API/macosx/skinny-corefile/TestSkinnyCorefile.py
@@ -17,7 +17,6 @@ class TestSkinnyCorefile(TestBase):
debug_info=no_match(["dsym"]),
bugnumber="This test is looking explicitly for a dSYM",
)
- @skipUnlessDarwin
@skipIfRemote
def test_lc_note(self):
self.build()
>From 66a4b0986412692f513a7479c868aa9616bf34a7 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Fri, 2 Aug 2024 15:54:09 -0700
Subject: [PATCH 2/3] add new SBSaveCoreOptions based tests for minidump and
mach-o
---
.../process_save_core/TestProcessSaveCore.py | 51 +++++++++++++++++++
.../skinny-corefile/TestSkinnyCorefile.py | 1 +
2 files changed, 52 insertions(+)
diff --git a/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py b/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py
index 8573d15733927..eaab1004b6a5b 100644
--- a/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py
+++ b/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py
@@ -10,6 +10,11 @@
class ProcessSaveCoreTestCase(TestBase):
+ def validate_core_pid(self, pid, core_path):
+ target = self.dbg.CreateTarget(None)
+ process = target.LoadCore(core_path)
+ return process.GetProcessID() == pid
+
@skipIfRemote
@skipUnlessWindows
def test_cannot_save_core_unless_process_stopped(self):
@@ -88,3 +93,49 @@ def test_save_core_via_process_plugin(self):
os.unlink(core)
except OSError:
pass
+
+ @skipUnlessPlatform(["linux"])
+ def test_save_core_default_values_for_style_minidump(self):
+ """Test we can still save a core for minidump when no
+ core style is specified."""
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+ core = self.getBuildArtifact("core.dmp")
+ target = self.dbg.CreateTarget(exe)
+ target.BreakpointCreateByName("bar")
+ process = target.LaunchSimple(
+ None, None, self.get_process_working_directory()
+ )
+ self.assertState(process.GetState(), lldb.eStateStopped)
+ pid = process.GetProcessID()
+ options = lldb.SBSaveCoreOptions()
+ minidump_path = core + ".minidump"
+ options.SetOutputFile(lldb.SBFileSpec(minidump_path))
+ options.SetPluginName("minidump")
+ error = process.SaveCore(options)
+ self.assertSuccess(error, error.GetCString())
+ self.assertTrue(os.path.isfile(minidump_path))
+ self.assertTrue(self.validate_core_pid(pid, minidump_path))
+
+ @skipUnlessDarwin
+ def test_save_core_default_values_for_style_mach_o(self):
+ """Test we can still save a core for minidump when no
+ core style is specified."""
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+ core = self.getBuildArtifact("core.dmp")
+ target = self.dbg.CreateTarget(exe)
+ target.BreakpointCreateByName("bar")
+ process = target.LaunchSimple(
+ None, None, self.get_process_working_directory()
+ )
+ self.assertState(process.GetState(), lldb.eStateStopped)
+ pid = process.GetProcessID()
+ options = lldb.SBSaveCoreOptions()
+
+ options.SetPluginName("mach-o")
+ mach_o_path = core + ".mach-o"
+ error = process.SaveCore(options)
+ self.assertSuccess(error, error.GetCString())
+ self.assertTrue(os.path.isfile(mach_o_path))
+ self.assertTrue(self.validate_core_pid(pid, mach_o_path))
diff --git a/lldb/test/API/macosx/skinny-corefile/TestSkinnyCorefile.py b/lldb/test/API/macosx/skinny-corefile/TestSkinnyCorefile.py
index 56593c29f0579..138792d817dbf 100644
--- a/lldb/test/API/macosx/skinny-corefile/TestSkinnyCorefile.py
+++ b/lldb/test/API/macosx/skinny-corefile/TestSkinnyCorefile.py
@@ -17,6 +17,7 @@ class TestSkinnyCorefile(TestBase):
debug_info=no_match(["dsym"]),
bugnumber="This test is looking explicitly for a dSYM",
)
+ @skipUnlessDarwin
@skipIfRemote
def test_lc_note(self):
self.build()
>From 9d105b25b2e0721d68db8dca60cb155d233d0933 Mon Sep 17 00:00:00 2001
From: Jacob Lalonde <jalalonde at fb.com>
Date: Fri, 2 Aug 2024 16:03:03 -0700
Subject: [PATCH 3/3] Drop mach-o test as testskinnycore already covers case
---
.../process_save_core/TestProcessSaveCore.py | 23 -------------------
1 file changed, 23 deletions(-)
diff --git a/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py b/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py
index eaab1004b6a5b..9876dd6e29f3a 100644
--- a/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py
+++ b/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py
@@ -116,26 +116,3 @@ def test_save_core_default_values_for_style_minidump(self):
self.assertSuccess(error, error.GetCString())
self.assertTrue(os.path.isfile(minidump_path))
self.assertTrue(self.validate_core_pid(pid, minidump_path))
-
- @skipUnlessDarwin
- def test_save_core_default_values_for_style_mach_o(self):
- """Test we can still save a core for minidump when no
- core style is specified."""
- self.build()
- exe = self.getBuildArtifact("a.out")
- core = self.getBuildArtifact("core.dmp")
- target = self.dbg.CreateTarget(exe)
- target.BreakpointCreateByName("bar")
- process = target.LaunchSimple(
- None, None, self.get_process_working_directory()
- )
- self.assertState(process.GetState(), lldb.eStateStopped)
- pid = process.GetProcessID()
- options = lldb.SBSaveCoreOptions()
-
- options.SetPluginName("mach-o")
- mach_o_path = core + ".mach-o"
- error = process.SaveCore(options)
- self.assertSuccess(error, error.GetCString())
- self.assertTrue(os.path.isfile(mach_o_path))
- self.assertTrue(self.validate_core_pid(pid, mach_o_path))
More information about the lldb-commits
mailing list