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

Fix memory breakpoint when checking the middle of the data #5009

Merged
merged 1 commit into from
Mar 20, 2017

Conversation

aldelaro5
Copy link
Member

Instead of just repeating what the commit says, here's a more practical example so it's easier to understand what the issue is and how I fixed it.

In Super Mario Sunshine, the RNG seed address is 0x8040CE30 and is a word (4 bytes long). Currently, in master, if you were to add a memory breakpoint that would break on this address, the address MUST be included in the range or must EXACTLY be this address in the case of a single address memory breakpoint. The problem comes from the fact that it didn't JUST wrote to this address, it also wrote to the next 3 one because it is writting 4 bytes so a single address memory breakpoint to 0x8040CE33 for instance would not break, even though it did wrote/read that address, it just didn't STARTED the read/write there.

So I changed how GetMemCheck works, I added a size parameter so it can adapt itself depending on how much you are writting (by default 1 because only the MMU memcheck function needs to pass an exact size, the rest actually only care about memchecks being present which is the case for the GUI display). Then, after a lot of tinkering around to get the conditions right, I actually found out that you don't even need to do something different whether the memory breakpoint is ranged or not. The reason the function can be so simpler is because in the case of non ranged memory breakpoint, the start address is equal to the end one so the same condition can apply to everything.

Because of how minimal this function does, it doesn't impact performance, even though it's called very often. I actually wasn't able to notice any performance dips on rs2 and the intro of rs3.

Last note, to test this on Windows, you need to apply #5004 otherwise they just won't work.

@lioncash
Copy link
Member

lioncash commented Mar 4, 2017

sizes should be represented with size_t.

@@ -418,7 +418,7 @@ void CMemoryView::OnPaint(wxPaintEvent& event)
draw_text(StrToWxStr(desc), 2);

// Show blue memory check dot
if (debugger->IsMemCheck(address))
if (debugger->IsMemCheck(address, 1))

This comment was marked as off-topic.

@aldelaro5 aldelaro5 force-pushed the memcheck-fix branch 2 times, most recently from b7a970a to af1cb57 Compare March 4, 2017 07:14
@@ -26,7 +26,7 @@ class DebugInterface
virtual void ToggleBreakpoint(unsigned int /*address*/) {}
virtual void AddWatch(unsigned int /*address*/) {}
virtual void ClearAllMemChecks() {}
virtual bool IsMemCheck(unsigned int /*address*/) { return false; }
virtual bool IsMemCheck(unsigned int /*address*/, unsigned int /*size*/) { return false; }

This comment was marked as off-topic.

This comment was marked as off-topic.

@@ -129,9 +129,9 @@ void PPCDebugInterface::ClearAllMemChecks()
PowerPC::memchecks.Clear();
}

bool PPCDebugInterface::IsMemCheck(unsigned int address)
bool PPCDebugInterface::IsMemCheck(unsigned int address, unsigned int size)

This comment was marked as off-topic.

This comment was marked as off-topic.

@@ -25,7 +25,7 @@ class PPCDebugInterface final : public DebugInterface
void AddWatch(unsigned int address) override;
void ToggleBreakpoint(unsigned int address) override;
void ClearAllMemChecks() override;
bool IsMemCheck(unsigned int address) override;
bool IsMemCheck(unsigned int address, unsigned int size = 1) override;

This comment was marked as off-topic.

@@ -116,7 +116,7 @@ void DSPDebugInterface::ToggleBreakpoint(unsigned int address)
}
}

bool DSPDebugInterface::IsMemCheck(unsigned int address)
bool DSPDebugInterface::IsMemCheck(unsigned int address, unsigned int size)

This comment was marked as off-topic.

@@ -27,7 +27,7 @@ class DSPDebugInterface final : public DebugInterface
void ClearAllBreakpoints() override;
void ToggleBreakpoint(unsigned int address) override;
void ClearAllMemChecks() override;
bool IsMemCheck(unsigned int address) override;
bool IsMemCheck(unsigned int address, unsigned int size) override;

This comment was marked as off-topic.

@@ -201,16 +201,11 @@ void MemChecks::Remove(u32 address)
}
}

TMemCheck* MemChecks::GetMemCheck(u32 address)
TMemCheck* MemChecks::GetMemCheck(u32 address, size_t size)

This comment was marked as off-topic.

@@ -86,7 +86,7 @@ class MemChecks
void Add(const TMemCheck& memory_check);

// memory breakpoint
TMemCheck* GetMemCheck(u32 address);
TMemCheck* GetMemCheck(u32 address, size_t size = 1);

This comment was marked as off-topic.

return &mc;
}
else if (mc.start_address == address)
if (mc.end_address >= address && (address + size - 1) >= mc.start_address)

This comment was marked as off-topic.

@degasus
Copy link
Member

degasus commented Mar 13, 2017

I'd prefer u32 instead of size_t. We're talking about memory sizes of hte GC, not of the host.

@@ -116,7 +116,7 @@ void DSPDebugInterface::ToggleBreakpoint(unsigned int address)
}
}

bool DSPDebugInterface::IsMemCheck(unsigned int address)
bool DSPDebugInterface::IsMemCheck(unsigned int address, u32 size)

This comment was marked as off-topic.

@@ -201,16 +201,11 @@ void MemChecks::Remove(u32 address)
}
}

TMemCheck* MemChecks::GetMemCheck(u32 address)
TMemCheck* MemChecks::GetMemCheck(u32 address, u32 size)

This comment was marked as off-topic.

@lioncash
Copy link
Member

lioncash commented Mar 18, 2017

@degasus This will also make it a requirement to cast every sizeof expression, to avoid type truncation. This is why most of the copy routine use size_t. That's kind of a bad idea, in my opinion, especially given those are the most appropriate inputs most of the time. It would likely be better to add an assert or check against the value range, if it's that important.

@aldelaro5
Copy link
Member Author

aldelaro5 commented Mar 19, 2017

Changed everything to size_t because I am a bit confused tbh.

EDIT: decided to stay with size_t because as @lioncash said, it reduces problem with sizeof and it's still a size, you don't loose that much meaning by not using u32.

@aldelaro5 aldelaro5 force-pushed the memcheck-fix branch 3 times, most recently from 2030bba to 1b65a0a Compare March 19, 2017 02:37
If the delimiters of a memory aren't exactly the same as an address, but their size includes the memory breakpoint delimiter, the break will not go through.  This makes it so that you can specify a search for a memory breakpoint with a data size and will check if the data fits with that size on all memory breakpoints so the breaks go through.
@Helios747 Helios747 merged commit 50faffc into dolphin-emu:master Mar 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants