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

PatchEngine: Allow patching of files on disk #7982

Open
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@phire
Copy link
Member

phire commented Apr 11, 2019

Resident Evil 2/3 require patching code which live in .rel files (Relocatable Executable)

Our current memory patching system makes this difficult, we would have to patch the rel loader to detect which .rel was loaded and apply the correct patch.

So I've implemented file based patches instead.
Here is the patch for Resident Evil 2 (U)

[FilePatch_enabled]
$fix dcache issue
[FilePatch]
$fix dcache issue
/leon.rel
0x1903f0:dword:0x42800010
/claire.rel
0x190288:dword:0x42800010

Current limitations:

  • Can't patch main.dol
  • Can't patch files not on the main partition
  • Can't patch data outside of files
  • Not hooked up to editor UI

@phire phire added the WIP label Apr 11, 2019

@phire phire requested review from delroth, lioncash and JosJuice Apr 11, 2019

@JosJuice
Copy link
Contributor

JosJuice left a comment

We talked a little on IRC about having this code in the Volume classes instead. I think that would make sense, as long as we make sure that the patches only are applied when the disc is being accessed by emulation and not the game list, maybe by having a function Volume::ApplyPatches that is called by the boot code or PatchEngine that stores the patches in the Volume object.

u64 end_offset = offset + buffer.size();

// Scan through patches which might overlap our current read request and apply them
for (auto it = s_disc_patches->begin(); it != s_disc_patches->end(); it++) {

This comment has been minimized.

Copy link
@JosJuice

JosJuice Apr 11, 2019

Contributor

Would it be possible to use upper_bound instead of looping through all patches? Kinda like we do in this function in the file system code:

std::unique_ptr<FileInfo> FileSystemGCWii::FindFileInfo(u64 disc_offset) const

This comment has been minimized.

Copy link
@delroth

delroth Apr 11, 2019

Member

This isn't looping through all patches since maps are sorted and there's a "return" in the loop once reaching an element that's too big.

However it feels like you could indeed micro-optimize this with some chosen .lower_bound() / .upper_bound() which are O(lg N). Given that this map is tiny, I don't know if it's really worth it.

This comment has been minimized.

Copy link
@JosJuice

JosJuice Apr 11, 2019

Contributor

Yeah, in the case of what we're going to use this for now, the list is tiny and it doesn't really matter if we use a loop. I was more thinking of if we want to use this for more things in the future (phire mentioned Riivolution on IRC as a potential thing), but I guess that would require bigger changes anyway since patches can't be larger than 4 bytes in the current code.

This comment has been minimized.

Copy link
@phire

phire Apr 11, 2019

Author Member

I had that optimisation, but the logic was incorrect so I patched it out for testing.

Will reverse the search order and add it back in later.

const auto& file = fs->FindFileInfo(patch.path);
if (file) {
for (const auto& entry : patch.entries) {
u64 disc_offset = disc.PartitionOffsetToRawOffset(file->GetOffset(), partition) + entry.address;

This comment has been minimized.

Copy link
@JosJuice

JosJuice Apr 11, 2019

Contributor

This is wrong for Wii discs, since entry.address doesn't account for the hashes at the beginning of each cluster. (Unless you expect the person writing the patch to correct for the hashes manually?) Patching a Wii disc using the current code is inconvenient anyway, since you would need to handle the encryption manually...

If the code isn't going to support Wii discs properly, you might as well skip the call to PartitionOffsetToRawOffset.

std::memcpy(&old_data, buffer.data() + start, 4);
std::memmove(buffer.data() + start, it->second.data(), std::min(it->second.size(), buffer.size() - start));
std::memcpy(&new_data, buffer.data() + start, 4);
INFO_LOG(DVDINTERFACE, "patch applied at %08lx - old: %08x new: %08x", offset + start, old_data, new_data);

This comment has been minimized.

Copy link
@JosJuice

JosJuice Apr 11, 2019

Contributor

Don't we need to byteswap here in order for the values to make sense?

@@ -88,6 +92,14 @@ void LoadPatchSection(const std::string& section, std::vector<Patch>& patches, I
currentPatch.active = enabledNames.find(currentPatch.name) != enabledNames.end();
currentPatch.user_defined = (ini == &localIni);
}
else if (line[0] == '/') {
// Split mult-file patches

This comment has been minimized.

Copy link
@delroth
@@ -211,6 +224,52 @@ static void ApplyPatches(const std::vector<Patch>& patches)
}
}

std::unique_ptr<std::map<u64, std::vector<u8>>> CalculateDiscPatches(const DiscIO::Volume &disc) {
if (s_file_patches.empty())

This comment has been minimized.

Copy link
@delroth

delroth Apr 11, 2019

Member

Seems like an extra unneeded check? Why not return a size 0 collection? That way you can also remove the nullptr check in users.

break;
}

//std::reverse(data.begin(), data.end()); // Reverse for endianness

This comment has been minimized.

Copy link
@delroth

delroth Apr 11, 2019

Member

Remove?

}

// Return null
return patches->empty() ? nullptr : std::move(patches);

This comment has been minimized.

Copy link
@delroth

delroth Apr 11, 2019

Member

Same comment re: null vs. empty.

@@ -211,6 +224,52 @@ static void ApplyPatches(const std::vector<Patch>& patches)
}
}

std::unique_ptr<std::map<u64, std::vector<u8>>> CalculateDiscPatches(const DiscIO::Volume &disc) {

This comment has been minimized.

Copy link
@delroth

delroth Apr 11, 2019

Member

Any reason to make this a unique_ptr instead of just relying on copy ellision / moves?

u64 start = it->first - offset;
u32 old_data;
u32 new_data;
std::memcpy(&old_data, buffer.data() + start, 4);

This comment has been minimized.

Copy link
@lioncash

lioncash Apr 11, 2019

Member
Suggested change
std::memcpy(&old_data, buffer.data() + start, 4);
std::memcpy(&old_data, buffer.data() + start, sizeof(old_data));

ditto for the memcpy below this one, but utilizing new_data instead

{
case PatchType::Patch32Bit:
data.push_back(static_cast<u8>(entry.value>>24));
data.push_back(static_cast<u8>(entry.value>>16));

This comment has been minimized.

Copy link
@lioncash

lioncash Apr 11, 2019

Member

The relevant fallthrough cases should have a comment indicating it's intentional, making sure the paths aren't forgotten about when migration to C++17 is done (where [[fallthrough]]; can be utilized)

#include <string>
#include <vector>

#include "Common/CommonTypes.h"

#include "DiscIO/Volume.h"

This comment has been minimized.

Copy link
@lioncash

lioncash Apr 11, 2019

Member

This can be a forward declaration (assuming the include would otherwise be required, despite migration of the patching code to the volume handling code)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.