Make breakpoint stop reason more accurate for function breakpoints

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")

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

@@ -171,3 +171,37 @@ def test_functionality(self):
         self.continue_to_next_stop()
         x_val = self.dap_server.get_local_variable_value("x")
         self.assertEqual(x_val, "10")
+
+    @skipIfWindows
+    def test_breakpoint_reason(self):
+        """Tests setting data breakpoints on variable.
+        Verify that the breakpoint has the correct reason
+        and description in the stopped event."""
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+        source = "main.cpp"
+        first_loop_break_line = line_number(source, "// first loop breakpoint")
+        self.set_source_breakpoints(source, [first_loop_break_line])
+        self.continue_to_next_stop()
+        self.dap_server.get_local_variables()
+        # Test write watchpoints on x
+        response_x = self.dap_server.request_dataBreakpointInfo(1, "x")
+        # Test response from dataBreakpointInfo request.
+        self.assertEqual(response_x["body"]["dataId"].split("/")[1], "4")
+        self.assertEqual(response_x["body"]["accessTypes"], self.accessTypes)
+        dataBreakpoints = [
+            {"dataId": response_x["body"]["dataId"], "accessType": "write"},
+        ]
+        set_response = self.dap_server.request_setDataBreakpoint(dataBreakpoints)
+        self.assertEqual(
+            set_response["body"]["breakpoints"],
+            [{"verified": True}],
+        )
+
+        stopped_events = self.continue_to_next_stop()
+        self.assertEqual(len(stopped_events), 1)
+        stopped_event = stopped_events[0]
+        self.assertEqual(stopped_event["body"]["reason"], "data breakpoint")
+        self.assertEqual(stopped_event["body"]["description"], "data breakpoint 1.1")
+        x_val = self.dap_server.get_local_variable_value("x")
+        self.assertEqual(x_val, "2")

lldb/tools/lldb-dap/DAP.cpp

@@ -183,17 +183,6 @@ ExceptionBreakpoint *DAP::GetExceptionBreakpoint(const std::string &filter) {
   return nullptr;
 }
 
-ExceptionBreakpoint *DAP::GetExceptionBreakpoint(const lldb::break_id_t bp_id) {
-  // See comment in the other GetExceptionBreakpoint().
-  PopulateExceptionBreakpoints();
-
-  for (auto &bp : *exception_breakpoints) {
-    if (bp.bp.GetID() == bp_id)
-      return &bp;
-  }
-  return nullptr;
-}
-
 llvm::Error DAP::ConfigureIO(std::FILE *overrideOut, std::FILE *overrideErr) {
   in = lldb::SBFile(std::fopen(DEV_NULL, "r"), /*transfer_ownership=*/true);
 
@@ -444,27 +433,6 @@ DAP::SendFormattedOutput(OutputType o, const char *format, ...) {
       o, llvm::StringRef(buffer, std::min<int>(actual_length, sizeof(buffer))));
 }
 
-ExceptionBreakpoint *DAP::GetExceptionBPFromStopReason(lldb::SBThread &thread) {
-  const auto num = thread.GetStopReasonDataCount();
-  // Check to see if have hit an exception breakpoint and change the
-  // reason to "exception", but only do so if all breakpoints that were
-  // hit are exception breakpoints.
-  ExceptionBreakpoint *exc_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 exception breakpoint that is getting hit.
-    lldb::break_id_t bp_id = thread.GetStopReasonDataAtIndex(i);
-    exc_bp = GetExceptionBreakpoint(bp_id);
-    // If any breakpoint is not an exception breakpoint, then stop and
-    // report this as a normal breakpoint
-    if (exc_bp == nullptr)
-      return nullptr;
-  }
-  return exc_bp;
-}
-
 lldb::SBThread DAP::GetLLDBThread(const llvm::json::Object &arguments) {
   auto tid = GetInteger<int64_t>(arguments, "threadId")
                  .value_or(LLDB_INVALID_THREAD_ID);
@@ -1046,34 +1014,6 @@ void DAP::SetThreadFormat(llvm::StringRef format) {
   }
 }
 
