Skip to content
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

Support GoToTargets Request #1102

Merged
merged 12 commits into from
Jan 29, 2021
Merged

Support GoToTargets Request #1102

merged 12 commits into from
Jan 29, 2021

Conversation

WardenGnaw
Copy link
Member

Thank you @Trass3r for the initial work.

This PR will add in the GoToTargets request which can be used by the client to get instructionPointerReference from a file and line.

#1042 will focus on implement GoToRequest (Jump to Line)

{
return Constants.S_FALSE;
}
string result = frame.EvaluateExpression("$pc=" + EngineUtils.AsAddr(addr, _engine.DebuggedProcess.Is64BitArch));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is now only used by VS, right?
It can handle this expression?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is now only used by VS, right?

Correct. Currently this code isn't reachable in DAP scenarios.

It can handle this expression?

This is just an internal call within MIEngine, so it will not matter what IDE is used. It worked with whatever scenario Set Next Statement was originally tested (Android?). I don't know if it has been tested more broadly. Certainly using the actual GDB command is better. But this PR just seeks to finish off disassembly support. Actually implementing set next statement is still left for your PR. Someone needs to finish off handling the work of handling that stopping event...

Copy link
Contributor

@Trass3r Trass3r Jan 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah my bad, of course this goes down to gdb and not up.
Setting $pc is actually different from the jump command in that it does not start your program running; it only changes the address of where it will run when you continue: https://sourceware.org/gdb/current/onlinedocs/gdb/Jumping.html
So I'm not sure how this worked out regarding UI/UX.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, we would need to hide this difference -- the new set next statement would need to save some state in the process object to indicate that it is waiting for that stop event, and then SetNextStatement will need to wait for the stop event to be raised (or for the target process to exit).

@gregg-miskelly
Copy link
Member

                    response.StackFrames.Add(new ProtocolMessages.StackFrame()

nit: While you are here, do you want to add the instruction reference here as well. That should finish off the feature.


Refers to: src/OpenDebugAD7/AD7DebugSession.cs:1565 in f37fe48. [](commit_id = f37fe48, deletion_comment = False)

Copy link
Member

@gregg-miskelly gregg-miskelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

@WardenGnaw
Copy link
Member Author

                    response.StackFrames.Add(new ProtocolMessages.StackFrame()

nit: While you are here, do you want to add the instruction reference here as well. That should finish off the feature.

Refers to: src/OpenDebugAD7/AD7DebugSession.cs:1565 in f37fe48. [](commit_id = f37fe48, deletion_comment = False)

This will be handled in another PR.

@WardenGnaw WardenGnaw merged commit fbbc993 into main Jan 29, 2021
@WardenGnaw WardenGnaw deleted the dev/waan/SupportGoToTargets branch February 17, 2021 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants