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

gdbstub: Fix some bugs in IsMemoryBreak() and ServeBreak. Add workaround to let watchpoints break into GDB. #4651

Merged
merged 5 commits into from Mar 8, 2019

Conversation

DimitriPilot3
Copy link
Contributor

@DimitriPilot3 DimitriPilot3 commented Feb 17, 2019

  1. GDBStub::IsMemoryBreak had a simple bug: even while connected to a GDB client, it still returned false unexpectedly because the test for IsConnected was flipped.

  2. ARMul_State::ServeBreak had a conditional statement which consisted of setting the PC register (r15) to the location of a breakpoint hit. Though it relieves the necessity for Dynarmic to sync up PC prior to a call to ServeBreak, Dynarmic was in fact doing it already, so the statement is redundant!
    The flag involved (last_bkpt_hit) is set by a call to RecordBreak which, oddly enough, could also be called (from InterpreterMainLoop) for any memory watchpoint hit. Even assuming there exists a data record of such a hit, (sadly, there isn't, because CheckMemoryBreak doesn't save this information before returning ), this would cause ServeBreak to trash the PC register of a stopped thread's context, making the CPU dispatch there once it's told by GDB to continue executing! Oh no!

  3. Lastly, a few boolean data members (including last_bkpt_hit mentioned above) were left uninitialized, so ServeBreak could fire off once unexpectedly after the emulation core starts, which triggers the 2nd bug above, causing a thread to dispatch to NULL.


This PR fixes 1 and 3 (straightforward) as well as 2 by replacing the redundant code with an ASSERT.
It also adds a workaround on top of 2, which checks for last_bkpt_hit or IsMemoryBreak separately, allowing watchpoint hits to be safely passed on to GDB like any other breakpoint hit!

Of course, it would be super useful to let the GDBStub::SendSignal function know which memory address was hit, and pass it on to GDB as extended stop information (watch=\<hex string\>) so it can work out that a watchpoint was hit, and if so, which one. Sending this information to GDB is optional, so it's fine to leave the watchpoint feature as-is. I'm working on a separate PR to enhance it.


This change is Reviewable

As a result, the only existing codepath for a memory watchpoint hit to break into GDB (InterpeterMainLoop, GDB_BP_CHECK, ARMul_State::RecordBreak) is finally taken,
which exposes incorrect logic* in both RecordBreak and ServeBreak.

* a blank BreakpointAddress structure is passed, which sets r15 (PC) to NULL
For now, instead check for GDBStub::IsMemoryBreak() in InterpreterMainLoop and ServeBreak.

Fixes PC being set to a stale/unhit breakpoint address (often zero) when a memory watchpoint (rwatch, watch, awatch) is handled in ServeBreak() and generates a GDB trap.

Reasons for removing a call to RecordBreak() for memory watchpoints:
* The``breakpoint_data`` we pass is typed Execute or None. It describes the predicted next code breakpoint hit relative to PC;

* GDBStub::IsMemoryBreak() returns true if a recent Read/Write operation hit a watchpoint. It doesn't specify which in return, nor does it trace it anywhere. Thus, the only data we could give RecordBreak() is a placeholder BreakpointAddress at offset NULL and type Access. I found the idea silly, compared to simply relying on GDBStub::IsMemoryBreak().

There is currently no measure in the code that remembers the addresses (and types) of any watchpoints that were hit by an instruction, in order to send them to GDB as "extended stop information."
I'm considering an implementation for this.
@wwylele
Copy link
Member

wwylele commented Feb 25, 2019

Could you resolve the conflict?

I have never seen the (Reg[15] == last_bkpt.address) assert fail in practice, even after several weeks of (locally) developping various branches around GDB.  Only leave it inside Debug builds.
@DimitriPilot3
Copy link
Contributor Author

DimitriPilot3 commented Feb 27, 2019

@wwylele There, conflict resolved. Thanks for the notice!

I'm a little new to solving conflicts in a pushed PR (in particular choosing between merge-commit or force-push rebase), so the last 2 commits look a little rushed. Do I need to worry about squashing, or does the commit log look reasonable enough for review?

@merryhime
Copy link
Member

merryhime commented Feb 28, 2019

ARMul_State::ServeBreak had a conditional statement which consisted of setting the PC register (r15) to the location of a breakpoint hit. Though it relieves the necessity for Dynarmic to sync up PC prior to a call to ServeBreak, Dynarmic was in fact doing it already, so the statement is redundant!

