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

Debugger: Get target memory in load/store instructions #7675

Open
wants to merge 3 commits into
base: master
from

Conversation

2 participants
@TryTwo
Copy link
Contributor

TryTwo commented Jan 4, 2019

Backend:
Updates disassembler instructions. Mostly spaces and commas. PS load/store uses 0x notation. Required for new features.
Adds function to PPCDebugInterface to calculate target memory address for load/store instructions.

Code widget new feature:
-Context menu for copying or showing memory target in load/store instructions. Only works if the PC is near the instruction (register consistency).

/edit removed trace and diff for their own PRs
/edit2 changed breakpoint memory to 'show in memory'.

@TryTwo

This comment has been minimized.

Copy link
Contributor

TryTwo commented Jan 4, 2019

I could follow the "show in memory" context item to send the target memory address to the memory widget, rather than breakpoint or even copy it. If that's better?

@lioncash
Copy link
Member

lioncash left a comment

I've reviewed the first three commits. The tracing and differencing change should be in its own PR (it's 1K+ LoC on its own, not counting other things), the other changes preceding it are fine by themselves in one PR, since they're mostly related.

Show resolved Hide resolved Source/Core/Core/Debugger/PPCDebugInterface.cpp Outdated
Show resolved Hide resolved Source/Core/Core/Debugger/PPCDebugInterface.cpp Outdated
Show resolved Hide resolved Source/Core/Core/Debugger/PPCDebugInterface.cpp Outdated
Show resolved Hide resolved Source/Core/Core/Debugger/PPCDebugInterface.cpp Outdated
Show resolved Hide resolved Source/Core/Core/Debugger/PPCDebugInterface.cpp Outdated
Show resolved Hide resolved Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp Outdated
if (!IsInstructionLoadStore(codeline))
return;

const u32 addr = PowerPC::debug_interface.GetMemoryAddressFromInstruction(codeline);

This comment has been minimized.

@lioncash

lioncash Jan 4, 2019

Member
Suggested change Beta
const u32 addr = PowerPC::debug_interface.GetMemoryAddressFromInstruction(codeline);
const u32 address = PowerPC::debug_interface.GetMemoryAddressFromInstruction(codeline);

You don't need to mince words, we aren't running on an interpreter on a system with so little memory that shortening names is needed to save memory ;)

This comment has been minimized.

@TryTwo

TryTwo Jan 5, 2019

Contributor

I don't mind making it longer, but all the other functions use addr instead of address, so I probably should leave it for consistency?

Show resolved Hide resolved Source/Core/DolphinQt/Debugger/CodeViewWidget.cpp Outdated
if (!IsInstructionLoadStore(codeline))
return;

const u32 addr = PowerPC::debug_interface.GetMemoryAddressFromInstruction(codeline);

This comment has been minimized.

@lioncash

lioncash Jan 4, 2019

Member
Suggested change Beta
const u32 addr = PowerPC::debug_interface.GetMemoryAddressFromInstruction(codeline);
const u32 address = PowerPC::debug_interface.GetMemoryAddressFromInstruction(codeline);
@@ -324,6 +325,40 @@ std::string PPCDebugInterface::GetDescription(unsigned int address)
return g_symbolDB.GetDescription(address);
}

u32 PPCDebugInterface::GetMemoryAddressFromInstruction(std::string instruction)

This comment has been minimized.

@lioncash

lioncash Jan 4, 2019

Member

It may be preferable to use a std::optional<u32> for this. To handle the mentioned error case relating to checking the return value of std::regex_search.

This comment has been minimized.

@TryTwo

TryTwo Jan 5, 2019

Contributor

This is a tough one. For Trace I always need this to reliably return a memory address. I filter what can feed into this, to make sure it's a load/store, and so it should reliably work. If I could think of an instance of it failing, I could decide on how handle it. I'm thinking maybe just return 0 for false.

@TryTwo

This comment has been minimized.

Copy link
Contributor

TryTwo commented Jan 5, 2019

Thanks for the detailed review, I'll work on it. Removing tracing and differencing. I see they should each have their own PR, though tracing does rely on this PR.

@TryTwo TryTwo force-pushed the TryTwo:Debugger_Code_Features branch from 3ceb748 to 3e0d0ea Jan 5, 2019

@TryTwo TryTwo changed the title Debugger code features Debugger: Get target memory in load/store instructions Jan 6, 2019

@TryTwo

This comment has been minimized.

Copy link
Contributor

TryTwo commented Jan 10, 2019

I found one major issue with GekkoDisassembler. On load/stores that use a register for the offset (lwzx) if the offset register value is 0, then the offset is 0 and NOT r0. All others cases are normal (offset = 1 -> r1). I don't know how to correctly fix that. I will edit my PPCDebugInterface to deal with the error instead.

This error also applies to things like icbi. icbi r0, r4 should be icbi 0, r4.

Also, I dealt with all listed PR issues, but left a few open for discussion.

@TryTwo TryTwo force-pushed the TryTwo:Debugger_Code_Features branch from 3e0d0ea to 3e3d380 Jan 10, 2019

@TryTwo

This comment has been minimized.

Copy link
Contributor

TryTwo commented Jan 10, 2019

Fixed offset = 0 instead of r0.
Removed breakpoint target memory address context item.
Added show target memory in the memory widget context item. -- In practice you often want to BP it on write or on read, not both, so sending it to the memory widget and BPing it there works better..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment