Stop emitting a breakpoint for each location in a breakpoint when responding to breakpoint commands.

[clayborg]

Summary

The VS Code DAP expects on response for each breakpoint that was requested. If we responsd with multiple entries for one breakpoint the VS Code UI gets out of date. Currently the VS code DAP doesn't handle one breakpoint with multiple locations. If this ever gets fixed we can modify our code.

[labath]

This is not ideal, but if that's what vscode wants, then I guess we just have to do it.

However, are you sure that this is all that needs to be done here. It seems like also the handling of eBreakpointEventTypeLocationsAdded/Removed events in EventThreadFunction should be changed too. Otherwise, I think the whole breakpoint will disappear whenever a single location goes away...

[clayborg]

In D73665#1848755, @labath wrote:

This is not ideal, but if that's what vscode wants, then I guess we just have to do it.

However, are you sure that this is all that needs to be done here. It seems like also the handling of eBreakpointEventTypeLocationsAdded/Removed events in EventThreadFunction should be changed too. Otherwise, I think the whole breakpoint will disappear whenever a single location goes away...

Yes this needs to be fixed as well. I will switch this to pass in the breakpoint itself to avoid places iterating over the locations...

[clayborg]

Handle breakpoint events in the same way.

[labath]

Sorry about the delay. I think this is better, but the handling of`eBreakpointEventTypeLocationsAdded/Removed` it still does not seem completely right. Though I don't really understand how VSCode works, if my understanding that we are now creating a "vscode" breakpoint for each "lldb breakpoint" (instead of breakpoint _location_) is correct, then it seems that we should treat the "vscode breakpoint" enabled as long as the lldb breakpoint has at least one location.

However, that's not what the code does now, I think. Whenever a single location disappears (eBreakpointEventTypeLocationsRemoved), e.g. in response to a shared library unloading, we will set the reason=removed packet, even if other locations remain. I think that we should send the "removed" message only when the location count goes down to zero (and send the "new" packet whenever it goes non-zero).

Am I misunderstanding something?

[clayborg]

Fixed Pavel's issues. I thoroughly tested this and found that we should be only sending "changed" events for breakpoints since breakpoints never go away.

Changed in this patch:

• add a "vscode" name to each breakpoint so we know which breakpoints were set in the IDE UI. Any breakpoints that don't have this won't get updates to the IDE since the IDE doesn't know about them. This is in case people set breakpoints in the debugger console. I tried reporting such breakpoints as "new" breakpoints, but the IDE UI doesn't do anything with new breakpoint that weren't created in the UI.
• Fixed logic to always report "changed" as the breakpoint event reason. LLDB breakpoints never go away, so they are never "new" or "removed".
• If a breakpoint has no resolved locations, then the breakpoint is marked as "verified": false in the breakpoint responses and events. When a breakpoint resolves then it will get updated.

[clayborg]

Friendly ping.

[labath]

I'm sorry about the delay, I'm quite busy these days.

This code looks good to me now, but it's lacking test coverage. Can you add some test where new breakpoint locations get added or removed and check that the lldb-vscode generates the appropriate kind of dap message. And maybe another where a breakpoint is set via the the command line and check that we _don't_ emit any events...

[clayborg]

Yes, I will add tests now that the code has been finalized! Thanks for your time.

[clayborg]

Added a test to cover breakpoint events. The test has an executable and a shared library and we set breakpoints in the main executable and in the shared library. When we run, we stop at the entry point to the program and then the breakpoints are asked to be set. The breakpoint in the main executable should be resolved, and the one in the shared library should not be resolved since the shared library hasn't been loaded yet. We also set a breakpoint via the "preRunCommands" in the "launch" packet. This ensures we set a breakpoint using a LLDB command for which we expect to not get any breakpoint events for since the breakpoint wasn't set via VS Code DAP packets. After we stop, at the entry point we verify no breakpoint events have been received, and then we run to one of the breakpoints that was set via the VS Code DAP packets and check that we received only one event for the breakpoint set via DAP packets and not for the LLDB command created breakpoint.

Fixed lldbvscode_testcase.py set_source_breakpoints and set_function_breakpoints to return breakpoint IDs and not breakpoint ID + location ID. Also fixed verify_breakpoint_hit so that it actally checks that breakpoints were being set by now using an assertTrue call. Prior to this, this function would return true or false, but no one was looking at the result. Looking around the code it seems this should assert and cause the test to fail.

Modified vscode.py to store any breakpoint events in the object so tests can access these events in the test python code for verification.

Modified the "description" field of "stopped" events when we stop at breakpoints to contain "breakpoint %u" where %u is the breakpoint ID. This allows lldbvscode_testcase.verify_breakpoint_hit() to actually work.

[labath]

This looks good now. Thanks for your patience.

lldb/test/API/tools/lldb-vscode/breakpoint-events/TestVSCode_breakpointEvents.py

68 ↗

