Skip to content

[lldb-dap] Improve stackTrace and exceptionInfo DAP request handlers #105905

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.

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
merged 9 commits into from
Sep 10, 2024

Conversation

ashgti
Copy link
Contributor

@ashgti ashgti commented Aug 23, 2024

Refactoring stackTrace to perform frame look ups in a more on-demand fashion to improve overall performance.

Additionally adding additional information to the exceptionInfo request to report exception stacks there instead of merging the exception stack into the stack trace. The exceptionInfo request is only called if a stop event occurs with reason='exception', which should mitigate the performance of SBThread::GetCurrentException calls.

Adding unit tests for exception handling and stack trace supporting.

@ashgti ashgti requested a review from JDevlieghere as a code owner August 23, 2024 23:17
@llvmbot llvmbot added the lldb label Aug 23, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 23, 2024

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

Changes

Refactoring stackTrace to perform frame look ups in a more on-demand fashion to improve overall performance.

Additionally adding additional information to the exceptionInfo request to report exception stacks there instead of merging the exception stack into the stack trace. The exceptionInfo request is only called if a stop event occurs with reason='exception', which should mitigate the performance of SBThread::GetCurrentException calls.

Adding unit tests for exception handling and stack trace supporting.


Patch is 32.45 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/105905.diff

22 Files Affected:

  • (modified) lldb/packages/Python/lldbsuite/test/lldbplatformutil.py (+16)
  • (modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py (+11)
  • (modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py (+7-2)
  • (modified) lldb/test/API/tools/lldb-dap/exception/Makefile (+1-1)
  • (modified) lldb/test/API/tools/lldb-dap/exception/TestDAP_exception.py (+5-3)
  • (added) lldb/test/API/tools/lldb-dap/exception/cpp/Makefile (+3)
  • (added) lldb/test/API/tools/lldb-dap/exception/cpp/TestDAP_exception_cpp.py (+26)
  • (added) lldb/test/API/tools/lldb-dap/exception/cpp/main.cpp (+6)
  • (renamed) lldb/test/API/tools/lldb-dap/exception/main.c (+1-1)
  • (added) lldb/test/API/tools/lldb-dap/exception/objc/Makefile (+9)
  • (added) lldb/test/API/tools/lldb-dap/exception/objc/TestDAP_exception_objc.py (+27)
  • (added) lldb/test/API/tools/lldb-dap/exception/objc/main.m (+8)
  • (added) lldb/test/API/tools/lldb-dap/extendedStackTrace/Makefile (+5)
  • (added) lldb/test/API/tools/lldb-dap/extendedStackTrace/TestDAP_extendedStackTrace.py (+69)
  • (added) lldb/test/API/tools/lldb-dap/extendedStackTrace/main.m (+28)
  • (modified) lldb/test/API/tools/lldb-dap/stackTrace/TestDAP_stackTrace.py (+1-3)
  • (modified) lldb/test/API/tools/lldb-dap/stackTraceMissingFunctionName/TestDAP_stackTraceMissingFunctionName.py (-5)
  • (modified) lldb/tools/lldb-dap/DAP.cpp (-1)
  • (modified) lldb/tools/lldb-dap/DAP.h (+1-1)
  • (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+40)
  • (modified) lldb/tools/lldb-dap/JSONUtils.h (+3)
  • (modified) lldb/tools/lldb-dap/lldb-dap.cpp (+182-71)
diff --git a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
index 602e15d207e94a..3d8c713562e9bf 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
@@ -181,6 +181,22 @@ def findMainThreadCheckerDylib():
     return ""
 
 
+def findBacktraceRecordingDylib():
+    if not platformIsDarwin():
+        return ""
+
+    if getPlatform() in lldbplatform.translate(lldbplatform.darwin_embedded):
+        return "/Developer/usr/lib/libBacktraceRecording.dylib"
+
+    with os.popen("xcode-select -p") as output:
+        xcode_developer_path = output.read().strip()
+        mtc_dylib_path = "%s/usr/lib/libBacktraceRecording.dylib" % xcode_developer_path
+        if os.path.isfile(mtc_dylib_path):
+            return mtc_dylib_path
+
+    return ""
+
+
 class _PlatformContext(object):
     """Value object class which contains platform-specific options."""
 
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index 874383a13e2bb6..167142779cf12c 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -707,6 +707,17 @@ def request_evaluate(self, expression, frameIndex=0, threadId=None, context=None
         }
         return self.send_recv(command_dict)
 
+    def request_exceptionInfo(self, threadId=None):
+        if threadId is None:
+            threadId = self.get_thread_id()
+        args_dict = {"threadId": threadId}
+        command_dict = {
+            "command": "exceptionInfo",
+            "type": "request",
+            "arguments": args_dict,
+        }
+        return self.send_recv(command_dict)
+
     def request_initialize(self, sourceInitFile):
         command_dict = {
             "command": "initialize",
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
index 86eba355da83db..40d61636380588 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py
@@ -100,13 +100,14 @@ def verify_breakpoint_hit(self, breakpoint_ids):
                         return
         self.assertTrue(False, "breakpoint not hit")
 
-    def verify_stop_exception_info(self, expected_description):
+    def verify_stop_exception_info(self, expected_description, timeout=timeoutval):
         """Wait for the process we are debugging to stop, and verify the stop
         reason is 'exception' and that the description matches
         'expected_description'
         """
-        stopped_events = self.dap_server.wait_for_stopped()
+        stopped_events = self.dap_server.wait_for_stopped(timeout=timeout)
         for stopped_event in stopped_events:
+            print("stopped_event", stopped_event)
             if "body" in stopped_event:
                 body = stopped_event["body"]
                 if "reason" not in body:
@@ -177,6 +178,10 @@ def get_stackFrames(self, threadId=None, startFrame=None, levels=None, dump=Fals
         )
         return stackFrames
 
+    def get_exceptionInfo(self, threadId=None):
+        response = self.dap_server.request_exceptionInfo(threadId=threadId)
+        return self.get_dict_value(response, ["body"])
+
     def get_source_and_line(self, threadId=None, frameIndex=0):
         stackFrames = self.get_stackFrames(
             threadId=threadId, startFrame=frameIndex, levels=1
diff --git a/lldb/test/API/tools/lldb-dap/exception/Makefile b/lldb/test/API/tools/lldb-dap/exception/Makefile
index 99998b20bcb050..10495940055b63 100644
--- a/lldb/test/API/tools/lldb-dap/exception/Makefile
+++ b/lldb/test/API/tools/lldb-dap/exception/Makefile
@@ -1,3 +1,3 @@
-CXX_SOURCES := main.cpp
+C_SOURCES := main.c
 
 include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/exception/TestDAP_exception.py b/lldb/test/API/tools/lldb-dap/exception/TestDAP_exception.py
index 8c2c0154ba65c0..39d73737b7e8c0 100644
--- a/lldb/test/API/tools/lldb-dap/exception/TestDAP_exception.py
+++ b/lldb/test/API/tools/lldb-dap/exception/TestDAP_exception.py
@@ -1,5 +1,5 @@
 """
-Test exception behavior in DAP
+Test exception behavior in DAP with signal.
 """
 
 
@@ -16,8 +16,10 @@ def test_stopped_description(self):
         event.
         """
         program = self.getBuildArtifact("a.out")
-        print("test_stopped_description called", flush=True)
         self.build_and_launch(program)
-
         self.dap_server.request_continue()
         self.assertTrue(self.verify_stop_exception_info("signal SIGABRT"))
+        exceptionInfo = self.get_exceptionInfo()
+        self.assertEqual(exceptionInfo["breakMode"], "always")
+        self.assertEqual(exceptionInfo["description"], "signal SIGABRT")
+        self.assertEqual(exceptionInfo["exceptionId"], "signal")
diff --git a/lldb/test/API/tools/lldb-dap/exception/cpp/Makefile b/lldb/test/API/tools/lldb-dap/exception/cpp/Makefile
new file mode 100644
index 00000000000000..99998b20bcb050
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/exception/cpp/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/exception/cpp/TestDAP_exception_cpp.py b/lldb/test/API/tools/lldb-dap/exception/cpp/TestDAP_exception_cpp.py
new file mode 100644
index 00000000000000..6471e2b87251a7
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/exception/cpp/TestDAP_exception_cpp.py
@@ -0,0 +1,26 @@
+"""
+Test exception behavior in DAP with c++ throw.
+"""
+
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+import lldbdap_testcase
+
+
+class TestDAP_exception_cpp(lldbdap_testcase.DAPTestCaseBase):
+    @skipIfWindows
+    def test_stopped_description(self):
+        """
+        Test that exception description is shown correctly in stopped
+        event.
+        """
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+        self.dap_server.request_continue()
+        self.assertTrue(self.verify_stop_exception_info("signal SIGABRT"))
+        exceptionInfo = self.get_exceptionInfo()
+        self.assertEqual(exceptionInfo["breakMode"], "always")
+        self.assertEqual(exceptionInfo["description"], "signal SIGABRT")
+        self.assertEqual(exceptionInfo["exceptionId"], "signal")
+        self.assertIsNotNone(exceptionInfo["details"])
diff --git a/lldb/test/API/tools/lldb-dap/exception/cpp/main.cpp b/lldb/test/API/tools/lldb-dap/exception/cpp/main.cpp
new file mode 100644
index 00000000000000..39d89b95319a8c
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/exception/cpp/main.cpp
@@ -0,0 +1,6 @@
+#include <stdexcept>
+
+int main(int argc, char const *argv[]) {
+  throw std::invalid_argument("throwing exception for testing");
+  return 0;
+}
diff --git a/lldb/test/API/tools/lldb-dap/exception/main.cpp b/lldb/test/API/tools/lldb-dap/exception/main.c
similarity index 56%
rename from lldb/test/API/tools/lldb-dap/exception/main.cpp
rename to lldb/test/API/tools/lldb-dap/exception/main.c
index b940d07c6f2bb3..a653ac5d82aa3a 100644
--- a/lldb/test/API/tools/lldb-dap/exception/main.cpp
+++ b/lldb/test/API/tools/lldb-dap/exception/main.c
@@ -1,6 +1,6 @@
 #include <signal.h>
 
-int main() {
+int main(int argc, char const *argv[]) {
   raise(SIGABRT);
   return 0;
 }
diff --git a/lldb/test/API/tools/lldb-dap/exception/objc/Makefile b/lldb/test/API/tools/lldb-dap/exception/objc/Makefile
new file mode 100644
index 00000000000000..9b6528337cb9d8
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/exception/objc/Makefile
@@ -0,0 +1,9 @@
+OBJC_SOURCES := main.m
+
+CFLAGS_EXTRAS := -w
+
+USE_SYSTEM_STDLIB := 1
+
+LD_EXTRAS := -framework Foundation
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/exception/objc/TestDAP_exception_objc.py b/lldb/test/API/tools/lldb-dap/exception/objc/TestDAP_exception_objc.py
new file mode 100644
index 00000000000000..777d55f48e8504
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/exception/objc/TestDAP_exception_objc.py
@@ -0,0 +1,27 @@
+"""
+Test exception behavior in DAP with obj-c throw.
+"""
+
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+import lldbdap_testcase
+
+
+class TestDAP_exception_objc(lldbdap_testcase.DAPTestCaseBase):
+    @skipUnlessDarwin
+    def test_stopped_description(self):
+        """
+        Test that exception description is shown correctly in stopped event.
+        """
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+        self.dap_server.request_continue()
+        self.assertTrue(self.verify_stop_exception_info("signal SIGABRT"))
+        exception_info = self.get_exceptionInfo()
+        self.assertEqual(exception_info["breakMode"], "always")
+        self.assertEqual(exception_info["description"], "signal SIGABRT")
+        self.assertEqual(exception_info["exceptionId"], "signal")
+        exception_details = exception_info["details"]
+        self.assertRegex(exception_details["message"], "SomeReason")
+        self.assertRegex(exception_details["stackTrace"], "main.m")
diff --git a/lldb/test/API/tools/lldb-dap/exception/objc/main.m b/lldb/test/API/tools/lldb-dap/exception/objc/main.m
new file mode 100644
index 00000000000000..e8db04fb40de15
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/exception/objc/main.m
@@ -0,0 +1,8 @@
+#import <Foundation/Foundation.h>
+
+int main(int argc, char const *argv[]) {
+  @throw [[NSException alloc] initWithName:@"ThrownException"
+                                    reason:@"SomeReason"
+                                  userInfo:nil];
+  return 0;
+}
diff --git a/lldb/test/API/tools/lldb-dap/extendedStackTrace/Makefile b/lldb/test/API/tools/lldb-dap/extendedStackTrace/Makefile
new file mode 100644
index 00000000000000..e4ee1a0506c0cf
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/extendedStackTrace/Makefile
@@ -0,0 +1,5 @@
+OBJC_SOURCES := main.m
+
+USE_SYSTEM_STDLIB := 1
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/extendedStackTrace/TestDAP_extendedStackTrace.py b/lldb/test/API/tools/lldb-dap/extendedStackTrace/TestDAP_extendedStackTrace.py
new file mode 100644
index 00000000000000..efc907085b2c21
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/extendedStackTrace/TestDAP_extendedStackTrace.py
@@ -0,0 +1,69 @@
+"""
+Test lldb-dap stackTrace request with an extended backtrace thread.
+"""
+
+
+import os
+
+import lldbdap_testcase
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbplatformutil import *
+
+
+class TestDAP_extendedStackTrace(lldbdap_testcase.DAPTestCaseBase):
+    @skipUnlessDarwin
+    def test_stackTrace(self):
+        """
+        Tests the 'stackTrace' packet on a thread with an extended backtrace.
+        """
+        backtrace_recording_lib = findBacktraceRecordingDylib()
+        if not backtrace_recording_lib:
+            self.skipTest(
+                "Skipped because libBacktraceRecording.dylib was present on the system."
+            )
+
+        if not os.path.isfile("/usr/lib/system/introspection/libdispatch.dylib"):
+            self.skipTest(
+                "Skipped because introspection libdispatch dylib is not present."
+            )
+
+        program = self.getBuildArtifact("a.out")
+
+        self.build_and_launch(
+            program,
+            env=[
+                "DYLD_LIBRARY_PATH=/usr/lib/system/introspection",
+                "DYLD_INSERT_LIBRARIES=" + backtrace_recording_lib,
+            ],
+        )
+        source = "main.m"
+        breakpoint = line_number(source, "breakpoint 1")
+        lines = [breakpoint]
+
+        breakpoint_ids = self.set_source_breakpoints(source, lines)
+        self.assertEqual(
+            len(breakpoint_ids), len(lines), "expect correct number of breakpoints"
+        )
+
+        events = self.continue_to_next_stop()
+        print("huh", events)
+        stackFrames = self.get_stackFrames(threadId=events[0]["body"]["threadId"])
+        self.assertGreaterEqual(len(stackFrames), 3, "expect >= 3 frames")
+        self.assertEqual(stackFrames[0]["name"], "one")
+        self.assertEqual(stackFrames[1]["name"], "two")
+        self.assertEqual(stackFrames[2]["name"], "three")
+
+        stackLabels = [
+            frame
+            for frame in stackFrames
+            if frame.get("presentationHint", "") == "label"
+        ]
+        self.assertEqual(len(stackLabels), 2, "expected two label stack frames")
+        self.assertRegex(
+            stackLabels[0]["name"],
+            "Enqueued from com.apple.root.default-qos \(Thread \d\)",
+        )
+        self.assertRegex(
+            stackLabels[1]["name"], "Enqueued from com.apple.main-thread \(Thread \d\)"
+        )
diff --git a/lldb/test/API/tools/lldb-dap/extendedStackTrace/main.m b/lldb/test/API/tools/lldb-dap/extendedStackTrace/main.m
new file mode 100644
index 00000000000000..d513880236c517
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/extendedStackTrace/main.m
@@ -0,0 +1,28 @@
+#import <dispatch/dispatch.h>
+#include <stdio.h>
+
+void one() {
+  printf("one...\n"); // breakpoint 1
+}
+
+void two() {
+  printf("two...\n");
+  one();
+}
+
+void three() {
+  printf("three...\n");
+  two();
+}
+
+int main(int argc, char *argv[]) {
+  printf("main...\n");
+  // Nest from main queue > global queue > main queue.
+  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
+                 ^{
+                   dispatch_async(dispatch_get_main_queue(), ^{
+                     three();
+                   });
+                 });
+  dispatch_main();
+}
diff --git a/lldb/test/API/tools/lldb-dap/stackTrace/TestDAP_stackTrace.py b/lldb/test/API/tools/lldb-dap/stackTrace/TestDAP_stackTrace.py
index 0d7776faa4a9de..fad5419140f454 100644
--- a/lldb/test/API/tools/lldb-dap/stackTrace/TestDAP_stackTrace.py
+++ b/lldb/test/API/tools/lldb-dap/stackTrace/TestDAP_stackTrace.py
@@ -1,13 +1,11 @@
 """
-Test lldb-dap setBreakpoints request
+Test lldb-dap stackTrace request
 """
 
 
 import os
 
-import dap_server
 import lldbdap_testcase
-from lldbsuite.test import lldbutil
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 
diff --git a/lldb/test/API/tools/lldb-dap/stackTraceMissingFunctionName/TestDAP_stackTraceMissingFunctionName.py b/lldb/test/API/tools/lldb-dap/stackTraceMissingFunctionName/TestDAP_stackTraceMissingFunctionName.py
index a04c752764fbb2..f2131d6a821217 100644
--- a/lldb/test/API/tools/lldb-dap/stackTraceMissingFunctionName/TestDAP_stackTraceMissingFunctionName.py
+++ b/lldb/test/API/tools/lldb-dap/stackTraceMissingFunctionName/TestDAP_stackTraceMissingFunctionName.py
@@ -2,13 +2,8 @@
 Test lldb-dap stack trace response
 """
 
-
-import dap_server
 from lldbsuite.test.decorators import *
-import os
-
 import lldbdap_testcase
-from lldbsuite.test import lldbtest, lldbutil
 
 
 class TestDAP_stackTraceMissingFunctionName(lldbdap_testcase.DAPTestCaseBase):
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 57b93c28ce9301..1fd560f21904ab 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -36,7 +36,6 @@ DAP::DAP()
       focus_tid(LLDB_INVALID_THREAD_ID), stop_at_entry(false), is_attach(false),
       enable_auto_variable_summaries(false),
       enable_synthetic_child_debugging(false),
-      enable_display_extended_backtrace(false),
       restarting_process_id(LLDB_INVALID_PROCESS_ID),
       configuration_done_sent(false), waiting_for_run_in_terminal(false),
       progress_event_reporter(
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 0fc77ac1e81683..c621123f18097f 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -181,7 +181,6 @@ struct DAP {
   bool is_attach;
   bool enable_auto_variable_summaries;
   bool enable_synthetic_child_debugging;
-  bool enable_display_extended_backtrace;
   // The process event thread normally responds to process exited events by
   // shutting down the entire adapter. When we're restarting, we keep the id of
   // the old process here so we can detect this case and keep running.
@@ -193,6 +192,7 @@ struct DAP {
   // Keep track of the last stop thread index IDs as threads won't go away
   // unless we send a "thread" event to indicate the thread exited.
   llvm::DenseSet<lldb::tid_t> thread_ids;
+  std::map<lldb::tid_t, uint32_t> thread_stack_size_cache;
   uint32_t reverse_request_seq;
   std::mutex call_mutex;
   std::map<int /* request_seq */, ResponseCallback /* reply handler */>
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index a8b85f55939e17..dafe54a49186e0 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -1257,6 +1257,46 @@ llvm::json::Value CreateVariable(lldb::SBValue v, int64_t variablesReference,
   return llvm::json::Value(std::move(object));
 }
 
+// "ExceptionDetails": {
+//   "type": "object",
+//   "description": "Detailed information about an exception that has
+//   occurred.", "properties": {
+//     "message": {
+//       "type": "string",
+//       "description": "Message contained in the exception."
+//     },
+//     "typeName": {
+//       "type": "string",
+//       "description": "Short type name of the exception object."
+//     },
+//     "fullTypeName": {
+//       "type": "string",
+//       "description": "Fully-qualified type name of the exception object."
+//     },
+//     "evaluateName": {
+//       "type": "string",
+//       "description": "An expression that can be evaluated in the current
+//       scope to obtain the exception object."
+//     },
+//     "stackTrace": {
+//       "type": "string",
+//       "description": "Stack trace at the time the exception was thrown."
+//     },
+//     "innerException": {
+//       "type": "array",
+//       "items": {
+//         "$ref": "#/definitions/ExceptionDetails"
+//       },
+//       "description": "Details of the exception contained by this exception,
+//       if any."
+//     }
+//   }
+// },
+llvm::json::Value CreateExceptionDetails() {
+  llvm::json::Object object;
+  return llvm::json::Value(std::move(object));
+}
+
 llvm::json::Value CreateCompileUnit(lldb::SBCompileUnit unit) {
   llvm::json::Object object;
   char unit_path_arr[PATH_MAX];
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index 1515f5ba2e5f4d..88df05756a0eaa 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/lldb/tools/lldb-dap/JSONUtils.h
@@ -467,6 +467,9 @@ llvm::json::Value CreateVariable(lldb::SBValue v, int64_t variablesReference,
                                  bool is_name_duplicated = false,
                                  std::optional<std::string> custom_name = {});
 
+/// Create a "ExceptionDetail" object for a LLDB
+llvm::json::Value CreateExceptionDetails();
+
 llvm::json::Value CreateCompileUnit(lldb::SBCompileUnit unit);
 
 /// Create a runInTerminal reverse request object
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 495ed0256120e8..aa34c275fad589 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -701,8 +701,6 @@ void request_attach(const llvm::json::Object &request) {
       GetBoolean(arguments, "enableAutoVariableSummaries", false);
   g_dap.enable_synthetic_child_debugging =
       GetBoolean(arguments, "enableSyntheticChildDebugging", false);
-  g_dap.enable_display_extended_backtrace =
-      GetBoolean(arguments, "enableDisplayExtendedBacktrace", false);
   g_dap.command_escape_prefix =
       GetString(arguments, "commandEscapePrefix", "`");
   g_dap.SetFrameFormat(GetString(arguments, "customFrameFormat"));
@@ -1020,6 +1018,68 @@ void request_disconnect(const llvm::json::Object &request) {
   g_dap.disconnecting = true;
 }
 
+// "ExceptionInfoRequest": {
+//   "allOf": [ { "$ref": "#/definitions/Request" }, {
+//     "type": "object",
+//     "description": "Retrieves the details of the exception that
+//     caused this event to be raised. Clients should only call this request if
+//     the corresponding capability `supportsExceptionInfoR...
[truncated]

@ashgti
Copy link
Contributor Author

ashgti commented Aug 23, 2024

This was based on the comments in #104874

LMKWYT

@ashgti ashgti force-pushed the lldb-dap-stacktrace-refactor branch from e22a5dd to 029a174 Compare August 23, 2024 23:48
Copy link
Member

@walter-erquinigo walter-erquinigo left a comment

Choose a reason for hiding this comment

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

This LGTM, and thank you for your diligence with writing tests.

@ashgti ashgti requested a review from jeffreytan81 August 26, 2024 16:28
@ashgti
Copy link
Contributor Author

ashgti commented Aug 26, 2024

@jeffreytan81 I moved the expensive call into the exceptionInfo request, this is only going to be triggered if a thread has the stop event = 'exception', so it shouldn't be triggered while stepping. This does remove the option you added in the prior commit, since I think this isolated the expensive call to a place where the additional information it provides justifies the call and is only called in specific circumstances.

LMKWYT

body.try_emplace("totalFrames", totalFrames);
// If we loaded all the frames, set the total frame to the current total,
// otherwise use the totalFrames to indiciate more data is available.
body.try_emplace("totalFrames",
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested this? Since you are returning one more frame, I am not sure if this is causing VSCode client to ask for one more frame when you click "Get More frames" link. I believe @clayborg suggested defining a page size (like 20 frames), then if it is not done, return startFrame + stackFrames.size() + STACK_PAGE_SIZE instead. Then, client will ask for STACK_PAGE_SIZE instead of 1 more frame.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

I had tested this but the 20 page size does work better with deep stack traces.

Also updated the tests to verify this as well.

Copy link

github-actions bot commented Aug 28, 2024

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

Copy link

github-actions bot commented Aug 28, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@jeffreytan81
Copy link
Contributor

@ashgti, is there any concern to guard this feature behind a launch configuration option? I am happy original profile trace hot path is moved to a rare DAP request, but I am not sure about the cost of GetExtendedBacktraceThread() and if it will show up as next hot path in the new profile trace. I think it would be safer to make this feature opt-in instead of enabling by default. For example, the newly reported perf issue on Windows (msys2/MINGW-packages#21777) may or may not be related with this feature.

Also, if I understand it correctly, the extended backtrace is a plugin style feature which any runtime can implement its own extended backtrace however expensive it is. This means we can't foresee the future cost of enabling this. Adding a knob for it would be useful for people to quickly opt in/out.

@ashgti
Copy link
Contributor Author

ashgti commented Aug 29, 2024

Reverted the removal of enableDisplayExtendedBacktrace and added enableDisplayExtendedBacktrace to the documentation and package.json description of the lldb-dap debugger.

// *NOTE*: Threads can be chained across mutliple backtraces, so we
// need to keep track of each backtrace we've traversed fully in the
// offset.
while (!frame.IsValid() && current.IsValid() && !threadCluster.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: it may worth moving extended back trace specific code into a dedicated helper function to improve readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay, I was out for a week.

I refactored the request_stackTrace implementation and added some additional tests to validate the pagination behavior and ensure stack traces across thread backtraces are handled correctly.

…ers.

Refactoring `stackTrace` to perform frame look ups in a more on-demand fashion to improve overall performance.

Additionally adding additional information to the `exceptionInfo` request to report exception stacks there instead of merging the exception stack into the stack trace. The `exceptionInfo` request is only called if a stop event occurs with `reason='exception'`, which should mitigate the performance of `SBThread::GetCurrentException` calls.

Adding unit tests for exception handling and stack trace supporting.
…tead of only a +1.

This helps improve pagination of deep stack traces.
…ace` is true and document `enableDisplayExtendedBacktrace`.
@ashgti ashgti force-pushed the lldb-dap-stacktrace-refactor branch from caf309f to ecfd5b1 Compare September 10, 2024 16:53
@ashgti ashgti merged commit 5b4100c into llvm:main Sep 10, 2024
7 checks passed
@ashgti ashgti deleted the lldb-dap-stacktrace-refactor branch September 10, 2024 19:40
JDevlieghere pushed a commit to swiftlang/llvm-project that referenced this pull request Oct 16, 2024
…ers (llvm#105905)

Refactoring `stackTrace` to perform frame look ups in a more on-demand
fashion to improve overall performance.

Additionally adding additional information to the `exceptionInfo`
request to report exception stacks there instead of merging the
exception stack into the stack trace. The `exceptionInfo` request is
only called if a stop event occurs with `reason='exception'`, which
should mitigate the performance of `SBThread::GetCurrentException`
calls.

Adding unit tests for exception handling and stack trace supporting.

(cherry picked from commit 5b4100c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants