[Lldb-commits] [lldb] Fix saving minidump from lldb-dap (PR #89113)
via lldb-commits
lldb-commits at lists.llvm.org
Wed Apr 17 11:09:55 PDT 2024
https://github.com/jeffreytan81 created https://github.com/llvm/llvm-project/pull/89113
There are users reporting saving minidump from lldb-dap does not work.
Turns out our stack trace request always evaluate a function call which caused JIT object file like "__lldb_caller_function" to be created which can fail minidump builder to get its module size.
This patch fixes "getModuleFileSize" for ObjectFileJIT so that module list can be saved. I decided to create a lldb-dap test so that this end-to-end functionality can be validated from our tests (instead of only command line lldb).
The patch also improves several other small things in the workflow:
1. It logs any minidump stream failure so that it is easier to find out what stream saving fails. In future, we should show process and error to end users.
2. It handles error from "getModuleFileSize" llvm::Expected<T> otherwise it will complain the error is not handled.
>From 6cccde22723157260e7c0b19bf8372aae8d1afaa Mon Sep 17 00:00:00 2001
From: jeffreytan81 <jeffreytan at fb.com>
Date: Wed, 6 Mar 2024 12:07:03 -0800
Subject: [PATCH 1/2] Fix strcmp build error on buildbot
---
lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp | 1 +
1 file changed, 1 insertion(+)
diff --git a/lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp b/lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp
index b0a4446ba01581..40cb63755ee8a5 100644
--- a/lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp
+++ b/lldb/test/API/functionalities/fork/concurrent_vfork/main.cpp
@@ -1,6 +1,7 @@
#include <assert.h>
#include <iostream>
#include <mutex>
+#include <string.h>
#include <sys/wait.h>
#include <thread>
#include <unistd.h>
>From 4453a9cb876fe4ed3c5a3ea57a03a428c5d847fc Mon Sep 17 00:00:00 2001
From: jeffreytan81 <jeffreytan at fb.com>
Date: Wed, 17 Apr 2024 10:59:51 -0700
Subject: [PATCH 2/2] Fix saving minidump from lldb-dap
---
.../Minidump/MinidumpFileBuilder.cpp | 20 ++++-
.../Minidump/ObjectFileMinidump.cpp | 18 ++++-
.../API/tools/lldb-dap/save-core/Makefile | 5 ++
.../lldb-dap/save-core/TestDAP_save_core.py | 81 +++++++++++++++++++
.../API/tools/lldb-dap/save-core/main.cpp | 8 ++
5 files changed, 125 insertions(+), 7 deletions(-)
create mode 100644 lldb/test/API/tools/lldb-dap/save-core/Makefile
create mode 100644 lldb/test/API/tools/lldb-dap/save-core/TestDAP_save_core.py
create mode 100644 lldb/test/API/tools/lldb-dap/save-core/main.cpp
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
index 50d1b563f469cf..cefd4cb22b6bae 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp
@@ -20,6 +20,7 @@
#include "lldb/Target/StopInfo.h"
#include "lldb/Target/ThreadList.h"
#include "lldb/Utility/DataExtractor.h"
+#include "lldb/Utility/LLDBLog.h"
#include "lldb/Utility/RegisterValue.h"
#include "llvm/ADT/StringRef.h"
@@ -154,8 +155,16 @@ Status WriteString(const std::string &to_write,
llvm::Expected<uint64_t> getModuleFileSize(Target &target,
const ModuleSP &mod) {
- SectionSP sect_sp = mod->GetObjectFile()->GetBaseAddress().GetSection();
+ // JIT module has the same vm and file size.
uint64_t SizeOfImage = 0;
+ if (mod->GetObjectFile()->CalculateType() == ObjectFile::Type::eTypeJIT) {
+ for (const auto §ion : *mod->GetObjectFile()->GetSectionList()) {
+ SizeOfImage += section->GetByteSize();
+ }
+ return SizeOfImage;
+ }
+
+ SectionSP sect_sp = mod->GetObjectFile()->GetBaseAddress().GetSection();
if (!sect_sp) {
return llvm::createStringError(std::errc::operation_not_supported,
@@ -226,8 +235,13 @@ Status MinidumpFileBuilder::AddModuleList(Target &target) {
std::string module_name = mod->GetSpecificationDescription();
auto maybe_mod_size = getModuleFileSize(target, mod);
if (!maybe_mod_size) {
- error.SetErrorStringWithFormat("Unable to get the size of module %s.",
- module_name.c_str());
+ llvm::Error mod_size_err = maybe_mod_size.takeError();
+ llvm::handleAllErrors(std::move(mod_size_err),
+ [&](const llvm::ErrorInfoBase &E) {
+ error.SetErrorStringWithFormat(
+ "Unable to get the size of module %s: %s.",
+ module_name.c_str(), E.message().c_str());
+ });
return error;
}
diff --git a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
index fe609c7f3d2001..1af5d99f0b1604 100644
--- a/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
+++ b/lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp
@@ -14,6 +14,7 @@
#include "lldb/Core/PluginManager.h"
#include "lldb/Core/Section.h"
#include "lldb/Target/Process.h"
+#include "lldb/Utility/LLDBLog.h"
#include "llvm/Support/FileSystem.h"
@@ -68,26 +69,35 @@ bool ObjectFileMinidump::SaveCore(const lldb::ProcessSP &process_sp,
Target &target = process_sp->GetTarget();
+ Log *log = GetLog(LLDBLog::Object);
error = builder.AddSystemInfo(target.GetArchitecture().GetTriple());
- if (error.Fail())
+ if (error.Fail()) {
+ LLDB_LOG(log, "AddSystemInfo failed: %s", error.AsCString());
return false;
+ }
error = builder.AddModuleList(target);
- if (error.Fail())
+ if (error.Fail()) {
+ LLDB_LOG(log, "AddModuleList failed: %s", error.AsCString());
return false;
+ }
builder.AddMiscInfo(process_sp);
error = builder.AddThreadList(process_sp);
- if (error.Fail())
+ if (error.Fail()) {
+ LLDB_LOG(log, "AddThreadList failed: %s", error.AsCString());
return false;
+ }
// Add any exceptions but only if there are any in any threads.
builder.AddExceptions(process_sp);
error = builder.AddMemoryList(process_sp, core_style);
- if (error.Fail())
+ if (error.Fail()) {
+ LLDB_LOG(log, "AddMemoryList failed: %s", error.AsCString());
return false;
+ }
if (target.GetArchitecture().GetTriple().getOS() ==
llvm::Triple::OSType::Linux) {
diff --git a/lldb/test/API/tools/lldb-dap/save-core/Makefile b/lldb/test/API/tools/lldb-dap/save-core/Makefile
new file mode 100644
index 00000000000000..b7f3072e28c9d9
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/save-core/Makefile
@@ -0,0 +1,5 @@
+ENABLE_THREADS := YES
+
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/save-core/TestDAP_save_core.py b/lldb/test/API/tools/lldb-dap/save-core/TestDAP_save_core.py
new file mode 100644
index 00000000000000..cff89a98fe72ec
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/save-core/TestDAP_save_core.py
@@ -0,0 +1,81 @@
+"""
+Test saving core minidump from lldb-dap
+"""
+
+
+import dap_server
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import lldbdap_testcase
+
+
+class TestDAP_save_core(lldbdap_testcase.DAPTestCaseBase):
+ @skipUnlessArch("x86_64")
+ @skipUnlessPlatform(["linux"])
+ def test_save_core(self):
+ """
+ Tests saving core minidump from lldb-dap.
+ """
+ program = self.getBuildArtifact("a.out")
+ self.build_and_launch(program)
+ source = "main.cpp"
+ # source_path = os.path.join(os.getcwd(), source)
+ breakpoint1_line = line_number(source, "// breakpoint 1")
+ lines = [breakpoint1_line]
+ # Set breakpoint in the thread function so we can step the threads
+ breakpoint_ids = self.set_source_breakpoints(source, lines)
+ self.assertEqual(
+ len(breakpoint_ids), len(lines), "expect correct number of breakpoints"
+ )
+ self.continue_to_breakpoints(breakpoint_ids)
+
+ # Getting dap stack trace may trigger __lldb_caller_function JIT module to be created.
+ self.get_stackFrames(startFrame=0)
+
+ # Evaluating an expression that cause "_$__lldb_valid_pointer_check" JIT module to be created.
+ expression = 'printf("this is a test")'
+ self.dap_server.request_evaluate(expression, context="watch")
+
+ # Verify "_$__lldb_valid_pointer_check" JIT module is created.
+ modules = self.dap_server.get_modules()
+ self.assertTrue(modules["_$__lldb_valid_pointer_check"])
+ thread_count = len(self.dap_server.get_threads())
+
+ core_stack = self.getBuildArtifact("core.stack.dmp")
+ core_dirty = self.getBuildArtifact("core.dirty.dmp")
+ core_full = self.getBuildArtifact("core.full.dmp")
+
+ base_command = "`process save-core --plugin-name=minidump "
+ self.dap_server.request_evaluate(base_command + " --style=stack '%s'" % (core_stack), context="repl")
+
+ self.assertTrue(os.path.isfile(core_stack))
+ self.verify_core_file(
+ core_stack, len(modules), thread_count
+ )
+
+ self.dap_server.request_evaluate(base_command + " --style=modified-memory '%s'" % (core_dirty), context="repl")
+ self.assertTrue(os.path.isfile(core_dirty))
+ self.verify_core_file(
+ core_dirty, len(modules), thread_count
+ )
+
+ self.dap_server.request_evaluate(base_command + " --style=full '%s'" % (core_full), context="repl")
+ self.assertTrue(os.path.isfile(core_full))
+ self.verify_core_file(
+ core_full, len(modules), thread_count
+ )
+
+ def verify_core_file(
+ self, core_path, expected_module_count, expected_thread_count
+ ):
+ # To verify, we'll launch with the mini dump
+ target = self.dbg.CreateTarget(None)
+ process = target.LoadCore(core_path)
+
+ # check if the core is in desired state
+ self.assertTrue(process, PROCESS_IS_VALID)
+ self.assertTrue(process.GetProcessInfo().IsValid())
+ self.assertNotEqual(target.GetTriple().find("linux"), -1)
+ self.assertTrue(target.GetNumModules(), expected_module_count)
+ self.assertEqual(process.GetNumThreads(), expected_thread_count)
diff --git a/lldb/test/API/tools/lldb-dap/save-core/main.cpp b/lldb/test/API/tools/lldb-dap/save-core/main.cpp
new file mode 100644
index 00000000000000..8905beb5e7eff6
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/save-core/main.cpp
@@ -0,0 +1,8 @@
+int function(int x) {
+ if ((x % 2) == 0)
+ return function(x - 1) + x; // breakpoint 1
+ else
+ return x;
+}
+
+int main(int argc, char const *argv[]) { return function(2); }
More information about the lldb-commits
mailing list