-InstructionBreakpoint *
-DAP::GetInstructionBreakpoint(const lldb::break_id_t bp_id) {
-  for (auto &bp : instruction_breakpoints) {
-    if (bp.second.bp.GetID() == bp_id)
-      return &bp.second;
-  }
-  return nullptr;
-}
-
-InstructionBreakpoint *
-DAP::GetInstructionBPFromStopReason(lldb::SBThread &thread) {
-  const auto num = thread.GetStopReasonDataCount();
-  InstructionBreakpoint *inst_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 instruction breakpoint that is getting hit.
-    lldb::break_id_t bp_id = thread.GetStopReasonDataAtIndex(i);
-    inst_bp = GetInstructionBreakpoint(bp_id);
-    // If any breakpoint is not an instruction breakpoint, then stop and
-    // report this as a normal breakpoint
-    if (inst_bp == nullptr)
-      return nullptr;
-  }
-  return inst_bp;
-}
-
 lldb::SBValueList *Variables::GetTopLevelScope(int64_t variablesReference) {
   switch (variablesReference) {
   case VARREF_LOCALS:

lldb/tools/lldb-dap/DAP.h

@@ -233,7 +233,6 @@ struct DAP {
   /// @}
 
   ExceptionBreakpoint *GetExceptionBreakpoint(const std::string &filter);
-  ExceptionBreakpoint *GetExceptionBreakpoint(const lldb::break_id_t bp_id);
 
   /// Redirect stdout and stderr fo the IDE's console output.
   ///
@@ -392,9 +391,64 @@ struct DAP {
 
   void SetThreadFormat(llvm::StringRef format);
 
-  InstructionBreakpoint *GetInstructionBreakpoint(const lldb::break_id_t bp_id);
+  // Check to see if we have hit the  <BreakpointType> breakpoint and return the
+  // last breakpoint. but only do so if all breakpoints that were hit are of
+  // <BreakpointType>.
+  // Caller uses this to set the reason accordingly. i.e. If all the breakpoints
+  // are exception breakpoints then reason is 'exception'; if all the
+  // breakpoints are function breakpoints then reason is 'function breakpoint';
+  // if all the breakpoints are instruction breakpoints then reason =
+  // 'instruction breakpoint'; if there are combination of one more breakpoints,
+  // then the reason will be 'breakpoint'.
+  template <typename BreakpointType>
+  BreakpointType *GetBreakpointFromStopReason(lldb::SBThread &thread) {
+    // Check to see if we have hit the  <BreakpointType> breakpoint and change
+    // the reason accordingly, but only do so if all breakpoints that were hit
+    // are of <BreakpointType>.
+    const auto num = thread.GetStopReasonDataCount();
+    BreakpointType *bp = nullptr;
+    for (size_t i = 0; i < num; i += 2) {
+      lldb::break_id_t bp_id = thread.GetStopReasonDataAtIndex(i);
+      // If any breakpoint is not the <BreakpointType>, then stop and
+      // report this as a normal breakpoint
+      bp = GetBreakpoint<BreakpointType>(bp_id);
+      if (bp == nullptr)
+        return nullptr;
+    }
+    return bp;
+  }
+
+  template <typename BreakpointType>
+  BreakpointType *GetBreakpoint(const lldb::break_id_t bp_id);
+
+  template <>
+  FunctionBreakpoint *
+  GetBreakpoint<FunctionBreakpoint>(const lldb::break_id_t bp_id) {
+    auto it = std::find_if(
+        function_breakpoints.begin(), function_breakpoints.end(),
+        [bp_id](const auto &bp) { return bp.second.bp.GetID() == bp_id; });
+    return it != function_breakpoints.end() ? &it->second : nullptr;
+  }
 
-  InstructionBreakpoint *GetInstructionBPFromStopReason(lldb::SBThread &thread);
+  template <>
+  InstructionBreakpoint *
+  GetBreakpoint<InstructionBreakpoint>(const lldb::break_id_t bp_id) {
+    auto it = std::find_if(
+        instruction_breakpoints.begin(), instruction_breakpoints.end(),
+        [bp_id](const auto &bp) { return bp.second.bp.GetID() == bp_id; });
+    return it != instruction_breakpoints.end() ? &it->second : nullptr;
+  }
+
+  template <>
+  ExceptionBreakpoint *
+  GetBreakpoint<ExceptionBreakpoint>(const lldb::break_id_t bp_id) {
+    PopulateExceptionBreakpoints();
+
+    auto it = std::find_if(
+        exception_breakpoints->begin(), exception_breakpoints->end(),
+        [bp_id](const auto &bp) { return bp.bp.GetID() == bp_id; });
+    return it != exception_breakpoints->end() ? &*it : nullptr;
+  }
 };
 
 } // namespace lldb_dap

lldb/tools/lldb-dap/Handler/ExceptionInfoRequestHandler.cpp

@@ -123,7 +123,8 @@ void ExceptionInfoRequestHandler::operator()(
     if (stopReason == lldb::eStopReasonSignal)
       body.try_emplace("exceptionId", "signal");
     else if (stopReason == lldb::eStopReasonBreakpoint) {
-      ExceptionBreakpoint *exc_bp = dap.GetExceptionBPFromStopReason(thread);
+      ExceptionBreakpoint *exc_bp =
+          dap.GetBreakpointFromStopReason<ExceptionBreakpoint>(thread);
       if (exc_bp) {
         EmplaceSafeString(body, "exceptionId", exc_bp->filter);
         EmplaceSafeString(body, "description", exc_bp->label);

lldb/tools/lldb-dap/JSONUtils.cpp

@@ -962,28 +962,47 @@ llvm::json::Value CreateThreadStopped(DAP &dap, lldb::SBThread &thread,
     body.try_emplace("reason", "step");
     break;
   case lldb::eStopReasonBreakpoint: {
-    ExceptionBreakpoint *exc_bp = dap.GetExceptionBPFromStopReason(thread);
+    // Setting the reason as per DAP protocol spec.
+    // https://microsoft.github.io/debug-adapter-protocol/specification#Events_Stopped
+    const auto *exc_bp =
+        dap.GetBreakpointFromStopReason<ExceptionBreakpoint>(thread);
     if (exc_bp) {
       body.try_emplace("reason", "exception");
       EmplaceSafeString(body, "description", exc_bp->label);
     } else {
-      InstructionBreakpoint *inst_bp =
-          dap.GetInstructionBPFromStopReason(thread);
+      llvm::StringRef reason = "breakpoint";
+      const auto *inst_bp =
+          dap.GetBreakpointFromStopReason<InstructionBreakpoint>(thread);
       if (inst_bp) {
-        body.try_emplace("reason", "instruction breakpoint");
+        reason = "instruction breakpoint";
       } else {
-        body.try_emplace("reason", "breakpoint");
+        const auto *function_bp =
+            dap.GetBreakpointFromStopReason<FunctionBreakpoint>(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);
     }
   } break;
-  case lldb::eStopReasonWatchpoint:
+  case lldb::eStopReasonWatchpoint: {
+    // Assuming that all watch points are data breakpoints.
+    // data breakpoints are DAP terminology for watchpoints.
+    // https://microsoft.github.io/debug-adapter-protocol/specification#Requests_SetDataBreakpoints
+    body.try_emplace("reason", "data breakpoint");
+    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("data breakpoint {0}.{1}", bp_id, bp_loc_id);
+    EmplaceSafeString(body, "description", desc_str);
+  } break;
   case lldb::eStopReasonInstrumentation:
     body.try_emplace("reason", "breakpoint");
     break;