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

Core/MMU: Add HostTryRead/HostTryWrite functions to allow specifying the desired address space, and return whether it succeeded. #9522

Merged
merged 2 commits into from Jul 7, 2021

Conversation

AdmiralCurtiss
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss commented Feb 18, 2021

Alright, let's give this another shot. I'll summarize here, but if you want the previous attempts, see #6913, #8310, #8682, and sorta #9361.

Basically, our Cheat Search has a big problem where your search results may break (usually completely disappear, but sometimes other weird stuff if you do comparisons with 0) because the code calling the HostRead_*() functions cannot differentiate between them returning 0 because the actual value at that memory address is zero, or returning 0 because the address translation failed (eg. because the CPU is currently in an exception state).

To help with this, I've added new Try variants of the HostRead and HostWrite functions that allow specifying the expected address space of the given address and return whether the read or write succeeded or not.

@AdmiralCurtiss AdmiralCurtiss force-pushed the host-read-optional branch 2 times, most recently from 17534f0 to cccdd99 Compare February 18, 2021 04:01
Copy link
Member

@BhaaLseN BhaaLseN left a comment

Choose a reason for hiding this comment

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

Just checking, this looks like every place so far was just appended .value_or(N) and "good to go" - is this correct, or should there be places that check for std::nullopt returns instead (aside from ReadString for obvious reasons)?

Source/Core/Core/ActionReplay.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Debugger/WatchWidget.cpp Outdated Show resolved Hide resolved
@AdmiralCurtiss
Copy link
Contributor Author

Correct, that's all I did so far. There are definitely places here that should now check the return value and branch on it, but I didn't want to waste time thinking about it before I had an 'okay' that this is a good approach to the general issue.

@AdmiralCurtiss
Copy link
Contributor Author

AdmiralCurtiss commented Feb 18, 2021

Speaking of the ReadString(), I was wondering how to deal with that one. For consistency it would make sense to also return a nullopt (instead of an empty string) when the address is invalid, but at the same time it might be possible (unlikely, to be fair) that there is a string that goes right up to a memory address boundary that you might want returned. Maybe return nullopt if the initial address is invalid, but return the built up string if we encounter an invalid address while advancing?

@BhaaLseN
Copy link
Member

No idea, I stared at it for a while, and I'm not entirely sure if it makes much ofa difference.
For consistency, it should probably do that though - that is, return std::nullopt if the initial address is already invalid. Crossing over is probably fine when it simply ends there.

@sepalani
Copy link
Contributor

@AdmiralCurtiss
My issue with this approach is that you're repeating what you're trying to avoid:

functions cannot differentiate between them returning 0 because the actual value at that memory address is zero, or returning 0 because the address translation failed (eg. because the CPU is currently in an exception state).

In some of the debugging stuff, like HLE or other widgets, the address (or range) is checked (using HostIsRAMAddress) prior to read values from RAM. With this change, each Read/Write call will check redundantly the RAM address which isn't ideal. Using value_or also hides PanicAlert messages in places where it might have been intended.

What do you think about adding to the interface your functions renamed as Host_TryReadUX (or similar) which behaves exactly like your current implementation (using RAM address check, address space and optional) and using it where you think it's relevant? For instance, we already have a similar approach with the TryReadInstruction function.

@AdmiralCurtiss
Copy link
Contributor Author

Mhm, yeah I understand what you mean. That probably is a better idea indeed, let's see...

@AdmiralCurtiss
Copy link
Contributor Author

So, like this?

@sepalani
Copy link
Contributor

Yes like that. Is the assert(0) needed assuming it's ok for the "Try" functions to fail?

@AdmiralCurtiss
Copy link
Contributor Author

It's for ensuring no one passes a nonsense enum value. I could go either way though.

@AdmiralCurtiss AdmiralCurtiss force-pushed the host-read-optional branch 2 times, most recently from 52bd380 to 4e2f02e Compare February 27, 2021 15:31
@AdmiralCurtiss AdmiralCurtiss changed the title Core/MMU: Allow specifying the desired address space in the HostRead functions, and return std::nullopt on failure. Core/MMU: Add HostTryRead/HostTryWrite functions to allow specifying the desired address space, and return whether it succeeded. Feb 27, 2021
Source/Core/Core/PowerPC/MMU.h Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/MMU.h Outdated Show resolved Hide resolved
Source/Core/Core/PowerPC/MMU.h Outdated Show resolved Hide resolved
@sepalani
Copy link
Contributor

sepalani commented Jul 7, 2021

This still looks fine to me.

I guess you can add a 3rd commit regarding your use-case (Cheat Search) for these functions.

@leoetlino leoetlino merged commit 1450e97 into dolphin-emu:master Jul 7, 2021
11 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the host-read-optional branch July 7, 2021 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants