This is an archive of the discontinued LLVM Phabricator instance.

[lldb-vscode] Correctly return source mapped breakpoints for setBreakpoints request
ClosedPublic

Authored by wallace on Mar 27 2020, 7:46 PM.

Details

Summary

When using source maps for a breakpoint, in order to find the actual source that breakpoint has resolved, we
need to use a query similar to what CommandObjectSource::DumpLinesInSymbolContexts does, which is the logic
used by the CLI to display the source line at a given breakpoint. That's necessary because from a breakpoint
location you only have an address, which points to the original source location, not the source mapped one.

in the setBreakpoints request handler, we haven't been doing such query and we were returning the original
source location, which was breaking the UX of VSCode, as many breakpoints were being set but not displayed
in the source file next to each line. Besides, clicking the source path of a breakpoint in the breakpoints
view in the debug tab was not working for this case, as VSCode was trying to open a non-existent file, thus
showing an error to the user.

Ideally, we should do the query mentioned above to find the actual source location to respond to the IDE,
however, that query is expensive and users can have an arbitrary number of breakpoints set. As a simpler fix,
the request sent by VSCode already contains the full source path, which exists because the user set it from
the IDE itself, so we can simply reuse it instead of querying from the SB API.

I wrote a test for this, and found out that I had to move SetSourceMapFromArguments after RunInitCommands in
lldb-vscode.cpp, because an init command used in all tests is settings clear -all, which would clear the
source map unless specified after initCommands. And in any case, I think it makes sense to have initCommands
run before anything the debugger would do.

Diff Detail

Event Timeline

wallace created this revision.Mar 27 2020, 7:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 7:46 PM
labath added inline comments.Mar 30 2020, 2:06 AM
lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
25–43

Please make sure none of these commands modify the source tree. You can look at test/API/source-manager/Makefile for an example of how to build a binary to reference files in the build tree.

lldb/tools/lldb-vscode/JSONUtils.cpp
324

StringRefs are normally passed by value.

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

What about breakpoints in dynamically loaded shared libraries? Should you be remembering the original path somewhere so that one you can set it here too?

wallace marked 2 inline comments as done.Mar 30 2020, 10:42 AM
wallace added inline comments.
lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
25–43

thanks, will do

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

This case is different, as function breakpoints are set by the IDE without referring to any source path, as they are equivalent to making 'b function_name'. Probably function breakpoints are not working correctly, so I'll have to fix it any of these days using the expensive query I mentioned above.

wallace updated this revision to Diff 253649.Mar 30 2020, 11:43 AM

address comments

clayborg requested changes to this revision.Mar 30 2020, 1:35 PM

The description confused me a bit as I thought we were going to be doing some "CommandObjectSource::DumpLinesInSymbolContexts()" stuff somewhere. But this path is really just "return the same source path from the setBreakpoints" request in the response and I am all for that. So a few nits and this will be good to go. See my inline comments.

lldb/tools/lldb-vscode/JSONUtils.cpp
335–336

There might be a cheaper llvm::sys::path operation that can extract the basename from a StringRef instead of creating a SBFileSpec?

lldb/tools/lldb-vscode/JSONUtils.h
207

Maybe reword a bit to make sure people understand this is the sourcePath that was pass in the setBreakpoint request?

The path that was specified in the setBreakpoint request.
lldb/tools/lldb-vscode/lldb-vscode.cpp
1375

remove white only change

1377

remove white only change

This revision now requires changes to proceed.Mar 30 2020, 1:35 PM
wallace updated this revision to Diff 253695.Mar 30 2020, 1:55 PM

address comments

clayborg requested changes to this revision.Mar 30 2020, 3:29 PM

After reviewing more, we should just re-use CreateBreakpoint and add a "llvm::Optional<StringRef> request_path" argument. Then all breakpoints use the same function and we avoid duplicated code. Inline comments should detail what should be done. Let me know if you have questions.

lldb/tools/lldb-vscode/JSONUtils.cpp
285

Read my comments below first, then read this:

we should add an extra llvm::Optional<StringRef> arg to this so we can have all code just use this function:

llvm::json::Value CreateBreakpoint(lldb::SBBreakpoint &bp, llvm::Optional<StringRef> request_path)

If "request_path" has a value, then we use this in the "source" response. If it doesn't we look it up in from the location address.

318

we will need to add "llvm::Optional<StringRef> request_path" as an argument to CreateSource here, so it can use request_path if it has a value, else use the source file in the line_entry object.

323

Actually the _only_ difference between this function and "llvm::json::Value CreateBreakpoint(lldb::SBBreakpoint &bp)" is the fact that we pass along the path along. So maybe we can just add a new variable to CreateBreakpoint above so. See my inline comment on CreateBreakpoint().

332

This should do what it was doing before: grab the source line only from the address. Why? If you set a breakpoint on a line that has no code like:

