-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[lldb-dap] persistent assembly breakpoints #148061
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
Open
eronnen
wants to merge
19
commits into
llvm:main
Choose a base branch
from
eronnen:lldb-dap-persistent-assembly-breakpoints-file-addr
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+461
−106
Open
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
c9fc191
[lldb-dap] Support persistent assembly breakpoints
eronnen f48f313
save adapterData for assembly breakpoints
eronnen 06170ea
remove offset
eronnen 26b7000
create persistent breakpoints
eronnen 5ea62b2
add instructions_offset to breakppoint resolver
eronnen 189c308
format
eronnen 543bc2a
instruction offset
eronnen 19127b9
format
eronnen 61c3469
add persistence data to response
eronnen a493c4c
fix
eronnen 6aaae35
fix
eronnen 66510aa
fix address resolve of a module that is not loaded yet
eronnen f528713
fix check
eronnen 0019d2c
conventions
eronnen 57459f9
add persistence assembly breakpoint test
eronnen 70d369e
fix
eronnen 4b3412e
fix
eronnen b0e3865
python format
eronnen 9c90520
remove raw json usage
eronnen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to create a new API for this?
If we're storing the file path + offset, we should be able to create the breakpoint by checking if the module is loaded with the same path and then looking up the address for the file offset.
Something along the lines of:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According the specification the only persistent data we can add is to the
Source
object in theadapterData
field (https://microsoft.github.io/debug-adapter-protocol//specification.html#Types_Source).The source object is common for all breakpoints in the same function, so the only way I found to differentiate between two breakpoints in the same
Source
is theline
field in theBreakpoint
object (where we can't add custom persistent data), which is equivalent to theinstructions_offset
from the start of the function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add new APIs for lldb-dap, but they should make sense on their own. Right now, what kind of idea what kind of breakpoint this would set. Is the FileSpec the name of the module you're setting the breakpoint in? And does file_addr identify a file in that module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@labath Yes, the intention is to set a breakpoint in a virtual address that relates to the file address in a given module.
A function like this already exists in
main
inTarget.h
(CreateAddressInModuleBreakpoint) although not exported in the public API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The breakpoint resolvers got a "user provided"
offset
field because it was useful for some breakpoint types to allow the user to specifysymbol + offset
orfile & line address + offset
. That was handy - especially symbol + offset - in the case where the user has no easy way to go from the initial specifier to an address when they are making the breakpoint, and so that's not math they can do up front.But that wasn't used for Address breakpoints (and thus NOT passed in the original CreateAddressInModuleBreakpoint) because there's no point. You are starting with an lldb::addr_t, so you can immediately do whatever offsetting math you need to do before setting the breakpoint. There's no reason not to just add the offset to the file_addr you are passing in to create the breakpoint.
You are also adding to that another form of offset - the "instruction count offset" which is the size of a certain number of instructions after the initial addr (*). That's slightly more useful than the offset for address breakpoints, in that that computation is less trivial than adding two addresses.
But first off, if this IS useful as another way to specify "offset from some resolved location" it is useful for the other cases where we actually use offsets. So doing it only for breakpoints set by address doesn't make sense.
Secondly, It should be another variant of
offsets
. I don't think we anywhere need to offer an API that takes both an lldb::addr_t offset AND an "instruction count" offset. That seems unnecessary, and error prone (do you offset with the addr_t THEN count instructions or vice versa).(*) You need to check that that addr disassembles, BTW. This is user input, so there's no guarantee that address lies on an instruction boundary...