(On Diff #244275)

Under what conditions can response be None? Should this maybe be assertIsNotNone(response) instead ?

[clayborg]

Disabled the new breakpoint event test unless being run on Darwin to keep the buidbots happy until I can figure out what is going wrong:

commit c84a0bd9adb3ad699cd6bd4bf865d8b1ea76f2b0 (HEAD -> master, origin/master, origin/HEAD)
Author: Greg Clayton <[e-mail]>
Date: Thu Feb 13 09:32:19 2020 -0800

Fix buildbots by disabling this new test until I can fix it.

This tests works on Darwin. I will need to check windows and linux.

lldb/tools/lldb-vscode/JSONUtils.h

 /// \param[in] breakpoints
 ///     A JSON array that will get a llvm::json::Value for \a bp
 ///     appended to it.
 void AppendBreakpoint(lldb::SBBreakpoint &bp, llvm::json::Array &breakpoints);
 
 /// Converts breakpoint location to a Visual Studio Code "Breakpoint"
 /// JSON object and appends it to the \a breakpoints array.
 ///
-/// \param[in] bp_loc
-///     A LLDB breakpoint location object to convert into a JSON value
+/// \param[in] bp
+///     A LLDB breakpoint object to convert into a JSON value
 ///
 /// \return
 ///     A "Breakpoint" JSON object with that follows the formal JSON
 ///     definition outlined by Microsoft.
-llvm::json::Value CreateBreakpoint(lldb::SBBreakpointLocation &bp_loc);
+llvm::json::Value CreateBreakpoint(lldb::SBBreakpoint &bp);
 
 /// Create a "Event" JSON object using \a event_name as the event name
 ///
 /// \param[in] event_name
 ///     The string value to use for the "event" key in the JSON object.
 ///
 /// \return
 ///     A "Event" JSON object with that follows the formal JSON

lldb/tools/lldb-vscode/JSONUtils.cpp

 //       "type": "integer",
 //       "description": "An optional 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."
 //     }
 //   },
 //   "required": [ "verified" ]
 // }
-llvm::json::Value CreateBreakpoint(lldb::SBBreakpointLocation &bp_loc) {
+llvm::json::Value CreateBreakpoint(lldb::SBBreakpoint &bp) {
   // Each breakpoint location is treated as a separate breakpoint for VS code.
   // They don't have the notion of a single breakpoint with multiple locations.
   llvm::json::Object object;
-  if (!bp_loc.IsValid())
+  if (!bp.IsValid())
     return llvm::json::Value(std::move(object));
 
   object.try_emplace("verified", true);
-  const auto vs_id = MakeVSCodeBreakpointID(bp_loc);
+  const auto vs_id = bp.GetID();
   object.try_emplace("id", vs_id);
+  // VS Code DAP doesn't currently allow one breakpoint to have multiple
+  // locations so we just report the first one. If we report all locations
+  // then the IDE starts showing the wrong line numbers and locations for
+  // other source file and line breakpoints in the same file.
+  auto bp_loc = bp.GetLocationAtIndex(0);
   auto bp_addr = bp_loc.GetAddress();
   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);
     object.try_emplace("source", CreateSource(line_entry));
   }
   return llvm::json::Value(std::move(object));
 }
 
 void AppendBreakpoint(lldb::SBBreakpoint &bp, llvm::json::Array &breakpoints) {
-  if (!bp.IsValid())
-    return;
-  const auto num_locations = bp.GetNumLocations();
-  if (num_locations == 0)
-    return;
-  for (size_t i = 0; i < num_locations; ++i) {
-    auto bp_loc = bp.GetLocationAtIndex(i);
-    breakpoints.emplace_back(CreateBreakpoint(bp_loc));
-  }
+  breakpoints.emplace_back(CreateBreakpoint(bp));
 }
 
 // "Event": {
 //   "allOf": [ { "$ref": "#/definitions/ProtocolMessage" }, {
 //     "type": "object",
 //     "description": "Server-initiated event.",
 //     "properties": {
 //       "type": {
llvm::json::Value CreateVariable(lldb::SBValue v, int64_t variablesReference,
   v.GetExpressionPath(evaluateStream);
   const char *evaluateName = evaluateStream.GetData();
   if (evaluateName && evaluateName[0])
     EmplaceSafeString(object, "evaluateName", std::string(evaluateName));
   return llvm::json::Value(std::move(object));
 }
 
 } // namespace lldb_vscode
-

lldb/tools/lldb-vscode/LLDBUtils.h

 ///
 /// \param[in] dap_frame_id
 ///     The VSCode frame ID to convert to a frame ID.
 ///
 /// \return
 ///     The LLDB frame index ID.
 uint32_t GetLLDBFrameID(uint64_t dap_frame_id);
 
