[Lldb-commits] [PATCH] D76968: [lldb-vscode] Correctly return source mapped breakpoints for setBreakpoints request

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:285
 // }
 llvm::json::Value CreateBreakpoint(lldb::SBBreakpoint &bp) {
   // Each breakpoint location is treated as a separate breakpoint for VS code.
----------------
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.


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:318
       object.try_emplace("line", line);
     object.try_emplace("source", CreateSource(line_entry));
   }
----------------
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.


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:323
 
+llvm::json::Value CreateSourceBreakpoint(lldb::SBBreakpoint &bp,
+                                         llvm::StringRef sourcePath, int line) {
----------------
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().


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:332
+
+  object.try_emplace("line", line);
+
----------------
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.


================
Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:493
 // }
 llvm::json::Value CreateSource(lldb::SBLineEntry &line_entry) {
   llvm::json::Object object;
----------------
add llvm::Optional<StringRef> request_path argument for CreateBreakpoint


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76968/new/

https://reviews.llvm.org/D76968