Fix saving minidump from lldb-dap

[jeffreytan81]

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 otherwise it will complain the error is not handled.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: None (jeffreytan81)

Changes

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.

---

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

5 Files Affected:

• (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp (+17-3)
• (modified) lldb/source/Plugins/ObjectFile/Minidump/ObjectFileMinidump.cpp (+14-4)
• (added) lldb/test/API/tools/lldb-dap/save-core/Makefile (+5)
• (added) lldb/test/API/tools/lldb-dap/save-core/TestDAP_save_core.py (+81)
• (added) lldb/test/API/tools/lldb-dap/save-core/main.cpp (+8)

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 &section : *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); }

[github-actions]

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