1 int main(int argc, ...)
2 {                              /// <-- set bp here
3   int foo = arg * 10; /// <-- bp will end up here

So we should still be looking up the line number from the Address of the first resolved location so that the source line does get adjusted in the IDE.

496

add llvm::Optional<StringRef> request_path argument for CreateBreakpoint

This revision now requires changes to proceed.Mar 30 2020, 3:29 PM
wallace marked an inline comment as done.Mar 30 2020, 9:02 PM
wallace added inline comments.
lldb/tools/lldb-vscode/JSONUtils.cpp
332

TIL

wallace updated this revision to Diff 253779.Mar 30 2020, 9:37 PM

address comments

Looks good as long as with we have "llvm::Optional<llvm::StringRef> request_path = {}" in the arguments for a function, that "{}" will turn into llvm::None and not an empty StringRef? Unclear to me. As long as this is what is happening and as long is this coding convention is used elsewhere in llvm (using "{}" instead of "llvm::None") I am ok. Pavel?

lldb/tools/lldb-vscode/JSONUtils.h
204

s/{}/llvm::None/ if {} doesn't construct to llvm::None?

230

s/{}/llvm::None/ if {} doesn't construct to llvm::None

Looks good as long as with we have "llvm::Optional<llvm::StringRef> request_path = {}" in the arguments for a function, that "{}" will turn into llvm::None and not an empty StringRef? Unclear to me. As long as this is what is happening and as long is this coding convention is used elsewhere in llvm (using "{}" instead of "llvm::None") I am ok. Pavel?

The two expressions are equivalent here. We're currently using both styles in lldb. I have a slight preference for the explicit None version, but I don't think it's worth the trouble to standardize on one or the other.

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

The comment was supposed to be here.

1974

Ah, sorry. I misplaced this comment. It was meant to go above to line 417, where the eBreakpointEventTypeLocationsAdded event is handled. That code should be reached for "regular" file+line breakpoints in case a breakpoint gets new locations (e.g.) in response to a shared library being loaded.

wallace updated this revision to Diff 254039.Mar 31 2020, 4:26 PM

Account for dynamically loaded libraries and update the logic for breakpoint events

Some included changes:

  • Removed the "source" element if the "breakpoint" object returned in breakpoint events
  • CreateBreakpoint now accepts an optional line number, which is used as fallback if no location is valid
  • Added a test that asserts breakpoints in dynamically loaded libraries

I also tested the entire flow in VSCode and it was all fine

clayborg accepted this revision.Mar 31 2020, 7:07 PM

Just use "llvm::None" instead of "{}" and this is good to go.

lldb/tools/lldb-vscode/JSONUtils.h
230

lets use "llvm::None" here instead of "{}" for clarity

231

ditto

This revision is now accepted and ready to land.Mar 31 2020, 7:07 PM
labath requested changes to this revision.Apr 1 2020, 1:34 AM

Thanks for the updates and for writing the test. The code looks fine to me, the "request changes" is for making sure the test follows best practices and is portable.

lldb/test/API/tools/lldb-vscode/breakpoint/Makefile
10–17 ↗(On Diff #254041)

This is not the right way to build binaries -- it will fail on macos for instance.
If we were not dlopening, you should set CXX_SOURCES, and DYLIB_C_SOURCES and let the normal rules do build for you, but for dlopen you need to do something a tad more complicated and invoke a submake with the right arguments you can look at functionalities/load_unload/Makefile for an example of how to do that.

I don't think we currently have a test which uses both shared libraries, and relocated sources files, so it's possible that this may fail for some reason. But if that's the case, then I want to understand what's the problem first (so we can try to fix it).

lldb/test/API/tools/lldb-vscode/breakpoint/TestVSCode_setBreakpoints.py
32–33

If I follow this correctly this will escape from the build folder for this test (two dirnames). Can you stay within the confines of the build folder?

Among other things, then you won't need to rmtree these things as the build folder is automatically cleaned before each test.

203–204

maybe move these into the setUp function?

lldb/test/API/tools/lldb-vscode/breakpoint/main.cpp
19 ↗(On Diff #254041)

Looking at the other dlopen tests (e.g., functionalities/load_unload/main.cpp), it looks like you'll need to ifdef the correct library name here.

This revision now requires changes to proceed.Apr 1 2020, 1:34 AM
wallace updated this revision to Diff 254280.Apr 1 2020, 12:57 PM
wallace marked 2 inline comments as done.

address comments

Ping ping. This will fix some existing issues reported by users :)

labath accepted this revision.Apr 7 2020, 11:11 PM

Looks good now. Sorry about the delay, I have a lot of things in my queue now and this got lost at the bottom of the stack.

This revision is now accepted and ready to land.Apr 7 2020, 11:11 PM
This revision was automatically updated to reflect the committed changes.