Skip to content

[LLDB][Minidump] Minidump erase file on failure #108259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jlalond merged 7 commits into from
Sep 13, 2024
Merged

[LLDB][Minidump] Minidump erase file on failure #108259

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
022173d
Unlink file on failure
Jlalond Sep 11, 2024
8a51bd0
Add test to ensure file is deleted upon failure
Jlalond Sep 11, 2024
7150acb
use RAII object
Jlalond Sep 11, 2024
233f5cc
Run formatter
Jlalond Sep 11, 2024
cd89bcf
Rename the RAII object
Jlalond Sep 12, 2024
33b6474
Add error message assertion to test
Jlalond Sep 12, 2024
cbfe997
Run python formatter
Jlalond Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1218,3 +1218,15 @@ Status MinidumpFileBuilder::DumpFile() {

return error;
}

void MinidumpFileBuilder::DeleteFile() noexcept {
Log *log = GetLog(LLDBLog::Object);

if (m_core_file) {
Status error = m_core_file->Close();
if (error.Fail())
LLDB_LOGF(log, "Failed to close minidump file: %s", error.AsCString());

m_core_file.reset();
}
}
3 changes: 3 additions & 0 deletions lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,9 @@ class MinidumpFileBuilder {
// Run cleanup and write all remaining bytes to file
lldb_private::Status DumpFile();

// Delete the file if it exists
void DeleteFile() noexcept;

private:
// Add data to the end of the buffer, if the buffer exceeds the flush level,
// trigger a flush.
Expand Down
18 changes: 18 additions & 0 deletions lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,21 @@ size_t ObjectFileMinidump::GetModuleSpecifications(
return 0;
}

struct SaveCoreRequest {
Copy link
Contributor

@jeffreytan81 jeffreytan81 Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per our naming convention, let's call this RAII class FileRemoveHolder/DumpFailRemoveHolder or something similar, then the reader immediately know it is a RAII class. The current SaveCoreRequest is not indicating its purpose.

SaveCoreRequest(MinidumpFileBuilder &builder) : m_builder(builder) {}

~SaveCoreRequest() {
if (!m_success)
m_builder.DeleteFile();
Comment on lines +62 to +63
Copy link
Contributor

@jeffreytan81 jeffreytan81 Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should emit visible error message telling end user that saving minidump has failed instead of silently fail and remove minidump file.

Copy link
Contributor Author

@Jlalond Jlalond Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to get the patch, it's one of my SBSaveCore API changes, but we now print the error when Plugin Manager returns, we could print twice, one specific for minidump

}

void SetSuccess() { m_success = true; }

private:
MinidumpFileBuilder &m_builder;
bool m_success = false;
};

bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
lldb_private::SaveCoreOptions &options,
lldb_private::Status &error) {
Expand All @@ -75,6 +90,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
}
MinidumpFileBuilder builder(std::move(maybe_core_file.get()), process_sp,
options);
SaveCoreRequest request(builder);

Log *log = GetLog(LLDBLog::Object);
error = builder.AddHeaderAndCalculateDirectories();
Expand Down Expand Up @@ -133,5 +149,7 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
return false;
}

request.SetSuccess();

return true;
}
26 changes: 26 additions & 0 deletions lldb/test/API/functionalities/process_save_core_minidump/TestProcessSaveCoreMinidump.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,3 +493,29 @@ def test_save_minidump_custom_save_style_duplicated_regions(self):

finally:
self.assertTrue(self.dbg.DeleteTarget(target))

@skipUnlessPlatform(["linux"])
def minidump_deleted_on_save_failure(self):
"""Test that verifies the minidump file is deleted after an error"""

self.build()
exe = self.getBuildArtifact("a.out")
try:
target = self.dbg.CreateTarget(exe)
process = target.LaunchSimple(
None, None, self.get_process_working_directory()
)
self.assertState(process.GetState(), lldb.eStateStopped)

custom_file = self.getBuildArtifact("core.should.be.deleted.custom.dmp")
options = lldb.SBSaveCoreOptions()
options.SetOutputFile(lldb.SBFileSpec(custom_file))
options.SetPluginName("minidump")
options.SetStyle(lldb.eSaveCoreCustomOnly)
# We set custom only and have no thread list and have no memory.
error = process.SaveCore(options)
self.assertTrue(error.Fail())
self.assertTrue(not os.path.isfile(custom_file))

finally:
self.assertTrue(self.dbg.DeleteTarget(target))