Page MenuHomePhabricator

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

Authored by clayborg on Wed, Jan 29, 2:13 PM.

Details

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.

Diff Detail

Event Timeline

clayborg created this revision.Wed, Jan 29, 2:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jan 29, 2:13 PM

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 added a comment.EditedThu, Jan 30, 10:19 AM

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 updated this revision to Diff 241543.Thu, Jan 30, 12:18 PM

Handle breakpoint events in the same way.

labath added a comment.Wed, Feb 5, 8:59 AM

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 updated this revision to Diff 243385.Sat, Feb 8, 10:14 AM

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.

Friendly ping.

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...

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

clayborg updated this revision to Diff 244275.Wed, Feb 12, 1:59 PM

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 accepted this revision.Thu, Feb 13, 12:46 AM

This looks good now. Thanks for your patience.

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

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

This revision is now accepted and ready to land.Thu, Feb 13, 12:46 AM
This revision was automatically updated to reflect the committed changes.

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 <gclayton@fb.com>
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.