Does this work for the SkyEye interpreter as well?

@DimitriPilot3
Copy link
Contributor Author

DimitriPilot3 commented Feb 28, 2019

... Though it relieves the necessity for Dynarmic to sync up PC prior to a call to ServeBreak, Dynarmic was in fact doing it already, so the statement is redundant!

Does this work for the SkyEye interpreter as well?

Yes. ServeBreak is only called by the CPU backends from within their Run/Step methods, soon after their call to SkyEye's InterpreterMainLoop returns, which ensures the CPU context extracted from SkyEye (and passed on to GDB) is fresh and consistent.

Following below are all the examples of either ServeBreak or SendTrap being called in the midst of CPU execution:

  • In DynCom, this call chain is ensured by its ExecuteInstructions(u64 num_instructions) method.

  • In Dynarmic, each InterpreterFallback relies on SkyEye, and concludes with a call to ServeBreak. One situation in which this fallback is forced is via ARM_Dynarmic::Step().

    InterpreterMainLoop(parent.interpreter_state.get());
    bool is_thumb = (parent.interpreter_state->Cpsr & (1 << 5)) != 0;
    parent.interpreter_state->Reg[15] &= (is_thumb ? 0xFFFFFFFE : 0xFFFFFFFC);

    (The last 2 lines look redundant - a similar statement also exists in SkyEye's interpreter main loop.)

  • Also in Dynarmic, code breakpoints are handled immediately as part of an ARM exception handler:

    case Dynarmic::A32::Exception::Breakpoint:
    if (GDBStub::IsConnected()) {
    parent.jit->HaltExecution();
    parent.SetPC(pc);
    Kernel::Thread* thread =
    parent.system.Kernel().GetThreadManager().GetCurrentThread();
    parent.SaveContext(thread->context);
    GDBStub::Break();
    GDBStub::SendTrap(thread, 5);
    return;

Come to think of it, both calls to ServeBreak could be moved over to the epilogue of SkyEye's InterpreterMainLoop function. I can't think of any side effects from doing so (other than in the exact timing of ServeBreak overwriting the CPU context object inside the current thread, but how would that mess up Dynarmic?)

@viocar
Copy link

viocar commented Mar 1, 2019

I can't tell - does this PR fix memory watchpoints as-is, or is it laying the groundwork for getting them to work? Because they seem to still not work for me.

@DimitriPilot3
Copy link
Contributor Author

DimitriPilot3 commented Mar 1, 2019

@viocar Just groundwork, I'm afraid. This PR makes the memory watchpoints work, but not completely as intended. They finally break the program's execution, but they can't (yet) tell GDB which exact address was hit, so it treats it like any other SIGTRAP signal sent from code breakpoints and code-stepping.

As long as you only have 1 watchpoint active (and no breakpoints), the command x/3i $pc-4 will show you which instruction (the one just above PC) has accessed the memory you've been watching.

AFAIK, you won't find any Canary builds with this PR, until merged into master or tagged canary-build.
I also apologize it's taking so long to wrap up - I've been taking and versioning so seriously that I chose to set aside 2 of my previous implementation attempts, in fear the amount of code in them was "too much" to review. I think my 3rd take is coming up well, and is about to fix multiple issues at once!

@viocar
Copy link

viocar commented Mar 1, 2019

Well, I've merged the changes listed in the "files changed" tab into my own build (by hand, since git is a constant headache to me), and it still doesn't even break when I hit a memory watchpoint. Are there more changes that I need to get it to work?

And don't worry about getting it done ASAP - take your time! There's no rush.

@DimitriPilot3
Copy link
Contributor Author

DimitriPilot3 commented Mar 1, 2019

Are there more changes that I need to get it to work?

There shouldn't be. Make sure the CPU JIT is disabled - it doesn't support memory watchpoints. (For them to work with Dynarmic, either a host-hardware breakpoint or an interpreter fallback would be needed.)

@viocar
Copy link

viocar commented Mar 1, 2019

Ah, I didn't know it needed CPU JIT off. It works! It also seems to show the instruction that triggered the breakpoint as-is:

Thread 2 "Thread 9" received signal SIGTRAP, Trace/breakpoint trap.
[Switching to Thread 9]
0x003337b8 in ?? ()

0x3337B8 - 4 being the triggering instruction. Thanks! It'd be cool if CPU JIT could be made to work with watchpoints, but this is a good step forward.

@merryhime
Copy link
Member

merryhime commented Mar 2, 2019

  • The last 2 lines look redundant - a similar statement also exists in SkyEye's interpreter main loop.)

The last two lines are not always redundant, e.g. when executing an instruction like ADD R15, #3, alignment isn't performed in the interpreter when single-stepping instructions. This can of course be fixed within the intepreter. :p

Come to think of it, both calls to ServeBreak could be moved over to the epilogue of SkyEye's InterpreterMainLoop function. I can't think of any side effects from doing so (other than in the exact timing of ServeBreak overwriting the CPU context object inside the current thread, but how would that mess up Dynarmic?)

That sounds fine to me.

@DimitriPilot3
Copy link
Contributor Author

DimitriPilot3 commented Mar 4, 2019

The last two lines are not always redundant, e.g. when executing an instruction like ADD R15, #3, alignment isn't performed in the interpreter when single-stepping instructions. This can of course be fixed within the intepreter. :p

AFAIK, SkyEye already re-aligns R15 each time an instruction concludes by jumping to the DISPATCH label, typically after R15 is reassigned. Single-stepping doesn't inhibit this, nor do the other exit conditions - they're nearly[1] all evaluated within the GOTO_NEXT_INST macro at the end of DISPATCH.

Now, without a full test unit, I understand that stating that SkyEye always returns from InterpreterMainLoop with an aligned program counter, is merely a conjecture. Thus, you do have a point:

The last two lines are not always redundant.

I could move the ServeBreak calls, as well as the 2 lines ensuring 100% that R15 is aligned, into the epilogue of InterpreterMainLoop, (IMO the DEBUG_ASSERT I added to ServeBreak should be there) which implies adding those lines to DynCom::ExecuteInstructions as well. Thoughts?

Another option I'm fine with: Revert the "replace assignment with DEBUG_ASSERT" change from this PR, then open a new ticket (PR?) to reference and continue this clean-up discussion. That change was an additional, precautious measure on top of a bug I already fixed - that I thought would benefit my next and final PR for watchpoint support. But I did end up finding a simple way to avoid the bug altogether.


[1] Except for this condition. No idea what it's for, but nowhere in the current source code for Citra do I see that NirqSig is being initialized or set to LOW (0).

bunnei
bunnei approved these changes Mar 8, 2019
@bunnei bunnei merged commit acaca41 into citra-emu:master Mar 8, 2019
3 checks passed
liushuyu pushed a commit to liushuyu/citra that referenced this issue Apr 19, 2019
…und to let watchpoints break into GDB. (citra-emu#4651)

* gdbstub: fix IsMemoryBreak() returning false while connected to client

As a result, the only existing codepath for a memory watchpoint hit to break into GDB (InterpeterMainLoop, GDB_BP_CHECK, ARMul_State::RecordBreak) is finally taken,
which exposes incorrect logic* in both RecordBreak and ServeBreak.

* a blank BreakpointAddress structure is passed, which sets r15 (PC) to NULL

* gdbstub: DynCom: default-initialize two members/vars used in conditionals

* gdbstub: DynCom: don't record memory watchpoint hits via RecordBreak()

For now, instead check for GDBStub::IsMemoryBreak() in InterpreterMainLoop and ServeBreak.

Fixes PC being set to a stale/unhit breakpoint address (often zero) when a memory watchpoint (rwatch, watch, awatch) is handled in ServeBreak() and generates a GDB trap.

Reasons for removing a call to RecordBreak() for memory watchpoints:
* The``breakpoint_data`` we pass is typed Execute or None. It describes the predicted next code breakpoint hit relative to PC;

* GDBStub::IsMemoryBreak() returns true if a recent Read/Write operation hit a watchpoint. It doesn't specify which in return, nor does it trace it anywhere. Thus, the only data we could give RecordBreak() is a placeholder BreakpointAddress at offset NULL and type Access. I found the idea silly, compared to simply relying on GDBStub::IsMemoryBreak().

There is currently no measure in the code that remembers the addresses (and types) of any watchpoints that were hit by an instruction, in order to send them to GDB as "extended stop information."
I'm considering an implementation for this.

* gdbstub: Change an ASSERT to DEBUG_ASSERT

I have never seen the (Reg[15] == last_bkpt.address) assert fail in practice, even after several weeks of (locally) developping various branches around GDB.  Only leave it inside Debug builds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants