Skip to content
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

[lldb-dap] Enabling instruction breakpoint support to lldb-dap. #105278

Merged
merged 75 commits into from
Aug 26, 2024

Conversation

santhoshe447
Copy link
Contributor

@santhoshe447 santhoshe447 commented Aug 20, 2024

Added support for "supportsInstructionBreakpoints" capability and now it this command is triggered when we set instruction breakpoint.
We need this support as part of enabling disassembly view debugging. Following features should work as part of this feature enablement:

  1. Settings breakpoints in disassembly view: Unsetting the breakpoint is not happening from the disassembly view. Currently we need to unset breakpoint manually from the breakpoint List. Multiple breakpoints are getting set for the same $

  2. Step over, step into, continue in the disassembly view

The format for DisassembleRequest and DisassembleResponse at https://raw.githubusercontent.com/microsoft/vscode/master/src/vs/workbench/contrib/debug/common/debugProtocol.d.ts .

Ref Images:
Set instruction breakpoint in disassembly view:
image

After issuing continue:
image

Santhosh Kumar Ellendula and others added 30 commits November 17, 2023 15:09
Adding a "port" property to the VsCode "attach" command likely extends the functionality of the debugger configuratiuon to allow attaching to a process using PID or PORT number.
Currently, the "Attach" configuration lets the user specify a pid. We tell the user to use the attachCommands property to run "gdb-remote <port>".
Followed the below conditions for "attach" command with "port" and "pid"
We should add a "port" property. If port is specified and pid is not, use that port to attach. If both port and pid are specified, return an error saying that the user can't specify both pid and port.

Ex - launch.json
{
	"version": "0.2.0",
    "configurations": [
        {
            "name": "lldb-dap Debug",
            "type": "lldb-dap",
            "request": "attach",
            "port":1234,
            "program": "${workspaceFolder}/a.out",
            "args": [],
            "stopOnEntry": false,
            "cwd": "${workspaceFolder}",
            "env": [],

        }
    ]
}
Adding a "port" property to the VsCode "attach" command likely extends the functionality of the debugger configuration to allow attaching to a process using PID or PORT number.
Currently, the "Attach" configuration lets the user specify a pid. We tell the user to use the attachCommands property to run "gdb-remote ".
Followed the below conditions for "attach" command with "port" and "pid"
We should add a "port" property. If port is specified and pid is not, use that port to attach. If both port and pid are specified, return an error saying that the user can't specify both pid and port.

Ex - launch.json
{
"version": "0.2.0",
"configurations": [
{
"name": "lldb-dap Debug",
"type": "lldb-dap",
"request": "attach",
"port":1234,
"program": "${workspaceFolder}/a.out",
"args": [],
"stopOnEntry": false,
"cwd": "${workspaceFolder}",
"env": [],

    }
]
}

In this patch we have resolved code formatting issues and fixed "test_by_name" failure.
…ed review comments

Addressed all review comments.
…d all review comments.

All review comments have been addressed, and the attach-by-port tests have been verified on
Linux machine. Although the functionality is intended to support both Linux and macOS, we were
unable to verify it on macOS due to lack of access.
The following changes were committed by mistake, so they have been reverted.
"startDebugging", &g_dap.start_debugging_request_handler,
"repl-mode", &g_dap.repl_mode_request_handler,
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 21, 2024

@llvm/pr-subscribers-lldb

Author: Santhosh Kumar Ellendula (santhoshe447)

Changes

Added support for "supportsInstructionBreakpoints" capability and now it this command is triggered when we set instruction breakpoint.
We need this support as part of enabling disassembly view debugging. Following features should work as part of this feature enablement:

  1. Settings breakpoints in disassembly view: Unsetting the breakpoint is not happening from the disassembly view. Currently we need to unset breakpoint manually from the breakpoint List. Multiple breakpoints are getting set for the same $

  2. Step over, step into, continue in the disassembly view

The format for DisassembleRequest and DisassembleResponse at https://raw.githubusercontent.com/microsoft/vscode/master/src/vs/workbench/contrib/debug/common/debugProtocol.d.ts .

Ref Images:
Set instruction breakpoint in disassembly view:
image

After issuing continue:
image


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

12 Files Affected:

  • (modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py (+14)
  • (added) lldb/test/API/tools/lldb-dap/instruction-breakpoint/Makefile (+6)
  • (added) lldb/test/API/tools/lldb-dap/instruction-breakpoint/TestDAP_instruction_breakpoint.py (+97)
  • (added) lldb/test/API/tools/lldb-dap/instruction-breakpoint/main.cpp (+18)
  • (modified) lldb/tools/lldb-dap/CMakeLists.txt (+1)
  • (modified) lldb/tools/lldb-dap/DAP.h (+2)
  • (modified) lldb/tools/lldb-dap/DAPForward.h (+1)
  • (added) lldb/tools/lldb-dap/InstructionBreakpoint.cpp (+27)
  • (added) lldb/tools/lldb-dap/InstructionBreakpoint.h (+36)
  • (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+96)
  • (modified) lldb/tools/lldb-dap/JSONUtils.h (+11)
  • (modified) lldb/tools/lldb-dap/lldb-dap.cpp (+180)
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 a324af57b61df3..98b599608946c0 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
@@ -1095,6 +1095,20 @@ def terminate(self):
         self.send.close()
         # self.recv.close()
 
+    def request_setInstructionBreakpoints(self, memory_reference=[]):
+        breakpoints = []
+        for i in memory_reference:
+            args_dict = {
+                "instructionReference": i,
+            }
+            breakpoints.append(args_dict)
+        args_dict = {"breakpoints": breakpoints}
+        command_dict = {
+            "command": "setInstructionBreakpoints",
+            "type": "request",
+            "arguments": args_dict,
+        }
+        return self.send_recv(command_dict)
 
 class DebugAdaptorServer(DebugCommunication):
     def __init__(
diff --git a/lldb/test/API/tools/lldb-dap/instruction-breakpoint/Makefile b/lldb/test/API/tools/lldb-dap/instruction-breakpoint/Makefile
new file mode 100644
index 00000000000000..714cd70b0bd35c
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/instruction-breakpoint/Makefile
@@ -0,0 +1,6 @@
+CXX_SOURCES := main-copy.cpp
+CXXFLAGS_EXTRAS := -O1 -g
+include Makefile.rules
+
+main-copy.cpp: main.cpp
+	cp -f $< $@
diff --git a/lldb/test/API/tools/lldb-dap/instruction-breakpoint/TestDAP_instruction_breakpoint.py b/lldb/test/API/tools/lldb-dap/instruction-breakpoint/TestDAP_instruction_breakpoint.py
new file mode 100644
index 00000000000000..ed1a6bf250c761
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/instruction-breakpoint/TestDAP_instruction_breakpoint.py
@@ -0,0 +1,97 @@
+import dap_server
+import shutil
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import lldbdap_testcase
+import os
+import lldb
+
+
+class TestDAP_InstructionBreakpointTestCase(lldbdap_testcase.DAPTestCaseBase):
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def setUp(self):
+        lldbdap_testcase.DAPTestCaseBase.setUp(self)
+
+        self.main_basename = "main-copy.cpp"
+        self.main_path = os.path.realpath(self.getBuildArtifact(self.main_basename))
+
+    def test_instruction_breakpoint(self):
+        self.build()
+        self.instruction_breakpoint_test()
+
+    def instruction_breakpoint_test(self):
+        """Sample test to ensure SBFrame::Disassemble produces SOME output"""
+        # Create a target by the debugger.
+        target = self.createTestTarget()
+
+        main_line = line_number("main.cpp", "breakpoint 1")
+
+        program = self.getBuildArtifact("a.out")
+        self.build_and_launch(program)
+
+        # Set source breakpoint 1
+        response = self.dap_server.request_setBreakpoints(self.main_path, [main_line])
+        breakpoints = response["body"]["breakpoints"]
+        self.assertEquals(len(breakpoints), 1)
+        breakpoint = breakpoints[0]
+        self.assertEqual(
+            breakpoint["line"], main_line, "incorrect breakpoint source line"
+        )
+        self.assertTrue(breakpoint["verified"], "breakpoint is not verified")
+        self.assertEqual(
+            self.main_basename, breakpoint["source"]["name"], "incorrect source name"
+        )
+        self.assertEqual(
+            self.main_path, breakpoint["source"]["path"], "incorrect source file path"
+        )
+        other_breakpoint_id = breakpoint["id"]
+
+        # Continue and then verifiy the breakpoint
+        self.dap_server.request_continue()
+        self.verify_breakpoint_hit([other_breakpoint_id])
+
+        # now we check the stack trace making sure that we got mapped source paths
+        frames = self.dap_server.request_stackTrace()["body"]["stackFrames"]
+        intstructionPointerReference = []
+        setIntstructionBreakpoints = []
+        intstructionPointerReference.append(frames[0]["instructionPointerReference"])
+        self.assertEqual(
+            frames[0]["source"]["name"], self.main_basename, "incorrect source name"
+        )
+        self.assertEqual(
+            frames[0]["source"]["path"], self.main_path, "incorrect source file path"
+        )
+
+        # Check disassembly view
+        instruction = self.disassemble(frameIndex=0)
+        self.assertEqual(
+            instruction["address"],
+            intstructionPointerReference[0],
+            "current breakpoint reference is not in the disaasembly view",
+        )
+
+        # Get next instruction address to set instruction breakpoint
+        disassembled_instruction_list = self.dap_server.disassembled_instructions
+        instruction_addr_list = list(disassembled_instruction_list.keys())
+        index = instruction_addr_list.index(intstructionPointerReference[0])
+        if len(instruction_addr_list) >= (index + 1):
+            next_inst_addr = int(instruction_addr_list[index + 1], 16)
+            if next_inst_addr is not 0:
+                setIntstructionBreakpoints.append(hex(next_inst_addr))
+                instruction_breakpoint_response = (
+                    self.dap_server.request_setInstructionBreakpoints(
+                        setIntstructionBreakpoints
+                    )
+                )
+                inst_breakpoints = instruction_breakpoint_response["body"][
+                    "breakpoints"
+                ]
+                self.assertEqual(
+                    inst_breakpoints[0]["instructionReference"],
+                    hex(next_inst_addr),
+                    "Instruction breakpoint has not been resolved or failed to relocate the instruction breakpoint",
+                )
+                self.dap_server.request_continue()
+                self.verify_breakpoint_hit([inst_breakpoints[0]["id"]])
diff --git a/lldb/test/API/tools/lldb-dap/instruction-breakpoint/main.cpp b/lldb/test/API/tools/lldb-dap/instruction-breakpoint/main.cpp
new file mode 100644
index 00000000000000..3c710d64171570
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/instruction-breakpoint/main.cpp
@@ -0,0 +1,18 @@
+#include <stdio.h>
+#include <unistd.h>
+
+int function(int x) {
+
+  if (x == 0) // breakpoint 1
+    return x;
+
+  if ((x % 2) != 0)
+    return x;
+  else
+    return function(x - 1) + x;
+}
+
+int main(int argc, char const *argv[]) {
+  int n = function(2);
+  return n;
+}
\ No newline at end of file
diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt
index f8f0d86453f585..d68098bf7b3266 100644
--- a/lldb/tools/lldb-dap/CMakeLists.txt
+++ b/lldb/tools/lldb-dap/CMakeLists.txt
@@ -38,6 +38,7 @@ add_lldb_tool(lldb-dap
   SourceBreakpoint.cpp
   DAP.cpp
   Watchpoint.cpp
+  InstructionBreakpoint.cpp
 
   LINK_LIBS
     liblldb
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 7828272aa15a7d..8be5a5a95aa38a 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -68,6 +68,8 @@ namespace lldb_dap {
 
 typedef llvm::DenseMap<uint32_t, SourceBreakpoint> SourceBreakpointMap;
 typedef llvm::StringMap<FunctionBreakpoint> FunctionBreakpointMap;
+typedef llvm::DenseMap<lldb::addr_t, InstructionBreakpoint>
+    InstructionBreakpointMap;
 enum class OutputType { Console, Stdout, Stderr, Telemetry };
 
 enum DAPBroadcasterBits {
diff --git a/lldb/tools/lldb-dap/DAPForward.h b/lldb/tools/lldb-dap/DAPForward.h
index 8c79488fae8dbf..159d999a63c820 100644
--- a/lldb/tools/lldb-dap/DAPForward.h
+++ b/lldb/tools/lldb-dap/DAPForward.h
@@ -15,6 +15,7 @@ struct ExceptionBreakpoint;
 struct FunctionBreakpoint;
 struct SourceBreakpoint;
 struct Watchpoint;
+struct InstructionBreakpoint;
 } // namespace lldb_dap
 
 namespace lldb {
diff --git a/lldb/tools/lldb-dap/InstructionBreakpoint.cpp b/lldb/tools/lldb-dap/InstructionBreakpoint.cpp
new file mode 100644
index 00000000000000..d4d8db5c51ec15
--- /dev/null
+++ b/lldb/tools/lldb-dap/InstructionBreakpoint.cpp
@@ -0,0 +1,27 @@
+//===-- InstructionBreakpoint.cpp ------------------------------------*- C++
+//-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "InstructionBreakpoint.h"
+#include "DAP.h"
+
+namespace lldb_dap {
+
+// Instruction Breakpoint
+InstructionBreakpoint::InstructionBreakpoint(const llvm::json::Object &obj)
+    : Breakpoint(obj), instructionReference(LLDB_INVALID_ADDRESS), id(0),
+      offset(GetSigned(obj, "offset", 0)) {
+  GetString(obj, "instructionReference").getAsInteger(0, instructionReference);
+  instructionReference += offset;
+}
+
+void InstructionBreakpoint::SetInstructionBreakpoint() {
+  bp = g_dap.target.BreakpointCreateByAddress(instructionReference);
+  id = bp.GetID();
+}
+} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/InstructionBreakpoint.h b/lldb/tools/lldb-dap/InstructionBreakpoint.h
new file mode 100644
index 00000000000000..a9b94f648aeab7
--- /dev/null
+++ b/lldb/tools/lldb-dap/InstructionBreakpoint.h
@@ -0,0 +1,36 @@
+//===-- InstructionBreakpoint.h --------------------------------------*- C++
+//-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_TOOLS_LLDB_DAP_INSTRUCTIONBREAKPOINT_H
+#define LLDB_TOOLS_LLDB_DAP_INSTRUCTIONBREAKPOINT_H
+
+#include "Breakpoint.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace lldb_dap {
+
+// Instruction Breakpoint
+struct InstructionBreakpoint : public Breakpoint {
+
+  lldb::addr_t instructionReference;
+  int32_t id;
+  int32_t offset;
+
+  InstructionBreakpoint()
+      : Breakpoint(), instructionReference(LLDB_INVALID_ADDRESS), id(0),
+        offset(0) {}
+  InstructionBreakpoint(const llvm::json::Object &obj);
+
+  // Set instruction breakpoint in LLDB as a new breakpoint
+  void SetInstructionBreakpoint();
+};
+
+} // namespace lldb_dap
+
+#endif
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index a8b85f55939e17..5a2a6c044ea4b1 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -766,6 +766,102 @@ llvm::json::Value CreateStackFrame(lldb::SBFrame &frame) {
   return llvm::json::Value(std::move(object));
 }
 
+// Response to `setInstructionBreakpoints` request.
+// "Breakpoint": {
+//   "type": "object",
+//   "description": "Response to `setInstructionBreakpoints` request.",
+//   "properties": {
+//     "id": {
+//       "type": "number",
+//       "description": "The identifier for the breakpoint. It is needed if
+//       breakpoint events are used to update or remove breakpoints."
+//     },
+//     "verified": {
+//       "type": "boolean",
+//       "description": "If true, the breakpoint could be set (but not
+//       necessarily at the desired location."
+//     },
+//     "message": {
+//       "type": "string",
+//       "description": "A message about the state of the breakpoint.
+//       This is shown to the user and can be used to explain why a breakpoint
+//       could not be verified."
+//     },
+//     "source": {
+//       "type": "Source",
+//       "description": "The source where the breakpoint is located."
+//     },
+//     "line": {
+//       "type": "number",
+//       "description": "The start line of the actual range covered by the
+//       breakpoint."
+//     },
+//     "column": {
+//       "type": "number",
+//       "description": "The start column of the actual range covered by the
+//       breakpoint."
+//     },
+//     "endLine": {
+//       "type": "number",
+//       "description": "The end line of the actual range covered by the
+//       breakpoint."
+//     },
+//     "endColumn": {
+//       "type": "number",
+//       "description": "The end column of the actual range covered by the
+//       breakpoint. If no end line is given, then the end column is assumed to
+//       be in the start line."
+//     },
+//     "instructionReference": {
+//       "type": "string",
+//       "description": "A memory reference to where the breakpoint is set."
+//     },
+//     "offset": {
+//       "type": "number",
+//       "description": "The offset from the instruction reference.
+//       This can be negative."
+//     },
+//   },
+//   "required": [ "id", "verified", "line"]
+// }
+llvm::json::Value CreateInstructionBreakpoint(lldb::SBBreakpoint &bp) {
+  llvm::json::Object object;
+  if (!bp.IsValid()) {
+    return llvm::json::Value(std::move(object));
+  }
+  object.try_emplace("verified", bp.GetNumResolvedLocations() > 0);
+  object.try_emplace("id", bp.GetID());
+
+  lldb::SBBreakpointLocation bp_loc;
+  const auto num_locs = bp.GetNumLocations();
+  for (size_t i = 0; i < num_locs; ++i) {
+    bp_loc = bp.GetLocationAtIndex(i);
+    if (bp_loc.IsResolved())
+      break;
+  }
+  // If not locations are resolved, use the first location.
+  if (!bp_loc.IsResolved())
+    bp_loc = bp.GetLocationAtIndex(0);
+  auto bp_addr = bp_loc.GetAddress();
+
+  lldb::addr_t address_load = bp_addr.GetLoadAddress(g_dap.target);
+  std::string address_hex;
+  llvm::raw_string_ostream addr_strm(address_hex);
+  if (address_load != LLDB_INVALID_ADDRESS) {
+    addr_strm << llvm::format_hex(address_load, 0);
+    addr_strm.flush();
+    object.try_emplace("instructionReference", address_hex);
+  }
+
+  if (bp_addr.IsValid()) {
+    auto line_entry = bp_addr.GetLineEntry();
+    const auto line = line_entry.GetLine();
+    if (line != UINT32_MAX)
+      object.try_emplace("line", line);
+  }
+  return llvm::json::Value(std::move(object));
+}
+
 // "Thread": {
 //   "type": "object",
 //   "description": "A Thread",
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index 1515f5ba2e5f4d..693f35ee07e5f6 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/lldb/tools/lldb-dap/JSONUtils.h
@@ -322,6 +322,17 @@ llvm::json::Value CreateSource(llvm::StringRef source_path);
 ///     definition outlined by Microsoft.
 llvm::json::Value CreateStackFrame(lldb::SBFrame &frame);
 
+/// Create a "instruction" object for a LLDB disassemble object as described in
+/// the Visual Studio Code debug adaptor definition.
+///
+/// \param[in] bp
+///     The LLDB instruction object used to populate the disassembly
+///     instruction.
+/// \return
+///     A "Scope" JSON object with that follows the formal JSON
+///     definition outlined by Microsoft.
+llvm::json::Value CreateInstructionBreakpoint(lldb::SBBreakpoint &bp);
+
 /// Create a "Thread" object for a LLDB thread object.
 ///
 /// This function will fill in the following keys in the returned
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index f50a6c17310739..7c0799eaadfb6d 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -1718,6 +1718,8 @@ void request_initialize(const llvm::json::Object &request) {
   body.try_emplace("supportsLogPoints", true);
   // The debug adapter supports data watchpoints.
   body.try_emplace("supportsDataBreakpoints", true);
+  // The debug adapter support for instruction breakpoint.
+  body.try_emplace("supportsInstructionBreakpoints", true);
 
   // Put in non-DAP specification lldb specific information.
   llvm::json::Object lldb_json;
@@ -4046,6 +4048,181 @@ void request__testGetTargetBreakpoints(const llvm::json::Object &request) {
   g_dap.SendJSON(llvm::json::Value(std::move(response)));
 }
 
+// SetInstructionBreakpoints request; value of command field is
+// 'setInstructionBreakpoints'. Replaces all existing instruction breakpoints.
+// Typically, instruction breakpoints would be set from a disassembly window. To
+// clear all instruction breakpoints, specify an empty array. When an
+// instruction breakpoint is hit, a `stopped` event (with reason `instruction
+// breakpoint`) is generated. Clients should only call this request if the
+// corresponding capability `supportsInstructionBreakpoints` is true. interface
+// SetInstructionBreakpointsRequest extends Request {
+//   command: 'setInstructionBreakpoints';
+//   arguments: SetInstructionBreakpointsArguments;
+// }
+// interface SetInstructionBreakpointsArguments {
+//   The instruction references of the breakpoints
+//   breakpoints: InstructionBreakpoint[];
+// }
+// "InstructionBreakpoint ": {
+//   "type": "object",
+//   "description": "Properties of a breakpoint passed to the
+//   setInstructionBreakpoints request.", "properties": {
+//     "instructionReference": {
+//       "type": "string",
+//       "description": "The instruction reference of the breakpoint.
+//       This should be a memory or instruction pointer reference from an
+//       EvaluateResponse, Variable, StackFrame, GotoTarget, or Breakpoint."
+//     },
+//     "offset": {
+//       "type": "number",
+//       "description": "The offset from the instruction reference.
+//       This can be negative."
+//     },
+//     "condition": {
+//       "type": "string",
+//       "description": "An expression for conditional breakpoints.
+//       It is only honored by a debug adapter if the corresponding capability
+//       supportsConditionalBreakpoints` is true."
+//     },
+//     "hitCondition": {
+//       "type": "string",
+//       "description": "An expression that controls how many hits of the
+//       breakpoint are ignored. The debug adapter is expected to interpret the
+//       expression as needed. The attribute is only honored by a debug adapter
+//       if the corresponding capability `supportsHitConditionalBreakpoints` is
+//       true."
+//     },
+// }
+// interface SetInstructionBreakpointsResponse extends Response {
+//   body: {
+//     Information about the breakpoints. The array elements correspond to the
+//     elements of the `breakpoints` array.
+//     breakpoints: Breakpoint[];
+//   };
+// }
+// Response to `setInstructionBreakpoints` request.
+// "Breakpoint": {
+//   "type": "object",
+//   "description": "Response to `setInstructionBreakpoints` request.",
+//   "properties": {
+//     "id": {
+//       "type": "number",
+//       "description": "The identifier for the breakpoint. It is needed if
+//       breakpoint events are used to update or remove breakpoints."
+//     },
+//     "verified": {
+//       "type": "boolean",
+//       "description": "If true, the breakpoint could be set (but not
+//       necessarily at the desired location."
+//     },
+//     "message": {
+//       "type": "string",
+//       "description": "A message about the state of the breakpoint.
+//       This is shown to the user and can be used to explain why a breakpoint
+//       could not be verified."
+//     },
+//     "source": {
+//       "type": "Source",
+//       "description": "The source where the breakpoint is located."
+//     },
+//     "line": {
+//       "type": "number",
+//       "description": "The start line of the actual range covered by the
+//       breakpoint."
+//     },
+//     "column": {
+//       "type": "number",
+//       "description": "The start column of the actual range covered by the
+//       breakpoint."
+//     },
+//     "endLine": {
+//       "type": "number",
+//       "description": "The end line of the actual range covered by the
+//       breakpoint."
+//     },
+//     "endColumn": {
+//       "type": "number",
+//       "description": "The end column of the actual range covered by the
+//       breakpoint. If no end line is given, then the end column is assumed to
+//       be in the start line."
+//     },
+//     "instructionReference": {
+//       "type": "string",
+//       "description": "A memory reference to where the breakpoint is set."
+//     },
+//     "offset": {
+//       "type": "number",
+//       "description": "The offset from the instruction reference.
+//       This can be negative."
+//     },
+//   },
+//   "required": [ "id", "verified", "line"]
+// }
+void request_setInstructionBreakpoints(const llvm::json::Object &request) {
+  llvm::json::Object response;
+  llvm::json::Array response_breakpoints;
+  llvm::json::Object b...
[truncated]

Few changes were missed in lldb/tools/lldb-dap/DAP.h, so added in this version.
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, but I'd like @clayborg to give a second look.
Thanks for this fancy feature, I've been wanting it for a while

lldb/tools/lldb-dap/lldb-dap.cpp Outdated Show resolved Hide resolved
lldb/tools/lldb-dap/lldb-dap.cpp Outdated Show resolved Hide resolved
lldb/tools/lldb-dap/JSONUtils.cpp Outdated Show resolved Hide resolved
Resolved follwoing review comments :
1. Removed usage of removed_ibp, removed deleted instruction breakpoints deirectly from g_dap.instruction_breakpoints.erase(prev_ibp.first)
2. Reused CreateJsonObject function in CreateInstructionBreakpoint.
3. Updated test cases with stop reason.
@santhoshe447
Copy link
Contributor Author

santhoshe447 commented Aug 26, 2024

Hi Everyone,
Thanks for everyone for valuable feedback. Pls let me know if all are ok with the changes.

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. I'd just want a rename from instructionReference to addressReference or instructionAddressReference for clarity.

lldb/tools/lldb-dap/DAP.cpp Outdated Show resolved Hide resolved
lldb/tools/lldb-dap/InstructionBreakpoint.h Outdated Show resolved Hide resolved
lldb/tools/lldb-dap/JSONUtils.cpp Show resolved Hide resolved
lldb/tools/lldb-dap/lldb-dap.cpp Outdated Show resolved Hide resolved
@santhoshe447 santhoshe447 merged commit 89c27d6 into llvm:main Aug 26, 2024
5 of 6 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Aug 26, 2024

LLVM Buildbot has detected a new failure on builder lldb-aarch64-windows running on linaro-armv8-windows-msvc-05 while building lldb at step 6 "test".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/1866

Here is the relevant piece of the build log for the reference
Step 6 (test) failure: build (failure)
...
UNSUPPORTED: lldb-api :: tools/lldb-dap/console/TestDAP_redirection_to_console.py (1124 of 2011)
UNSUPPORTED: lldb-api :: tools/lldb-dap/coreFile/TestDAP_coreFile.py (1125 of 2011)
UNSUPPORTED: lldb-api :: tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py (1126 of 2011)
UNSUPPORTED: lldb-api :: tools/lldb-dap/disassemble/TestDAP_disassemble.py (1127 of 2011)
UNSUPPORTED: lldb-api :: tools/lldb-dap/disconnect/TestDAP_disconnect.py (1128 of 2011)
UNSUPPORTED: lldb-api :: tools/lldb-dap/evaluate/TestDAP_evaluate.py (1129 of 2011)
UNSUPPORTED: lldb-api :: tools/lldb-dap/exception/TestDAP_exception.py (1130 of 2011)
PASS: lldb-api :: tools/lldb-dap/commands/TestDAP_commands.py (1131 of 2011)
UNSUPPORTED: lldb-api :: tools/lldb-dap/launch/TestDAP_launch.py (1132 of 2011)
UNRESOLVED: lldb-api :: tools/lldb-dap/instruction-breakpoint/TestDAP_instruction_breakpoint.py (1133 of 2011)
******************** TEST 'lldb-api :: tools/lldb-dap/instruction-breakpoint/TestDAP_instruction_breakpoint.py' FAILED ********************
Script:
--
C:/Users/tcwg/scoop/apps/python/current/python.exe C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/llvm-project/lldb\test\API\dotest.py -u CXXFLAGS -u CFLAGS --env OBJCOPY=C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./bin/llvm-objcopy.exe --env LLVM_LIBS_DIR=C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./lib --env LLVM_INCLUDE_DIR=C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/include --env LLVM_TOOLS_DIR=C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./bin --arch aarch64 --build-dir C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/lldb-test-build.noindex --lldb-module-cache-dir C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/lldb-test-build.noindex/module-cache-lldb\lldb-api --clang-module-cache-dir C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/lldb-test-build.noindex/module-cache-clang\lldb-api --executable C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./bin/lldb.exe --compiler C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./bin/clang.exe --dsymutil C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./bin/dsymutil.exe --llvm-tools-dir C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./bin --lldb-obj-root C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/tools/lldb --lldb-libs-dir C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/./lib --skip-category=watchpoint C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\test\API\tools\lldb-dap\instruction-breakpoint -p TestDAP_instruction_breakpoint.py
--
Exit Code: 1

Command Output (stdout):
--
lldb version 20.0.0git (https://github.com/llvm/llvm-project.git revision 89c27d6b07dd03a71e5692caa4e20ab14f948921)
  clang revision 89c27d6b07dd03a71e5692caa4e20ab14f948921
  llvm revision 89c27d6b07dd03a71e5692caa4e20ab14f948921
Skipping the following test categories: ['watchpoint', 'libc++', 'libstdcxx', 'dwo', 'dsym', 'gmodules', 'debugserver', 'objc', 'fork', 'pexpect']


--
Command Output (stderr):
--
FAIL: LLDB (C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\bin\clang.exe-aarch64) :: test_instruction_breakpoint (TestDAP_instruction_breakpoint.TestDAP_InstructionBreakpointTestCase.test_instruction_breakpoint)

======================================================================

ERROR: test_instruction_breakpoint (TestDAP_instruction_breakpoint.TestDAP_InstructionBreakpointTestCase.test_instruction_breakpoint)

----------------------------------------------------------------------

Error when building test subject.



Build Command:

make 'VPATH=C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\test\API\tools\lldb-dap\instruction-breakpoint' -C 'C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\lldb-test-build.noindex\tools\lldb-dap\instruction-breakpoint\TestDAP_instruction_breakpoint.test_instruction_breakpoint' -I 'C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\test\API\tools\lldb-dap\instruction-breakpoint' -I 'C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\packages\Python\lldbsuite\test\make' -f 'C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\test\API\tools\lldb-dap\instruction-breakpoint\Makefile' all ARCH=aarch64 'CC="C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\bin\clang.exe"' 'CLANG_MODULE_CACHE_DIR=C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/lldb-test-build.noindex/module-cache-clang\lldb-api' LLDB_OBJ_ROOT=C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/tools/lldb OS=Windows_NT HOST_OS=Windows_NT



Build Command Output:

make: Entering directory 'C:/Users/tcwg/llvm-worker/lldb-aarch64-windows/build/lldb-test-build.noindex/tools/lldb-dap/instruction-breakpoint/TestDAP_instruction_breakpoint.test_instruction_breakpoint'

@DavidSpickett
Copy link
Collaborator

DavidSpickett commented Aug 27, 2024

The rest of the lldb-dap tests are disabled on Windows, this one should be too.

One of these days I'll figure out what's wrong with them all.

@santhoshe447
Copy link
Contributor Author

The rest of the lldb-dap tests are disabled on Windows, this one should be too.

One of these days I'll figure out what's wrong with them all.

Thanks,
I have submitted the PR for this.
#106200

5c4lar pushed a commit to 5c4lar/llvm-project that referenced this pull request Aug 29, 2024
…#105278)

Added support for "supportsInstructionBreakpoints" capability and now it
this command is triggered when we set instruction breakpoint.
We need this support as part of enabling disassembly view debugging.
Following features should work as part of this feature enablement:

1. Settings breakpoints in disassembly view: Unsetting the breakpoint is
not happening from the disassembly view. Currently we need to unset
breakpoint manually from the breakpoint List. Multiple breakpoints are
getting set for the same $

2. Step over, step into, continue in the disassembly view

The format for DisassembleRequest and DisassembleResponse at
https://raw.githubusercontent.com/microsoft/vscode/master/src/vs/workbench/contrib/debug/common/debugProtocol.d.ts
.

Ref Images:
Set instruction breakpoint in disassembly view:

![image](https://github.com/user-attachments/assets/833bfb34-86f4-40e2-8c20-14b638a612a2)

After issuing continue:

![image](https://github.com/user-attachments/assets/884572a3-915e-422b-b8dd-d132e5c00de6)

---------

Co-authored-by: Santhosh Kumar Ellendula <[email protected]>
Co-authored-by: Santhosh Kumar Ellendula <[email protected]>
dmpolukhin pushed a commit to dmpolukhin/llvm-project that referenced this pull request Sep 2, 2024
…#105278)

Added support for "supportsInstructionBreakpoints" capability and now it
this command is triggered when we set instruction breakpoint.
We need this support as part of enabling disassembly view debugging.
Following features should work as part of this feature enablement:

1. Settings breakpoints in disassembly view: Unsetting the breakpoint is
not happening from the disassembly view. Currently we need to unset
breakpoint manually from the breakpoint List. Multiple breakpoints are
getting set for the same $

2. Step over, step into, continue in the disassembly view

The format for DisassembleRequest and DisassembleResponse at
https://raw.githubusercontent.com/microsoft/vscode/master/src/vs/workbench/contrib/debug/common/debugProtocol.d.ts
.

Ref Images:
Set instruction breakpoint in disassembly view:

![image](https://github.com/user-attachments/assets/833bfb34-86f4-40e2-8c20-14b638a612a2)

After issuing continue:

![image](https://github.com/user-attachments/assets/884572a3-915e-422b-b8dd-d132e5c00de6)

---------

Co-authored-by: Santhosh Kumar Ellendula <[email protected]>
Co-authored-by: Santhosh Kumar Ellendula <[email protected]>
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.

7 participants