[LLDB][SBSaveCore] Fix bug where default values are not propagated. #101770

Jlalond · 2024-08-02T23:06:52Z

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.

Removed const on the savecoreoptions so defaults can be inspected by the user

CC: @Michael137

llvmbot · 2024-08-02T23:07:25Z

@llvm/pr-subscribers-lldb

Author: Jacob Lalonde (Jlalond)

Changes

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

Full diff: https://github.com/llvm/llvm-project/pull/101770.diff

3 Files Affected:

(modified) lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp (+10-6)
(modified) lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp (+7-4)
(modified) lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py (+28)

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/functionalities/process_save_core/TestProcessSaveCore.py b/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py
index 8573d15733927..9876dd6e29f3a 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,26 @@ 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))

github-actions · 2024-08-02T23:10:45Z

✅ With the latest revision this PR passed the Python code formatter.

github-actions · 2024-08-02T23:10:45Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 1ae837ab34424a0b81bcc9a4fabc89e36cd57235 78e09d72398e3460341d46fa6fcb1173309db2ea --extensions cpp,h -- lldb/include/lldb/Core/PluginManager.h lldb/include/lldb/lldb-private-interfaces.h 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/MinidumpFileBuilder.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.h

View the diff from clang-format here.

diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
index fc1af24819..da12618043 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
@@ -78,7 +78,7 @@ public:
                       const lldb::ProcessSP &process_sp,
                       lldb_private::SaveCoreOptions &save_core_options)
       : m_process_sp(process_sp), m_core_file(std::move(core_file)),
-        m_save_core_options(save_core_options){};
+        m_save_core_options(save_core_options) {};
 
   MinidumpFileBuilder(const MinidumpFileBuilder &) = delete;
   MinidumpFileBuilder &operator=(const MinidumpFileBuilder &) = delete;

clayborg

Might actually be nice to make the SaveCoreOptions non const. Why? Because clients could see what the settings were set to if a plug-in modifies or overrides certain settings.

… formatter for C++, and Py

hokein · 2024-08-05T07:32:45Z

The newly-added test causes a msan failure:

==1960==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x55e25271b29e in RetryAfterSignal<int, long (int, const void *, unsigned long), int, const void *, unsigned long> llvm-project/llvm/include/llvm/Support/Errno.h:37:11
    #1 0x55e25271b29e in lldb_private::NativeFile::Write(void const*, unsigned long&) llvm-project/lldb/source/Host/common/File.cpp:618:9
    #2 0x55e25223793f in MinidumpFileBuilder::FlushBufferToDisk() llvm-project/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp:1165:26
    #3 0x55e252237548 in MinidumpFileBuilder::FixThreadStacks() llvm-project/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp:568:3
    #4 0x55e25223ba52 in MinidumpFileBuilder::AddMemoryList() llvm-project/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp:890:10
    #5 0x55e2522438ad in ObjectFileMinidump::SaveCore(std::__msan::shared_ptr<lldb_private::Process> const&, lldb_private::SaveCoreOptions&, lldb_private::Status&) llvm-project/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp:124:19
    #6 0x55e251e1f482 in lldb_private::PluginManager::SaveCore(std::__msan::shared_ptr<lldb_private::Process> const&, lldb_private::SaveCoreOptions&) llvm-project/lldb/source/Core/PluginManager.cpp:736:33

The test doesn't pass on my local machine:

  File "/llvm-project/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py", line 107, in test_save_core_default_values_for_style_minidump
    self.assertState(process.GetState(), lldb.eStateStopped)
  File "/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2580, in assertState
    self.fail(self._formatMessage(msg, error))
AssertionError: launching (4) != stopped (5)
Config=x86_64-/llvm-project/build/bin/clang
======================================================================
FAIL: test_save_core_default_values_for_style_minidump_dwo (TestProcessSaveCore.ProcessSaveCoreTestCase.test_save_core_default_values_for_style_minidump_dwo)
    Test we can still save a core for minidump when no
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1758, in test_method
    return attrvalue(self)
           ^^^^^^^^^^^^^^^
  File "/llvm-project/lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py", line 107, in test_save_core_default_values_for_style_minidump
    self.assertState(process.GetState(), lldb.eStateStopped)
  File "/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2580, in assertState
    self.fail(self._formatMessage(msg, error))
AssertionError: launching (4) != stopped (5)

