Make breakpoint stop reason more accurate for function breakpoints #130841

satyajanga · 2025-03-11T21:23:35Z

VSCode uses DAP protocol (https://microsoft.github.io/debug-adapter-protocol/specification) to lldb-dap for native debugging. For breakpoint, current lldb-dap implementation simply returns "breakpoint" stop reason for most breakpoints.

However, DAP Stopped event (https://microsoft.github.io/debug-adapter-protocol/specification#Events_Stopped) actually supports more accurate breakpoint types such as 'function breakpoint' | 'data breakpoint which we did not leverage.

This task aims to support showing function breakpoint and data breakpoint stop reason in VSCode.

Test Plan:
following tests already test the functionality for function and instruction breakpoints,

tools/lldb-dap/attach/TestDAP_attach.py : creates function breakpoints tests that they are hit.
tools/lldb-dap/breakpoint/TestDAP_setFunctionBreakpoints.py also creates breakpoints tests that they are hit.
They test the reason and description in DAPTestCaseBase.verify_breakpoint_hit
For Data Break points (watch points) added a new testcase

github-actions · 2025-03-11T21:23:56Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-03-11T21:24:27Z

@llvm/pr-subscribers-lldb

Author: None (satyajanga)

Changes

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

4 Files Affected:

(modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/lldbdap_testcase.py (+2-1)
(modified) lldb/tools/lldb-dap/DAP.cpp (+26)
(modified) lldb/tools/lldb-dap/DAP.h (+9-3)
(modified) lldb/tools/lldb-dap/JSONUtils.cpp (+9-3)

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 70b04b051e0ec..5faf83ca826a0 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
@@ -86,6 +86,7 @@ def verify_breakpoint_hit(self, breakpoint_ids):
                 if (
                     body["reason"] != "breakpoint"
                     and body["reason"] != "instruction breakpoint"
+                    and body["reason"] != "function breakpoint"
                 ):
                     continue
                 if "description" not in body:
@@ -100,7 +101,7 @@ def verify_breakpoint_hit(self, breakpoint_ids):
                 # location.
                 description = body["description"]
                 for breakpoint_id in breakpoint_ids:
-                    match_desc = "breakpoint %s." % (breakpoint_id)
+                    match_desc = "%s %s." % (body["reason"], breakpoint_id)
                     if match_desc in description:
                         return
         self.assertTrue(False, "breakpoint not hit")
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index edd3b31be8ff7..485c420dc59e8 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -1128,6 +1128,32 @@ DAP::GetInstructionBPFromStopReason(lldb::SBThread &thread) {
   return inst_bp;
 }
 
+FunctionBreakpoint *DAP::GetFunctionBPFromStopReason(lldb::SBThread &thread) {
+  const auto num = thread.GetStopReasonDataCount();
+  FunctionBreakpoint *func_bp = nullptr;
+  for (size_t i = 0; i < num; i += 2) {
+    // thread.GetStopReasonDataAtIndex(i) will return the bp ID and
+    // thread.GetStopReasonDataAtIndex(i+1) will return the location
+    // within that breakpoint. We only care about the bp ID so we can
+    // see if this is an function breakpoint that is getting hit.
+    lldb::break_id_t bp_id = thread.GetStopReasonDataAtIndex(i);
+    func_bp = GetFunctionBreakPoint(bp_id);
+    // If any breakpoint is not an function breakpoint, then stop and
+    // report this as a normal breakpoint
+    if (func_bp == nullptr)
+      return nullptr;
+  }
+  return func_bp;
+}
+
+FunctionBreakpoint *DAP::GetFunctionBreakPoint(const lldb::break_id_t bp_id) {
+  for (auto &bp : function_breakpoints) {
+    if (bp.second.bp.GetID() == bp_id)
+      return &bp.second;
+  }
+  return nullptr;
+}
+
 lldb::SBValueList *Variables::GetTopLevelScope(int64_t variablesReference) {
   switch (variablesReference) {
   case VARREF_LOCALS:
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 3ff1992b61f5b..ea72b41d02516 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -125,21 +125,21 @@ struct Variables {
 
 struct StartDebuggingRequestHandler : public lldb::SBCommandPluginInterface {
   DAP &dap;
-  explicit StartDebuggingRequestHandler(DAP &d) : dap(d) {};
+  explicit StartDebuggingRequestHandler(DAP &d) : dap(d){};
   bool DoExecute(lldb::SBDebugger debugger, char **command,
                  lldb::SBCommandReturnObject &result) override;
 };
 
 struct ReplModeRequestHandler : public lldb::SBCommandPluginInterface {
   DAP &dap;
-  explicit ReplModeRequestHandler(DAP &d) : dap(d) {};
+  explicit ReplModeRequestHandler(DAP &d) : dap(d){};
   bool DoExecute(lldb::SBDebugger debugger, char **command,
                  lldb::SBCommandReturnObject &result) override;
 };
 
 struct SendEventRequestHandler : public lldb::SBCommandPluginInterface {
   DAP &dap;
-  explicit SendEventRequestHandler(DAP &d) : dap(d) {};
+  explicit SendEventRequestHandler(DAP &d) : dap(d){};
   bool DoExecute(lldb::SBDebugger debugger, char **command,
                  lldb::SBCommandReturnObject &result) override;
 };
@@ -383,6 +383,12 @@ struct DAP {
 
   InstructionBreakpoint *GetInstructionBPFromStopReason(lldb::SBThread &thread);
 
+  FunctionBreakpoint *GetFunctionBPFromStopReason(lldb::SBThread &thread);
+
+  FunctionBreakpoint *GetFunctionBreakPoint(const lldb::break_id_t bp_id);
+
+  void WaitWorkerThreadsToExit();
+
 private:
   // Send the JSON in "json_str" to the "out" stream. Correctly send the
   // "Content-Length:" field followed by the length, followed by the raw
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 932145b1799bd..cd0d518cd4f7d 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -967,17 +967,23 @@ llvm::json::Value CreateThreadStopped(DAP &dap, lldb::SBThread &thread,
       body.try_emplace("reason", "exception");
       EmplaceSafeString(body, "description", exc_bp->label);
     } else {
+      std::string reason = "breakpoint";
       InstructionBreakpoint *inst_bp =
           dap.GetInstructionBPFromStopReason(thread);
       if (inst_bp) {
-        body.try_emplace("reason", "instruction breakpoint");
+        reason = "instruction breakpoint";
       } else {
-        body.try_emplace("reason", "breakpoint");
+        FunctionBreakpoint *function_bp =
+            dap.GetFunctionBPFromStopReason(thread);
+        if (function_bp) {
+          reason = "function breakpoint";
+        }
       }
+      body.try_emplace("reason", reason);
       lldb::break_id_t bp_id = thread.GetStopReasonDataAtIndex(0);
       lldb::break_id_t bp_loc_id = thread.GetStopReasonDataAtIndex(1);
       std::string desc_str =
-          llvm::formatv("breakpoint {0}.{1}", bp_id, bp_loc_id);
+          llvm::formatv("{0} {1}.{2}", reason, bp_id, bp_loc_id);
       body.try_emplace("hitBreakpointIds",
                        llvm::json::Array{llvm::json::Value(bp_id)});
       EmplaceSafeString(body, "description", desc_str);

jeffreytan81

tools/lldb-dap/attach/TestDAP_attach.py : creates function breakpoints tests that they are hit.
tools/lldb-dap/breakpoint/TestDAP_setFunctionBreakpoints.py also creates breakpoints tests that they are hit.

Are these two tests checking "reason" field in stopped DAP event?

jeffreytan81 · 2025-03-11T22:47:40Z

lldb/tools/lldb-dap/DAP.cpp

+FunctionBreakpoint *DAP::GetFunctionBPFromStopReason(lldb::SBThread &thread) {
+  const auto num = thread.GetStopReasonDataCount();
+  FunctionBreakpoint *func_bp = nullptr;
+  for (size_t i = 0; i < num; i += 2) {
+    // thread.GetStopReasonDataAtIndex(i) will return the bp ID and
+    // thread.GetStopReasonDataAtIndex(i+1) will return the location
+    // within that breakpoint. We only care about the bp ID so we can
+    // see if this is an function breakpoint that is getting hit.
+    lldb::break_id_t bp_id = thread.GetStopReasonDataAtIndex(i);
+    func_bp = GetFunctionBreakPoint(bp_id);
+    // If any breakpoint is not an function breakpoint, then stop and
+    // report this as a normal breakpoint
+    if (func_bp == nullptr)
+      return nullptr;
+  }
+  return func_bp;
+}
+
+FunctionBreakpoint *DAP::GetFunctionBreakPoint(const lldb::break_id_t bp_id) {
+  for (auto &bp : function_breakpoints) {
+    if (bp.second.bp.GetID() == bp_id)
+      return &bp.second;
+  }
+  return nullptr;
+}
+


These two functions are exact the same logic as GetInstructionBPFromStopReason and GetInstructionBreakpoint. Can you refactor them into a common function with template for FunctionBreakpoint vs InstructionBreakpoint?

Done. I also refactored in Exception Break points.

Curious to know your thoughts about putting the specializations in header file. Another thought was adding DAP-inl.h and add them there.

jeffreytan81 · 2025-03-11T22:48:21Z

lldb/tools/lldb-dap/DAP.h

+  explicit StartDebuggingRequestHandler(DAP &d) : dap(d){};
  bool DoExecute(lldb::SBDebugger debugger, char **command,
                 lldb::SBCommandReturnObject &result) override;
 };

 struct ReplModeRequestHandler : public lldb::SBCommandPluginInterface {
  DAP &dap;
-  explicit ReplModeRequestHandler(DAP &d) : dap(d) {};
+  explicit ReplModeRequestHandler(DAP &d) : dap(d){};
  bool DoExecute(lldb::SBDebugger debugger, char **command,
                 lldb::SBCommandReturnObject &result) override;
 };

 struct SendEventRequestHandler : public lldb::SBCommandPluginInterface {
  DAP &dap;
-  explicit SendEventRequestHandler(DAP &d) : dap(d) {};
+  explicit SendEventRequestHandler(DAP &d) : dap(d){};


Undo these unrelated changes (you can do in VSCode's diff view).

jeffreytan81 · 2025-03-11T22:48:38Z

lldb/tools/lldb-dap/DAP.h

+
+  FunctionBreakpoint *GetFunctionBreakPoint(const lldb::break_id_t bp_id);
+
+  void WaitWorkerThreadsToExit();


What is this?

Removed it, it got added when resolving the merge conflict.

jeffreytan81 · 2025-03-11T22:49:22Z

lldb/tools/lldb-dap/JSONUtils.cpp

@@ -967,17 +967,23 @@ llvm::json::Value CreateThreadStopped(DAP &dap, lldb::SBThread &thread,
      body.try_emplace("reason", "exception");
      EmplaceSafeString(body, "description", exc_bp->label);
    } else {
+      std::string reason = "breakpoint";


You do not need to create a std::string object, simply use StringRef or char * is enough.

satyajanga · 2025-03-12T18:39:47Z

tools/lldb-dap/attach/TestDAP_attach.py : creates function breakpoints tests that they are hit.
tools/lldb-dap/breakpoint/TestDAP_setFunctionBreakpoints.py also creates breakpoints tests that they are hit.

Are these two tests checking "reason" field in stopped DAP event?

satyajanga · 2025-03-12T18:44:01Z

tools/lldb-dap/attach/TestDAP_attach.py : creates function breakpoints tests that they are hit.
tools/lldb-dap/breakpoint/TestDAP_setFunctionBreakpoints.py also creates breakpoints tests that they are hit.

Are these two tests checking "reason" field in stopped DAP event?

Yes. They call the DAPTestCaseBase.verify_breakpoint_hit it validates the description and reason (indirectly).

Jlalond · 2025-03-14T16:33:16Z

lldb/tools/lldb-dap/DAP.h

+  template <>
+  FunctionBreakpoint *
+  GetBreakpoint<FunctionBreakpoint>(const lldb::break_id_t bp_id) {
+    for (auto &bp : function_breakpoints) {


Because we're duplicating code here three times, would it make more sense that the template controls which breakpoint collection to grab, and then we just have the following logic in every case?

So we'd make GetBreakpoint

template <typename BreakpointType> BreakpointType *GetBreakpoint(const lldb::break_id_t bp_id) { for (auto &bp : GetBreakpoints<>) { if (bp.second.bp.GetID() == bp_id) return &bp.second; } return nullptr; }

Where the new GetBreakpoints call just gets the correct collection?

Also, @satyajanga does the underlying collection support find()? Can we call that instead of enumerating ourselves

updated to use std:find_if

the types for the collections are different, so further templatization is challenging. Let me know if I am mis-undersanding.
exception_breakpoints -> std::optional<std::vector>
function_breakpoints -> llvm::StringMap
instruction_breakpoints -> llvm::DenseMap<lldb::addr_t, InstructionBreakpoint>

I am open to changing. just didn't see a way to control the return collection type for template function that returns the collections.

Didn't realize they're all different. Let's not worry about it

Jlalond · 2025-03-14T16:34:40Z

lldb/tools/lldb-dap/JSONUtils.cpp

@@ -962,28 +962,43 @@ llvm::json::Value CreateThreadStopped(DAP &dap, lldb::SBThread &thread,
    body.try_emplace("reason", "step");
    break;
  case lldb::eStopReasonBreakpoint: {
-    ExceptionBreakpoint *exc_bp = dap.GetExceptionBPFromStopReason(thread);
+    ExceptionBreakpoint *exc_bp =


Nit, const?

(I feel like Miro!)

changed it.

lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py

Jlalond · 2025-03-17T16:36:14Z

lldb/tools/lldb-dap/DAP.h

+    return bp;
+  }
+
+  template <typename BreakpointType> BreakpointType *GetBreakpointCollection();


We can remove this as unused

Jlalond

Left one comment on the unused template. Otherwise LGTM, I'd wait for Jeffrey to give you a final sign off

github-actions · 2025-03-17T16:38:14Z

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

clayborg

Pretty good start. There can be more than one breakpoint at a location and we need to be sure we test these cases. Like we can have an exception breakpoint (which might be at cxa_throw) and we can have a function breakpoint for this as well. We need to make sure we document how we are doing to display this. If we must pick one, then lets be clear about:

exception breakpoints take precedence
then function breakpoints
then source file and line breakpoints

If we have a free form text we can attach to the stop notification it might be nice to see "function breakpoint & exception breakpoint", but if we have to pick just one, then we need to be clear about what we are going to do.

satyajanga · 2025-03-18T17:55:58Z

Pretty good start. There can be more than one breakpoint at a location and we need to be sure we test these cases. Like we can have an exception breakpoint (which might be at cxa_throw) and we can have a function breakpoint for this as well. We need to make sure we document how we are doing to display this. If we must pick one, then lets be clear about:

exception breakpoints take precedence

then function breakpoints

then source file and line breakpoints

If we have a free form text we can attach to the stop notification it might be nice to see "function breakpoint & exception breakpoint", but if we have to pick just one, then we need to be clear about what we are going to do.

Agree that there can be more than one breakpoint at a location.

but 'reason' field in StoppedEvent only takes one string from fixed set of strings.

reason: 'step' | 'breakpoint' | 'exception' | 'pause' | 'entry' | 'goto' | 'function breakpoint' | 'data breakpoint' | 'instruction breakpoint' | string;
https://microsoft.github.io/debug-adapter-protocol/specification#Events_Stopped

So the existing logic for instruction_breakpoint only sets the reason as 'instruction breakpoint' if all the breakpoints are instruction_breakpoints.
Current behavior

if all the breakpoints are exception breakpoints set reason = 'exception'
if all the breakpoints are function breakpoints set reason = 'function breakpoint'
if all the breakpoints are instruction breakpoints set reason = 'instruction breakpoint'
if there are combination of one more breakpoints , then the reason will be 'breakpoint'

I think this is a reasonable behavior, I will document this in the code. @clayborg let me know what you think?

clayborg · 2025-03-24T18:07:18Z

lldb/tools/lldb-dap/DAP.h

-  InstructionBreakpoint *GetInstructionBreakpoint(const lldb::break_id_t bp_id);
+  template <typename BreakpointType>
+  BreakpointType *GetBreakpointFromStopReason(lldb::SBThread &thread) {
+    // Check to see if have hit the <BreakpointType> breakpoint and change the


s/Check to see if have hit/Check to see if we have hit/

clayborg · 2025-03-24T18:10:10Z

lldb/tools/lldb-dap/JSONUtils.cpp

-  case lldb::eStopReasonWatchpoint:
+  case lldb::eStopReasonWatchpoint: {
+    // Assuming that all watch points are data breakpoints.
+    body.try_emplace("reason", "data breakpoint");


Are they called "data breakpoint" in VS Code? If so then this is ok. LLDB uses the term "watchpoint", but we are making this change for VS code.

VS Code also calls them watchpoints.
"data breakpoint" is a DAP terminology.
https://microsoft.github.io/debug-adapter-protocol/specification#Requests_SetDataBreakpoints and they trigger SetDataBreakpointsRequestHandler
https://github.com/llvm/llvm-project/blob/main/lldb/tools/lldb-dap/Handler/SetDataBreakpointsRequestHandler.cpp

satyajanga requested a review from JDevlieghere as a code owner March 11, 2025 21:23

llvmbot added lldb lldb-dap labels Mar 11, 2025

satyajanga marked this pull request as draft March 11, 2025 21:24

satyajanga changed the title ~~Make breakpoint stop reason more accurate~~ Make breakpoint stop reason more accurate for function breakpoints Mar 11, 2025

satyajanga marked this pull request as ready for review March 11, 2025 21:57

jeffreytan81 requested changes Mar 11, 2025

View reviewed changes

jeffreytan81 requested a review from clayborg March 11, 2025 22:51

satyajanga marked this pull request as draft March 11, 2025 22:57

satyajanga closed this Mar 12, 2025

satyajanga reopened this Mar 12, 2025

satyajanga marked this pull request as ready for review March 12, 2025 18:40

satyajanga added 3 commits March 13, 2025 20:33

Make breakpoint stop reason more accurate

e949d2e

Make breakpoint stop reason more accurate

be55209

Make breakpoint stop reason more accurate

7af0265

satyajanga force-pushed the dap_bp_reason branch from 6000508 to 6cc3b96 Compare March 14, 2025 03:35

satyajanga marked this pull request as draft March 14, 2025 03:38

satyajanga force-pushed the dap_bp_reason branch 2 times, most recently from 76f8213 to dc97ca6 Compare March 14, 2025 03:47

satyajanga marked this pull request as ready for review March 14, 2025 03:51

Jlalond reviewed Mar 14, 2025

View reviewed changes

lldb/test/API/tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py Outdated Show resolved Hide resolved

satyajanga force-pushed the dap_bp_reason branch 2 times, most recently from edb06f3 to 23a710f Compare March 16, 2025 06:22

Jlalond reviewed Mar 17, 2025

View reviewed changes

Jlalond approved these changes Mar 17, 2025

View reviewed changes

satyajanga force-pushed the dap_bp_reason branch 2 times, most recently from 9ae819f to 8b5cd8f Compare March 17, 2025 17:16

clayborg reviewed Mar 18, 2025

View reviewed changes

clayborg reviewed Mar 24, 2025

View reviewed changes

Make breakpoint stop reason more accurate

aa90036

satyajanga force-pushed the dap_bp_reason branch from 8b5cd8f to aa90036 Compare March 26, 2025 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make breakpoint stop reason more accurate for function breakpoints #130841

Make breakpoint stop reason more accurate for function breakpoints #130841

satyajanga commented Mar 11, 2025 •

edited

Loading

github-actions bot commented Mar 11, 2025

llvmbot commented Mar 11, 2025

jeffreytan81 left a comment

jeffreytan81 Mar 11, 2025

satyajanga Mar 12, 2025

jeffreytan81 Mar 11, 2025

jeffreytan81 Mar 11, 2025

satyajanga Mar 12, 2025

jeffreytan81 Mar 11, 2025

satyajanga Mar 12, 2025

satyajanga commented Mar 12, 2025

satyajanga commented Mar 12, 2025

Jlalond Mar 14, 2025

Jlalond Mar 14, 2025

satyajanga Mar 16, 2025

Jlalond Mar 16, 2025

Jlalond Mar 14, 2025

satyajanga Mar 16, 2025

Jlalond Mar 17, 2025

Jlalond left a comment

github-actions bot commented Mar 17, 2025 •

edited

Loading

clayborg left a comment

satyajanga commented Mar 18, 2025

clayborg Mar 24, 2025

clayborg Mar 24, 2025

satyajanga Mar 25, 2025


		FunctionBreakpoint *GetFunctionBreakPoint(const lldb::break_id_t bp_id);

		void WaitWorkerThreadsToExit();

Make breakpoint stop reason more accurate for function breakpoints #130841

Are you sure you want to change the base?

Make breakpoint stop reason more accurate for function breakpoints #130841

Conversation

satyajanga commented Mar 11, 2025 • edited Loading

github-actions bot commented Mar 11, 2025

llvmbot commented Mar 11, 2025

jeffreytan81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satyajanga commented Mar 12, 2025

satyajanga commented Mar 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jlalond left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 17, 2025 • edited Loading

clayborg left a comment

Choose a reason for hiding this comment

satyajanga commented Mar 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satyajanga commented Mar 11, 2025 •

edited

Loading

github-actions bot commented Mar 17, 2025 •

edited

Loading