-/// Given a LLDB breakpoint, make a breakpoint ID that is unique to a
-/// specific breakpoint and breakpoint location.
-///
-/// VSCode requires a Breakpoint "id" to be unique, so we use the
-/// breakpoint ID in the lower BREAKPOINT_ID_SHIFT bits and the
-/// breakpoint location ID in the upper BREAKPOINT_ID_SHIFT bits.
-///
-/// \param[in] bp_loc
-///     The LLDB break point location.
-///
-/// \return
-///     A unique integer that allows us to easily find the right
-///     stack frame within a thread on subsequent VS code requests.
-int64_t MakeVSCodeBreakpointID(lldb::SBBreakpointLocation &bp_loc);
-
-/// Given a VSCode breakpoint ID, convert to a LLDB breakpoint ID.
-///
-/// VSCode requires a Breakpoint "id" to be unique, so we use the
-/// breakpoint ID in the lower BREAKPOINT_ID_SHIFT bits and the
-/// breakpoint location ID in the upper BREAKPOINT_ID_SHIFT bits.
-///
-/// \param[in] dap_breakpoint_id
-///     The VSCode breakpoint ID to convert to an LLDB breakpoint ID.
-///
-/// \return
-///     The LLDB breakpoint ID.
-uint32_t GetLLDBBreakpointID(uint64_t dap_breakpoint_id);
-
-/// Given a VSCode breakpoint ID, convert to a LLDB breakpoint location ID.
-///
-/// VSCode requires a Breakpoint "id" to be unique, so we use the
-/// breakpoint ID in the lower BREAKPOINT_ID_SHIFT bits and the
-/// breakpoint location ID in the upper BREAKPOINT_ID_SHIFT bits.
-///
-/// \param[in] dap_breakpoint_id
-///     The VSCode frame ID to convert to a breakpoint location ID.
-///
-/// \return
-///     The LLDB breakpoint location ID.
-uint32_t GetLLDBBreakpointLocationID(uint64_t dap_breakpoint_id);
 } // namespace lldb_vscode
 
 #endif

lldb/tools/lldb-vscode/LLDBUtils.cpp

uint32_t GetLLDBFrameID(uint64_t dap_frame_id) {
   return dap_frame_id & ((1u << THREAD_INDEX_SHIFT) - 1);
 }
 
 int64_t MakeVSCodeFrameID(lldb::SBFrame &frame) {
   return (int64_t)(frame.GetThread().GetIndexID() << THREAD_INDEX_SHIFT |
                    frame.GetFrameID());
 }
 
-static uint32_t constexpr BREAKPOINT_ID_SHIFT = 22;
-
-uint32_t GetLLDBBreakpointID(uint64_t dap_breakpoint_id) {
-  return dap_breakpoint_id >> BREAKPOINT_ID_SHIFT;
-}
-
-uint32_t GetLLDBBreakpointLocationID(uint64_t dap_breakpoint_id) {
-  return dap_breakpoint_id & ((1u << BREAKPOINT_ID_SHIFT) - 1);
-}
-
-int64_t MakeVSCodeBreakpointID(lldb::SBBreakpointLocation &bp_loc) {
-  return (int64_t)(bp_loc.GetBreakpoint().GetID() << BREAKPOINT_ID_SHIFT |
-                   bp_loc.GetID());
-}
-
 } // namespace lldb_vscode

lldb/tools/lldb-vscode/lldb-vscode.cpp

if (listener.WaitForEvent(1, event)) {
         } else if ((event_mask & lldb::SBProcess::eBroadcastBitSTDOUT) ||
                    (event_mask & lldb::SBProcess::eBroadcastBitSTDERR)) {
           SendStdOutStdErr(process);
         }
       } else if (lldb::SBBreakpoint::EventIsBreakpointEvent(event)) {
         if (event_mask & lldb::SBTarget::eBroadcastBitBreakpointChanged) {
           auto event_type =
               lldb::SBBreakpoint::GetBreakpointEventTypeFromEvent(event);
-          const auto num_locs =
-              lldb::SBBreakpoint::GetNumBreakpointLocationsFromEvent(event);
           auto bp = lldb::SBBreakpoint::GetBreakpointFromEvent(event);
           bool added = event_type & lldb::eBreakpointEventTypeLocationsAdded;
           bool removed =
               event_type & lldb::eBreakpointEventTypeLocationsRemoved;
           if (added || removed) {
-            for (size_t i = 0; i < num_locs; ++i) {
-              auto bp_loc =
-                  lldb::SBBreakpoint::GetBreakpointLocationAtIndexFromEvent(
-                      event, i);
-              auto bp_event = CreateEventObject("breakpoint");
-              llvm::json::Object body;
-              body.try_emplace("breakpoint", CreateBreakpoint(bp_loc));
-              if (added)
-                body.try_emplace("reason", "new");
-              else
-                body.try_emplace("reason", "removed");
-              bp_event.try_emplace("body", std::move(body));
-              g_vsc.SendJSON(llvm::json::Value(std::move(bp_event)));
-            }
-          }
-        }
+            body.try_emplace("breakpoint", CreateBreakpoint(bp));
       } else if (event.BroadcasterMatchesRef(g_vsc.broadcaster)) {
         if (event_mask & eBroadcastBitStopEventThread) {
           done = true;
         }
       }
     }
   }
 }