…gated. (#101770)" This reverts commit 34766d0 which caused a msan failure, see comment #101770 (comment) for details.

hokein · 2024-08-05T07:41:44Z

@Jlalond, I revert it in 86f7374 to keep the trunk green. Feel free to reland the patch with the fix.

Jlalond · 2024-08-05T16:30:56Z

@hokein Thank you for reverting it, I'll look into what I missed :)

Jlalond · 2024-08-05T20:33:38Z

@hokein Can you share the build command you got to trigger the above msan error? I'm having trouble reproducing your issue

clayborg · 2024-08-05T21:55:30Z

@hokein Can you share the build command you got to trigger the above msan error? I'm having trouble reproducing your issue

And was your build using msan? or just a regular build? I can see a msan build fail to create a core file due to it crashing, and then you might see this result. But if you didn't have msan enabled, what OS did this fail for?

hokein · 2024-08-06T07:49:31Z

I saw the msan failure when integrating LLVM into our internal codebase, it is a msan-build lldb.

The second error AssertionError: launching (4) != stopped (5) occurred in a non-msan-build lldb from the upstream repository. And I ran llvm-lit -sv lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py

Jlalond · 2024-08-07T20:45:54Z

I saw the msan failure when integrating LLVM into our internal codebase, it is a msan-build lldb.

The second error AssertionError: launching (4) != stopped (5) occurred in a non-msan-build lldb from the upstream repository. And I ran llvm-lit -sv lldb/test/API/functionalities/process_save_core/TestProcessSaveCore.py

@hokein can you share your repro steps for this? I haven't been able to reproduce this error on my machine, so I will need more information from you

…agated. (llvm#101770)" This reverts commit 86f7374.

Reapply #100443 and #101770. These were originally reverted due to a test failure and an MSAN failure. I changed the test attribute to restrict to x86 (following the other existing tests). I could not reproduce the test or the MSAN failure and no repo steps were provided.

Reapply llvm#100443 and llvm#101770. These were originally reverted due to a test failure and an MSAN failure. I changed the test attribute to restrict to x86 (following the other existing tests). I could not reproduce the test or the MSAN failure and no repo steps were provided. (cherry picked from commit 572943e)

Jlalond added 3 commits August 2, 2024 15:44

Create copies in both minidump and mach-o so we can set a default value

11f8323

add new SBSaveCoreOptions based tests for minidump and mach-o

66a4b09

Drop mach-o test as testskinnycore already covers case

9d105b2

Jlalond requested a review from JDevlieghere as a code owner August 2, 2024 23:06

llvmbot added the lldb label Aug 2, 2024

clayborg reviewed Aug 2, 2024

View reviewed changes

Jlalond force-pushed the fix-skinny-core-test branch from bda40c4 to db508b0 Compare August 2, 2024 23:41

Drop const reference so mutations can be pushed back to the user. Run…

78e09d7

… formatter for C++, and Py

Jlalond force-pushed the fix-skinny-core-test branch from db508b0 to 78e09d7 Compare August 2, 2024 23:48

clayborg approved these changes Aug 2, 2024

View reviewed changes

Jlalond merged commit 34766d0 into llvm:main Aug 3, 2024
3 of 5 checks passed

Jlalond deleted the fix-skinny-core-test branch August 3, 2024 02:13

hokein added a commit that referenced this pull request Aug 5, 2024

Revert "[LLDB][SBSaveCore] Fix bug where default values are not propa…

86f7374

…gated. (#101770)" This reverts commit 34766d0 which caused a msan failure, see comment #101770 (comment) for details.

Jlalond added a commit to Jlalond/llvm-project that referenced this pull request Aug 15, 2024

Reapply "[LLDB][SBSaveCore] Fix bug where default values are not prop…

29cb63f

…agated. (llvm#101770)" This reverts commit 86f7374.

Jlalond mentioned this pull request Aug 15, 2024

[LLDB] Reapply #100443 SBSaveCore Thread list #104497

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[LLDB][SBSaveCore] Fix bug where default values are not propagated. #101770

[LLDB][SBSaveCore] Fix bug where default values are not propagated. #101770

Uh oh!

Jlalond commented Aug 2, 2024 •

edited

Loading

Uh oh!

llvmbot commented Aug 2, 2024

Uh oh!

github-actions bot commented Aug 2, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Aug 2, 2024 •

edited

Loading

Uh oh!

clayborg left a comment

Uh oh!

Uh oh!

hokein commented Aug 5, 2024 •

edited

Loading

Uh oh!

hokein commented Aug 5, 2024

Uh oh!

Jlalond commented Aug 5, 2024

Uh oh!

Jlalond commented Aug 5, 2024

Uh oh!

clayborg commented Aug 5, 2024

Uh oh!

hokein commented Aug 6, 2024

Uh oh!

Jlalond commented Aug 7, 2024

Uh oh!

Uh oh!

[LLDB][SBSaveCore] Fix bug where default values are not propagated. #101770

[LLDB][SBSaveCore] Fix bug where default values are not propagated. #101770

Uh oh!

Conversation

Jlalond commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 2, 2024

Uh oh!

github-actions bot commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hokein commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hokein commented Aug 5, 2024

Uh oh!

Jlalond commented Aug 5, 2024

Uh oh!

Jlalond commented Aug 5, 2024

Uh oh!

clayborg commented Aug 5, 2024

Uh oh!

hokein commented Aug 6, 2024

Uh oh!

Jlalond commented Aug 7, 2024

Uh oh!

Uh oh!

Jlalond commented Aug 2, 2024 •

edited

Loading

github-actions bot commented Aug 2, 2024 •

edited

Loading

github-actions bot commented Aug 2, 2024 •

edited

Loading

hokein commented Aug 5, 2024 •

edited

Loading